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.
There was a problem hiding this comment.
The two branches return the same error, but I kept them separate to make the unsupported type cases explicit. I can merge them if we prefer a single error path here.
There was a problem hiding this comment.
I think we can merge them.
If we keep it explicit it would require a comment, because someone else reading the code would come to the same conclusion of dropping it.
Without the historical context a separate arm provides no value.
|
The scope of the issue was normalizing the FSL
|
|
@cakeni should we add the changes I proposed above here? |
25a8670 to
9d629f0
Compare
7c91c98 to
6901aa9
Compare
|
Updated this to normalize |
Which issue does this PR close?
Rationale for this change
The Variant shredding spec does not define
FixedSizeListas a valid shreddedtyped_value. Keep writes strict by rejecting it inshred_variant, while normalizing legacy ArrowFixedSizeListmetadata toListon read for backwards compatibility.What changes are included in this PR?
DataType::FixedSizeListwhen selecting a shredding type.FixedSizeListtyped_valuetoListinVariantArray::try_new.unshred_variant.Are these changes tested?
cargo test -p parquet-variant-compute --libAre there any user-facing changes?
shred_variantrejectsFixedSizeList; existing data with ArrowFixedSizeListmetadata is read as a regularList.