[Variant] Align cast logic for variant_get to cast kernel for numeric/bool types by klion26 · Pull Request #9563 · apache/arrow-rs

klion26 · 2026-03-16T11:50:21Z

Which issue does this PR close?

Closes [Variant] Align cast logic for variant_get to cast kernel for numeric/bool types #9564 .

What changes are included in this PR?

Align tests with cast kernel

Are these changes tested?

Covered by the existing tests

Are there any user-facing changes?

Yes, changed the logic for `Variant::asboolean/as_int8/as_int16/as_int32/as_int64/as_u8/as_u16/as_u32/as_u64/as_f16/as_f32/as_f64

klion26

@scovich This is what is in my mind to align cast logic for variant_get with cast kernel, current, only changed int/float/double/bool (no decimal for now).

Currently, this will change some behaviors (details in inline comments). Please help review this when you're in free, thanks.

klion26 · 2026-03-16T11:54:15Z

-            Variant::Int64(i) if fits_precision::<11>(i) => Some(f16::from_f32(i as _)),
-            _ => None,
-        }
+        self.as_num::<f16>()


This will change the behavior from before. Now 65536 will return Some(inf) instead of None.

klion26 · 2026-03-16T12:04:10Z

+            Variant::Int64(i) => num_cast::<i64, T>(i),
+            Variant::Float(f) => num_cast::<f32, T>(f),
+            Variant::Double(d) => num_cast::<f64, T>(d),
+            Variant::Decimal4(d) if d.scale() == 0 => num_cast::<_, T>(d.integer()),


from/to_decimal needs to be added in a following commit if the direction is right

sdf-jkl · 2026-03-18T20:19:40Z

I did a quick run, and it looks cool! I'll take a closer look tomorrow

scovich

High level question: Should we consider keeping the lossless as_xxx methods, with new cast_as_xxx methods that are fully aggressive?

Asking because even lossless casting is complex enough that users could easily get it wrong, which could be a reason for the library to provide it.

Alternatively, we might consider expanding CastOptions to differentiate between lossless casts (e.g. 42i8 -> 42u64) vs. lossy casts (also allows e.g. f64::PI -> 3i32) vs. converting casts (also allows e.g. "true" -> true, 0 -> false, "123" -> 123i8)?

scovich · 2026-03-18T20:35:07Z

+{
+    if value {
+        // a workaround to cast a primitive to type O, infallible
+        num_traits::cast::cast(1)


Seems like T::Native already provides ONE and ZERO as constants?
https://docs.rs/arrow/latest/arrow/array/trait.ArrowNativeTypeOp.html

I guess we could have an impedance mismatch between ArrowNativeType (arrow) and NumCast (num_cast) tho?

Yes, ArrowNativeTypeOp contains ONE and ZEOR, but Rust type (i32, etc) does not have ZEOR/ONE.

scovich · 2026-03-18T20:36:43Z

+where
+    I: Default + PartialEq,
+{
+    value != I::default()


I wasn't aware that rust or SQL allows coercing numbers to bool?
But arrow-cast already allows this, I guess?

Meanwhile, this should probably compare directly against I::ZERO?
(to match the other direction, below)

Yes, arrorw-cast already allows this, This is the current equivalent conversion for arrow-cast.

arrow-rs/arrow-cast/src/cast/mod.rs

Lines 2534 to 2548 in 88422cb

fn numeric_to_bool_cast<T>(from: &PrimitiveArray<T>) -> Result<BooleanArray, ArrowError>

where

T: ArrowPrimitiveType + ArrowPrimitiveType,

{

let mut b = BooleanBuilder::with_capacity(from.len());

for i in 0..from.len() {

if from.is_null(i) {

b.append_null();

} else if from.value(i) != T::default_value() {

b.append_value(true);

} else {

b.append_value(false);

}

}

scovich · 2026-03-18T20:40:17Z

        match self {
            Variant::BooleanTrue => Some(true),
            Variant::BooleanFalse => Some(false),
+            Variant::Int8(i) => Some(cast_num_to_bool::<i8>(*i)),


Are these type annotations actually necessary? Seems like the compiler would infer them easily?

Yes, the type annotations can be removed.

scovich · 2026-03-18T20:42:43Z

+            Variant::Int64(i) => Some(cast_num_to_bool::<i64>(*i)),
+            Variant::Float(f) => Some(cast_num_to_bool::<f32>(*f)),
+            Variant::Double(d) => Some(cast_num_to_bool::<f64>(*d)),
+            Variant::ShortString(s) => cast_single_string_to_boolean_default(s.0),


ShortString provides Deref?

Suggested change

Variant::ShortString(s) => cast_single_string_to_boolean_default(s.0),

Variant::ShortString(s) => cast_single_string_to_boolean_default(*s),

s here is &ShortString, *s will give ShortString, we need to use &**s to convert to &str, adjusted to s.as_ref()

klion26

@scovich Thanks for the review. I've addressed some of the comments and replied to the others.

For the function names Variant::as_xx and Variant::cast_as_xxx, I think as_xx is fine, as conversion name guide gives that conversion prefix like as_/to_, and we can do 3.2f64 as i64 in Rust code, but maybe we can add lossy suffix in the name?

Alternatively, we might consider expanding CastOptions to differentiate between lossless casts (e.g. 42i8 -> 42u64) vs. lossy casts (also allows e.g. f64::PI -> 3i32) vs. converting casts (also allows e.g. "true" -> true, 0 -> false, "123" -> 123i8)?

I've tried adding the lossy flag to CastOptions in #9169; the conclusion is to keep it as it is for now.

klion26 · 2026-03-19T05:48:16Z

+where
+    I: Default + PartialEq,
+{
+    value != I::default()


Yes, arrorw-cast already allows this, This is the current equivalent conversion for arrow-cast.

arrow-rs/arrow-cast/src/cast/mod.rs

Lines 2534 to 2548 in 88422cb

fn numeric_to_bool_cast<T>(from: &PrimitiveArray<T>) -> Result<BooleanArray, ArrowError>

where

T: ArrowPrimitiveType + ArrowPrimitiveType,

{

let mut b = BooleanBuilder::with_capacity(from.len());

for i in 0..from.len() {

if from.is_null(i) {

b.append_null();

} else if from.value(i) != T::default_value() {

b.append_value(true);

} else {

b.append_value(false);

}

}

klion26 · 2026-03-19T05:51:37Z

+{
+    if value {
+        // a workaround to cast a primitive to type O, infallible
+        num_traits::cast::cast(1)


Yes, ArrowNativeTypeOp contains ONE and ZEOR, but Rust type (i32, etc) does not have ZEOR/ONE.

klion26 · 2026-03-19T05:58:43Z

        match self {
            Variant::BooleanTrue => Some(true),
            Variant::BooleanFalse => Some(false),
+            Variant::Int8(i) => Some(cast_num_to_bool::<i8>(*i)),


Yes, the type annotations can be removed.

klion26 · 2026-03-19T05:59:37Z

+            Variant::Int64(i) => Some(cast_num_to_bool::<i64>(*i)),
+            Variant::Float(f) => Some(cast_num_to_bool::<f32>(*f)),
+            Variant::Double(d) => Some(cast_num_to_bool::<f64>(*d)),
+            Variant::ShortString(s) => cast_single_string_to_boolean_default(s.0),


s here is &ShortString, *s will give ShortString, we need to use &**s to convert to &str, adjusted to s.as_ref()

sdf-jkl

Everything LGMT, left a few nits.

sdf-jkl · 2026-03-19T15:55:25Z

        }
    }

+    fn as_num<T>(&self) -> Option<T>


Add a doc to this function?

sdf-jkl · 2026-03-19T16:02:54Z

-            Variant::Decimal16(d) if d.scale() == 0 => d.integer().try_into().ok(),
-            _ => None,
-        }
+        self.as_num::<_>()


why type inference here, but not for other as_int funcs?

+self.as_num::<_>() -self.as_num::<32>()

Thanks for pointing this out. The inference has been removed now

klion26

@sdf-jkl Thanks for the review, comments addressed. Please take another look when you're free.

klion26 · 2026-03-20T08:39:32Z

-            Variant::Decimal16(d) if d.scale() == 0 => d.integer().try_into().ok(),
-            _ => None,
-        }
+        self.as_num::<_>()


Thanks for pointing this out. The inference has been removed now

klion26 · 2026-03-20T08:39:42Z

        }
    }

+    fn as_num<T>(&self) -> Option<T>


sdf-jkl

Thanks @klion26! I only have nits and typos

sdf-jkl · 2026-03-25T15:03:25Z

    fn test_error_message_numeric_type_display() {
        let mut builder = VariantArrayBuilder::new(1);
-        builder.append_variant(Variant::BooleanTrue);
+        builder.append_variant(Variant::Null);


Using Variant::Null to cause error in strict casting will cause conflict with the PR I'm working on #9599.
Let's see who merges first 😄

Changed to other variants

klion26

@sdf-jkl I've addressed the comments, please another look

klion26 · 2026-03-27T07:27:28Z

    fn test_error_message_numeric_type_display() {
        let mut builder = VariantArrayBuilder::new(1);
-        builder.append_variant(Variant::BooleanTrue);
+        builder.append_variant(Variant::Null);


Changed to other variants

sdf-jkl

LGMT, thank you!

scovich

The code seems good.

I don't love silently lossy casts with no option to be strict(er), but that's unfortunately how arrow casts seem to roll, not a decision this PR takes.

scovich · 2026-03-27T21:52:45Z

    Ok(Arc::new(output_array))
 }

+fn cast_single_string_to_boolean(


Does this need #[inline] directive to avoid a performance hazard?

scovich · 2026-03-27T21:57:11Z

+        match *self {
+            Variant::BooleanFalse => single_bool_to_numeric::<T>(false),
+            Variant::BooleanTrue => single_bool_to_numeric::<T>(true),
+            Variant::Int8(i) => num_cast::<_, T>(i),


I'm surprised type inference doesn't pick up that T? Does it fail to compile if you do this?

Suggested change

Variant::Int8(i) => num_cast::<_, T>(i),

Variant::Int8(i) => num_cast(i),

scovich · 2026-03-27T21:59:17Z

+    /// let v5 = Variant::from("hello!");
+    /// assert_eq!(v5.as_f32(), None);
    /// ```
    #[allow(clippy::cast_possible_truncation)]


Probably don't need this any more since the truncation is hidden behind too many layers for clippy to "see" now?

scovich · 2026-03-27T22:01:39Z

+            // It will always fit in i16 because u8 max is 255 and i16 max is 32767
+            Variant::Int16(num_cast(value).unwrap())


nit: Seems simpler to just say

Suggested change

// It will always fit in i16 because u8 max is 255 and i16 max is 32767

Variant::Int16(num_cast(value).unwrap())

Variant::Int16(num_cast(value).unwrap()) // u8 -> i16 is infallible

(more below)

klion26 · 2026-03-31T10:38:18Z

@scovich I've addressed the comments, please take another look when you're fee, thanks.

scovich · 2026-03-31T13:54:35Z

Hmm, what about:

If there are user-facing changes then we may require documentation to be
updated before approving the PR.

Do we have a docs gap to fill?

sdf-jkl · 2026-03-31T15:10:35Z

 /// [Parquet Variant]: https://github.com/apache/parquet-format/blob/master/VariantEncoding.md
 /// [specification]: https://github.com/apache/parquet-format/blob/master/VariantEncoding.md
 /// [Variant Shredding specification]: https://github.com/apache/parquet-format/blob/master/VariantShredding.md
 ///


Hmm, what about:

If there are user-facing changes then we may require documentation to be
updated before approving the PR.

Do we have a docs gap to fill?

It seems like there is no single place that has the casting logic defined. We could add it to the Variant doc

Suggested change

///

/// # Casting Semantics

///

/// Scalar conversion semantics intentionally follow Arrow cast behavior where applicable.

/// Conversions in this module delegate to Arrow compute cast helpers such as

/// [`num_cast`], [`cast_num_to_bool`], [`single_bool_to_numeric`], and

/// [`cast_single_string_to_boolean_default`].

///

/// - [`Self::as_boolean`] accepts boolean, numeric, and string variants.

/// Numeric zero maps to `false`; non-zero maps to `true`. String parsing follows

/// Arrow UTF8-to-boolean cast rules.

/// - Numeric accessors such as [`Self::as_int8`], [`Self::as_int64`], [`Self::as_u8`],

/// [`Self::as_u64`], [`Self::as_f16`], [`Self::as_f32`], and [`Self::as_f64`] accept

/// boolean and numeric variants (integers, floating-point, and decimals with scale `0`).

/// They return `None` when conversion is not possible.

/// - Decimal accessors such as [`Self::as_decimal4`], [`Self::as_decimal8`], and

/// [`Self::as_decimal16`] accept compatible decimal variants and integer variants.

/// They return `None` when conversion is not possible.

It seems like that would do the job

Need to change the PR description as well.

Are there any user-facing changes?

No

klion26 · 2026-04-01T08:31:05Z

If there are user-facing changes then we may require documentation to be
updated before approving the PR.

It's a very good idea to do this, it can keep the doc as fresh as possible.

The doc has been updated.

scovich · 2026-04-01T16:10:08Z

Looks like some kind of CI hiccup on the integration test. I triggered a retry, let's see if it succeeds.

alamb · 2026-04-06T19:37:04Z

Thanks @klion26 @sdf-jkl and @scovich

alamb · 2026-04-06T19:37:19Z

(BTw @scovich you should feel free to merge these PRs without waiting for me)

klion26 · 2026-04-07T11:48:57Z

@scovich @sdf-jkl @alamb Thanks for the review and merging!

WIP

41dcbf5

klion26 changed the title ~~WIP~~ [Variant] Align cast logic for variant_get to cast kernel for numeric/bool types Mar 16, 2026

fix test

74d9e51

github-actions Bot added arrow Changes to the arrow crate parquet-variant parquet-variant* crates labels Mar 16, 2026

klion26 commented Mar 16, 2026

View reviewed changes

fix tests

2f63276

scovich reviewed Mar 18, 2026

View reviewed changes

klion26 force-pushed the variant_cast_number_and_bool branch from 4f7214e to e0ee913 Compare March 19, 2026 08:48

klion26 commented Mar 19, 2026

View reviewed changes

address comment

a0f711c

klion26 force-pushed the variant_cast_number_and_bool branch from e0ee913 to a0f711c Compare March 19, 2026 09:08

sdf-jkl reviewed Mar 19, 2026

View reviewed changes

klion26 commented Mar 20, 2026

View reviewed changes

address comments

966091f

klion26 force-pushed the variant_cast_number_and_bool branch from 1cfca42 to 966091f Compare March 20, 2026 10:06

sdf-jkl mentioned this pull request Mar 24, 2026

[Variant] extend shredded null handling for arrays #9599

Merged

sdf-jkl suggested changes Mar 25, 2026

View reviewed changes

klion26 commented Mar 27, 2026

View reviewed changes

klion26 force-pushed the variant_cast_number_and_bool branch from 522ef0c to 2b33884 Compare March 27, 2026 07:47

address comments

a9214bc

klion26 force-pushed the variant_cast_number_and_bool branch from 2b33884 to a9214bc Compare March 27, 2026 08:04

sdf-jkl approved these changes Mar 27, 2026

View reviewed changes

scovich approved these changes Mar 27, 2026

View reviewed changes

address comment to refine code

d7dda39

sdf-jkl reviewed Mar 31, 2026

View reviewed changes

update doc

75eb71f

alamb merged commit 989ac03 into apache:main Apr 6, 2026
34 of 35 checks passed

klion26 deleted the variant_cast_number_and_bool branch April 7, 2026 11:48

alamb mentioned this pull request Apr 23, 2026

[Variant] Align cast logic for variant_get to cast kernel for numeric/bool types #9564

Closed

	fn numeric_to_bool_cast<T>(from: &PrimitiveArray<T>) -> Result<BooleanArray, ArrowError>
	where
	T: ArrowPrimitiveType + ArrowPrimitiveType,
	{
	let mut b = BooleanBuilder::with_capacity(from.len());

	for i in 0..from.len() {
	if from.is_null(i) {
	b.append_null();
	} else if from.value(i) != T::default_value() {
	b.append_value(true);
	} else {
	b.append_value(false);
	}
	}

	Variant::ShortString(s) => cast_single_string_to_boolean_default(s.0),
	Variant::ShortString(s) => cast_single_string_to_boolean_default(*s),

	Variant::Int8(i) => num_cast::<_, T>(i),
	Variant::Int8(i) => num_cast(i),

		// It will always fit in i16 because u8 max is 255 and i16 max is 32767
		Variant::Int16(num_cast(value).unwrap())

+///
+/// # Casting Semantics
+///
+/// Scalar conversion semantics intentionally follow Arrow cast behavior where applicable.
+/// Conversions in this module delegate to Arrow compute cast helpers such as
+/// [`num_cast`], [`cast_num_to_bool`], [`single_bool_to_numeric`], and
+/// [`cast_single_string_to_boolean_default`].
+///
+/// - [`Self::as_boolean`] accepts boolean, numeric, and string variants.
+///   Numeric zero maps to `false`; non-zero maps to `true`. String parsing follows
+///   Arrow UTF8-to-boolean cast rules.
+/// - Numeric accessors such as [`Self::as_int8`], [`Self::as_int64`], [`Self::as_u8`],
+///   [`Self::as_u64`], [`Self::as_f16`], [`Self::as_f32`], and [`Self::as_f64`] accept
+///   boolean and numeric variants (integers, floating-point, and decimals with scale `0`).
+///   They return `None` when conversion is not possible.
+/// - Decimal accessors such as [`Self::as_decimal4`], [`Self::as_decimal8`], and
+///   [`Self::as_decimal16`] accept compatible decimal variants and integer variants.
+///   They return `None` when conversion is not possible.

Conversation

klion26 commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

klion26 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdf-jkl commented Mar 18, 2026

Uh oh!

scovich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klion26 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdf-jkl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klion26 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdf-jkl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

klion26 commented Mar 16, 2026 •

edited

Loading