Repository navigation
feat(variant): allow creating VariantArray from Option items convertible to Variant - #11053
Conversation
…ble to Variant Generalize FromIterator<Option<Variant>> for VariantArray to FromIterator<Option<V>> where V: Into<Variant>, so values that convert cleanly to Variant (e.g. i64, &str) can be collected directly. Adds regression tests for i64 and str items, including null rows and short vs long string storage.
sdf-jkl
left a comment
There was a problem hiding this comment.
Thanks @jabrailkhalil. Everything looks great.
I'd add examples of building a VariantArray::from_iter.
This change also introduces a breaking API change with None handling. Earlier, None could only mean None::<Variant>, but now it can be any type coercible into Variant.
[None, Some(42_i64)] // Element type inferred as Option<i64>
[None, None] // Element type unknownWe should add an example of building a correct Variant::Null array to the docs addition.
let null_rows = VariantArray::from_iter(vec![None::<Variant>; 3]);|
@sdf-jkl Addressed in 75e9e43. The VariantArray docs now show direct collection from Option, explain the distinction between None and Some(Variant::Null), and include the typed None:: all-null case you requested. cargo fmt --check passes locally. I also attempted the targeted crate test, but this Windows environment has no MSVC link.exe; CI will run the Rust test suite. |
sdf-jkl
left a comment
There was a problem hiding this comment.
Thanks @jabrailkhalil this is great. This should also let us to simplify some of the VariantArray generations in tests where arrays are homogeneous. LMK if you want to add the simplifications here or in a separate PR.
@jabrailkhalil will you have time to address @sdf-jkl 's comments? Or shall we merge this PR and do the simplications as a follow on? |
|
|
|
🚢 🇮🇹 |
# Which issue does this PR close? Follow-up to #11053. # Rationale for this change #11053 allows `VariantArray` to collect `Option<T>` values directly when `T` converts into `Variant`. Homogeneous test fixtures can now use `VariantArray::from_iter([Some(42_i64), None])`, removing explicit `Variant` conversions and row-by-row builder setup. # What changes are included in this PR? Simplify fixtures in the variant array iterator, extraction/conversion, shredding, and unshredding tests. Preserve value types, row order, null semantics, and existing assertions. Tests that exercise builders themselves remain unchanged. # Are these changes tested? Existing tests cover the fixture changes: - `cargo test -p parquet-variant-compute --lib` — 369 passed. - `cargo clippy -p parquet-variant-compute --all-targets -- -D warnings` — passed. - `cargo fmt --all -- --check` — passed. # Are there any user-facing changes? No. This is a test-only cleanup; production code and public APIs are unchanged.
|
Yes, I will continue this work. The documentation comments from the original PR were addressed before merge. I have now opened the small follow-up #11320 to simplify five homogeneous VariantArray doctest fixtures using the newly merged API. The assertions and resulting values are preserved; 369 crate unit tests and all 15 doctests pass, and formatting passes. |
### Which issue does this PR close? Follow-up to #11053 and the maintainer discussion there. Related to #8628. ### Rationale for this change The `FromIterator<Option<T>>` API merged in #11053 accepts homogeneous inputs directly. Five `VariantArray` doctest examples still construct a one-element string array with a mutable builder and explicit `Variant::from` conversion. ### What changes are included in this PR? Use `VariantArray::from_iter([Some("such wow")])` in these five examples and remove imports made unnecessary by the simpler fixture. The example assertions and resulting values are preserved. ### Are these changes tested? - `cargo +stable fmt --all -- --check`: passed. - `cargo +stable test -p parquet-variant-compute`: **369 unit tests and 15 doctests passed**. - Linux x86_64, Rust 1.98.1 stable. ### Are there any user-facing changes? The documentation examples are simpler. There are no runtime or API changes. Signed-off-by: jabrailkhalil <jabrailkhalil@gmail.com>
Rationale
Closes #8628.
Previously
VariantArraycould be collected only fromOption<Variant>items:With this change, any type that converts cleanly into
Variantcan be collected directly:This follows the suggestion made in #8625 (comment).
Changes
parquet-variant-compute/src/variant_array.rs: generalizeFromIterator<Option<Variant<'m, 'v>>>toimpl<'m, 'v, V: Into<Variant<'m, 'v>>> FromIterator<Option<V>>, converting each item throughInto::into. Existing code that collectsOption<Variant>items keeps working (identityInto).i64items with nulls,&stritems covering both short-string (ShortString) and long-string (String) storage.Verification
cargo test -p parquet-variant-compute --lib— 366 tests passed (364 existing + 2 new), including the newtest_from_option_into_variants_into_variant_arrayandtest_from_option_str_into_variant_array.cargo clippy -p parquet-variant-compute --all-targets -- -D warnings— clean.cargo fmt --all -- --check— clean.Note: local verification used the
x86_64-pc-windows-gnutarget; upstream CI covers the full matrix.AI disclosure
Per the Arrow project's AI-generated submissions guidance: this change was implemented with the assistance of an AI coding agent. The author reviewed the final diff, verified it compiles and passes tests/clippy/fmt locally, and takes full ownership of the change.
The ASF ICLA may need to be on file before merge (asf-clabot will report the status).