fix(variant): reject FixedSizeList shredding - #10639
Conversation
Jefffrey
left a comment
There was a problem hiding this comment.
the docs dont mention support for fixed size list: https://arrow.apache.org/docs/format/CanonicalExtensions.html#parquet-variant
should we be supporting this?
f2da410 to
1801209
Compare
|
You're right. FixedSizeList isn't part of the Variant shredding spec. I changed the PR to reject it in shred_variant instead of extending the reader APIs, and added it to the invalid shredding type coverage. |
|
This is a more difficult one because it is still an open question and has a long history. My head is spinning after thinking about it all day. The Arrow canonical extension documentation does not list At the Parquet level, an Arrow The relevant arrow-rs history is:
Using I think reusing the same abstraction to determine which types If we decide that Alternatively, if we want a lenient-reader policy for data previously produced by arrow-rs, we should explicitly accept or normalize FSL on read and add a persisted read/unshred test. Leaving only the existing internal unshredding branch does not accomplish that. DuckDB provides the clearest comparison. Its fixed-length list equivalent, |
|
I’d keep this strict for now: reject FixedSizeList in shred_variant and remove the unreachable FSL branch in unshred_variant, since VariantArray::try_new already rejects it. If we want to support older data written by arrow-rs, we can handle that separately by normalizing FSL to a regular list when reading it. |
💯 |
| )?; | ||
| VariantToShreddedVariantRowBuilder::Array(typed_value_builder) | ||
| } | ||
| DataType::FixedSizeList(..) => { |
There was a problem hiding this comment.
Is the separation of this branch and underscore our expected behavior? Seems the error is the same.
Which issue does this PR close?
Rationale for this change
The Variant shredding spec does not define
FixedSizeListas a valid shreddedtyped_value. Rather than extending support beyond the spec, reject it consistently at theshred_variantboundary.What changes are included in this PR?
DataType::FixedSizeListwhen selecting a shredding type.VariantArray::try_newrejection unchanged.FixedSizeListin the invalid shredding type test set.Are these changes tested?
cargo test -p parquet-variant-compute --lib(362 passed)cargo fmt --all -- --checkgit diff --checkAre there any user-facing changes?
shred_variantnow returns an error forFixedSizeListinstead of producing a shredded representation that other Variant APIs reject.