fix(parquet): keep virtual columns in the schema reported with a schema hint - #11047
Open
bharadwaj-pendyala wants to merge 1 commit into
Open
fix(parquet): keep virtual columns in the schema reported with a schema hint#11047bharadwaj-pendyala wants to merge 1 commit into
bharadwaj-pendyala wants to merge 1 commit into
Conversation
…ma hint with_supplied_schema passes virtual_columns to parquet_to_arrow_field_levels_with_virtual and counts them in its own length check, but returned the bare supplied schema, so ArrowReaderMetadata::schema() dropped them. The no-hint branch of try_new keeps them, and the reader decodes them either way. Append the virtual fields to the supplied fields, preserving the supplied schema's metadata. Closes apache#11046
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
ArrowReaderMetadata::with_supplied_schemaalready treats virtual columns as extra fields on top of the hint. It passes them toparquet_to_arrow_field_levels_with_virtual, and its own length check readssupplied_schema.fields().len() + virtual_columns.len(). Then it returnsschema: supplied_schema, which doesn't have them.So the two branches of
try_newdisagree. Without a hint you get a schema with the virtual fields in it; add a hint and they vanish, even though the reader still decodes them. On the file from the issue that meansmetadata.schema()reports one field while every batch that metadata produces has two:@limenilbuz asked for either of two behaviours: include the virtual columns in the reported schema, or stop erroring when the hint itself contains them. This does the first. The second is a change to what
with_schemaaccepts, and the length check here already assumes virtual fields live outside the hint, so the first is the one that makes the function agree with itself.What changes are included in this PR?
The returned schema is now the supplied fields followed by the virtual fields, keeping the supplied schema's key/value metadata. When no virtual columns are requested the supplied schema is returned untouched, so nothing changes for that path.
parquet_to_arrow_field_levels_with_virtualappends virtual columns to the root in the order given and clones them unchanged (parquet/src/arrow/schema/mod.rs:223), so appending them here in the same order lines the reported schema up withfield_levels.Are these changes tested?
Yes.
test_supplied_schema_keeps_virtual_columnsbuilds metadata from a hint plus two virtual fields and checks the field order, that the hint's schema metadata survives, and thatmetadata.schema()agrees with the fields of the batch the reader emits. It fails onf9e02bawith:cargo test -p parquet --libis 1381 passed, 0 failed.cargo fmt --all -- --checkandcargo clippy -p parquet --all-targetsare both clean.Are there any user-facing changes?
ArrowReaderMetadata::schema(), and the builder schema derived from it, gain the virtual fields when a hint and virtual columns are combined. That's the fix, but it is a field-count change on a public accessor, so it's worth calling out. Physical column indices are unaffected and no crate in the tree combines those two options.One thing I left alone: with an explicit projection the async reader's
schema()drops virtual fields while the batches still carry them. That reproduces with and without a schema hint, so it's a separate bug from this one and I didn't touch it here.