[Variant] clean up `variant_get` tests by sdf-jkl · Pull Request #9518 · apache/arrow-rs

sdf-jkl · 2026-03-05T22:23:18Z

Which issue does this PR close?

closes [Variant] variant_get tests clean up #9517.

Rationale for this change

check issue

What changes are included in this PR?

Use variant_shred in test macros
Use VariantArray::from_parts instead of using StructArrayBuilder

Are these changes tested?

yes, changes pass same tests

Are there any user-facing changes?

no

sdf-jkl · 2026-03-05T22:38:22Z

The test error was caused by:

thread 'variant_get::test::get_variant_perfectly_shredded_time_as_time32_second' (3372) panicked at parquet-variant-compute/src/type_conversion.rs:97:11:
attempt to multiply with overflow

the line here:

impl_primitive_from_variant!(datatypes::Time64MicrosecondType, as_time_utc, |v| {
    Some((v.num_seconds_from_midnight() * 1_000_000 + v.nanosecond() / 1_000) as i64)
});

Multiplication happens before casting v.num_seconds_from_midnight() or v.nanosecond() to i64 and could cause an overflow.

Fixed here: 718efbf

sdf-jkl · 2026-03-05T22:41:26Z

@codephage2020 @scovich @klion26 please check when you are available. Thanks!

codephage2020

LGTM. One small suggestion.

parquet-variant-compute/src/variant_get.rs

klion26

Thanks for the improvement. Overall looks great to me. I left some minor comments to be considered.

Another question is whether we need to support shred to LargeUtf8/LargeBinary, I can only find this comment, it seems is legal to support (it may not need to be included in the current pr even if we want to support it)

parquet-variant-compute/src/variant_get.rs

…t_get-clean-up

sdf-jkl · 2026-03-10T16:11:12Z

Thanks for the improvement. Overall looks great to me. I left some minor comments to be considered.

Another question is whether we need to support shred to LargeUtf8/LargeBinary, I can only find this comment, it seems is legal to support (it may not need to be included in the current pr even if we want to support it)

You're right. I found the mention in shred_variant.rs. They are not supported here yet, however they are supported by VariantArray::try_new and canonicalize_and_verify_data_type as mentioned by @friendlymatthew in #9515:

arrow-rs/parquet-variant-compute/src/shred_variant.rs

Lines 1147 to 1166 in d2e2cda

    
           #[test] 
        
           fn test_invalid_shredded_types_rejected() { 
        
               let input = VariantArray::from_iter([Variant::from(42)]); 
        
               let invalid_types = vec![ 
        
                   DataType::UInt8, 
        
                   DataType::Float16, 
        
                   DataType::Decimal256(38, 10), 
        
                   DataType::Date64, 
        
                   DataType::Time32(TimeUnit::Second), 
        
                   DataType::Time64(TimeUnit::Nanosecond), 
        
                   DataType::Timestamp(TimeUnit::Millisecond, None), 
        
                   DataType::LargeBinary, 
        
                   DataType::LargeUtf8, 
        
                   DataType::FixedSizeBinary(17), 
        
                   DataType::Union( 
        
                       UnionFields::from_fields(vec![ 
        
                           Field::new("int_field", DataType::Int32, false), 
        
                           Field::new("str_field", DataType::Utf8, true), 
        
                       ]),

I'd add support for these types for shred_variant before moving forward.

sdf-jkl · 2026-03-10T18:11:08Z

I filed a ticket for the LargeBinary|LargeUtf8 shred_variant support - #9525

sdf-jkl · 2026-03-10T21:45:01Z

Moved some tests to be close to each other, might split to a separate PR to make this easier to review. cc1869f

Mostly code movement and removed some comments.

klion26 · 2026-03-11T07:15:07Z

parquet-variant-compute/src/variant_get.rs

+        arrow::array::BooleanArray::from(vec![Some(true), None, None, Some(false)])
+    });
+
+    partially_shredded_variant_array_gen!(partially_shredded_utf8_variant_array, || {


IIUC, this will generate the data that(the second is invalid) typed_value contains hello, n/a, and world(Variant::from("n/a") in the macro will also be shredded into typed_value) , there is no item located in value, maybe we can improve this case to satisfy that there are some items in typed_value and some in value

thanks, I addressed it here 5af483f

…t_get-clean-up

klion26

LGTM, thanks for the improvement

…t_get-clean-up

sdf-jkl · 2026-03-16T14:57:07Z

parquet-variant-compute/src/shred_variant.rs

    }

    #[test]
-    // TODO(#9518): Drop this once variant_get tests build shredded fixtures via shred_variant.


I realized the tests we use in variant_get.rs create a perfectly shredded array, so it's not doing full multi type validation.

We should keep the shred_variant.rs tests as is.

alamb · 2026-03-17T20:22:23Z

parquet-variant-compute/src/type_conversion.rs

 });
 impl_primitive_from_variant!(datatypes::Time64MicrosecondType, as_time_utc, |v| {
-    Some((v.num_seconds_from_midnight() * 1_000_000 + v.nanosecond() / 1_000) as i64)
+    Some(v.num_seconds_from_midnight() as i64 * 1_000_000 + v.nanosecond() as i64 / 1_000)


why is this necessary?

#9518 (comment)

alamb · 2026-03-17T20:23:24Z

parquet-variant-compute/src/variant_get.rs

        assert_eq!(result.value(0), Variant::from("hello"));
        assert!(!result.is_valid(1));
-        assert_eq!(result.value(2), Variant::from("n/a"));
+        assert_eq!(result.value(2), Variant::from(42i32));


why did this change?

(Maybe we can change what is created in partially_shredded_utf8_variant_array?)

The output of this function should be a partially shredded VariantArray. If we reuse the same fall back value - "n/a", the Utf8 VariantArray would end up perfectly shredded.

Mentioned here: #9518 (comment)

alamb

Makes sense to me -- thank you @sdf-jkl

alamb · 2026-03-17T20:24:20Z

parquet-variant-compute/src/variant_get.rs

        assert_eq!(result.value(0), Variant::from("hello"));
        assert!(!result.is_valid(1));
-        assert_eq!(result.value(2), Variant::from("n/a"));
+        assert_eq!(result.value(2), Variant::from(42i32));


(Maybe we can change what is created in partially_shredded_utf8_variant_array?)

Cleanup

f64d4df

github-actions bot added the parquet-variant parquet-variant* crates label Mar 5, 2026

sdf-jkl marked this pull request as draft March 5, 2026 22:31

bug fix

718efbf

sdf-jkl marked this pull request as ready for review March 5, 2026 22:40

codephage2020 reviewed Mar 6, 2026

View reviewed changes

parquet-variant-compute/src/variant_get.rs Outdated Show resolved Hide resolved

klion26 reviewed Mar 6, 2026

View reviewed changes

parquet-variant-compute/src/variant_get.rs Outdated Show resolved Hide resolved

parquet-variant-compute/src/variant_get.rs Outdated Show resolved Hide resolved

parquet-variant-compute/src/variant_get.rs Outdated Show resolved Hide resolved

Merge branch 'main' of https://github.com/apache/arrow-rs into varian…

0aea350

…t_get-clean-up

code movement

cc1869f

sdf-jkl mentioned this pull request Mar 10, 2026

[Variant] Add shred_variant support for LargeUtf8 and LargeBinary types #9525

Closed

sdf-jkl added 3 commits March 10, 2026 17:36

More idiomatic macro

ff485cd

use ShreddedVariantFieldArray::from_parts

a56a368

Move comment

bdb036f

klion26 reviewed Mar 11, 2026

View reviewed changes

scovich mentioned this pull request Mar 13, 2026

add shred_variant support for LargeUtf8 and LargeBinary #9554

Merged

sdf-jkl added 2 commits March 15, 2026 20:55

Merge branch 'main' of https://github.com/apache/arrow-rs into varian…

7055ebb

…t_get-clean-up

make utf8 fixture have both typed and untyped values

5af483f

klion26 approved these changes Mar 16, 2026

View reviewed changes

sdf-jkl added 2 commits March 16, 2026 10:32

Merge branch 'main' of https://github.com/apache/arrow-rs into varian…

bc3c4c1

…t_get-clean-up

update comments

2596ecf

sdf-jkl commented Mar 16, 2026

View reviewed changes

alamb reviewed Mar 17, 2026

View reviewed changes

alamb approved these changes Mar 17, 2026

View reviewed changes

Conversation

sdf-jkl commented Mar 5, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

sdf-jkl commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdf-jkl commented Mar 5, 2026

Uh oh!

codephage2020 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

klion26 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sdf-jkl commented Mar 10, 2026

Uh oh!

sdf-jkl commented Mar 10, 2026

Uh oh!

sdf-jkl commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sdf-jkl commented Mar 5, 2026 •

edited

Loading

sdf-jkl commented Mar 10, 2026 •

edited

Loading