[SPARK-59108][4.0][SQL] Fix Avro positional matching under column pruning - #58540
Open
LuciferYang wants to merge 1 commit into
Open
[SPARK-59108][4.0][SQL] Fix Avro positional matching under column pruning#58540LuciferYang wants to merge 1 commit into
LuciferYang wants to merge 1 commit into
Conversation
…ning Backport of c809c28 (apache#58409) / e03066e (apache#58513) to branch-4.0.
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.
What changes were proposed in this pull request?
Backport of
c809c283c2d(#58409) tobranch-4.0. The Avro fix comes over as it landed; the two gate removals that PR also carried are not here, because neither gate exists on this branch.AvroDeserializernow takes the schema its Catalyst schema was projected from, and underpositionalFieldMatchingit resolves a Catalyst field against that field's position in the data schema rather than its position in the projection.AvroUtils.AvroSchemaHelpertakes the resulting positions; with none it keeps using a field's own position, which is what every caller whose Catalyst schema is not a projection needs (from_avro, the write path, the state-store encoder).Two read call sites pass the data schema on this branch:
AvroPartitionReaderFactoryon the V2 path andAvroFileFormat.buildReaderon V1. Master has a third,AvroFileFormat.readArchive, which does not exist here. A nested record keeps resolving by its own positions, since neither read path prunes nested fields:FileScanBuilder.supportsNestedSchemaPruningis false andAvroScanBuilderdoes not override it, andSchemaPruning.canPruneDataSchemacovers only Parquet and ORC.ORC already does this for
orc.force.positional.evolution:OrcUtils.requestedColumnIdsmaps the required schema throughdataSchema.fieldIndex(name), which makes its positional path projection-independent. Avro decodes the whole record whatever the projection asks for, so nothing extra is read.Master retired two gates that kept avro out of scan merging, and neither is on this branch: SPARK-57205 (#58340) withheld the
SCAN_MERGINGcapability fromAvroTable, and SPARK-59107 (#58411) named avro inDataSourceUtils.isProjectionSensitiveRead. So there is no predicate to change, noAvroTable.supportsScanMergingto turn on, no test to delete, anddocs/sql-performance-tuning.mdhas no "Merging Subplans" section stating the old behaviour. That also means subplan merging can widen an avro projection here with nothing in front of it, which makes the position mapping worth more on this branch than on 4.3, not less.One shape stays broken, with or without this change:
recursiveFieldMaxDepthmakesSchemaConvertersdrop a field it will not recurse into, so the data schema is a gapped view of the Avro schema and positional matching misaligns from the gap onwards. The code records that where the positions are computed.Why are the changes needed?
With
positionalFieldMatching=truethe deserializer is built from the projected read schema while the Avro side stays the full Avro schema, andAvroUtils.AvroSchemaHelper.getAvroFieldpairs Catalyst field i with Avro field i, so a column-pruned read takes the wrong Avro field and returns wrong values with no error. Measured on a file whose fieldsa,b,choldid,100 * id,10000 * idfor ids 0 to 4, read with the option on:Only a projection that is a prefix of the file's field list comes back right, so a column's value depends on which other columns the query selects. Both read paths behave the same way. Whether the failure is silent depends on the types of the mispaired fields: matching types return wrong values, as above, and incompatible ones fail the read with a schema-incompatibility error instead. A pushed filter is evaluated inside the deserializer, so the wrong pairing can also drop rows rather than only return wrong values for them.
A pruned projection is all it takes, so this does not depend on scan merging. Merging only makes it easier to reach without asking for it, and on this branch nothing keeps an avro relation out of it.
Does this PR introduce any user-facing change?
Yes, a bug fix on the Avro read path, both V1 and V2, and every 4.0.x release shipped the bug: positional matching has resolved against the projection since 3.2.0 (SPARK-34365). A read that sets
positionalFieldMatchingand prunes columns now returns the values of the columns it asked for. A query whose projection is a prefix of the Avro field list is unaffected, which is why the option's existing tests need no change. A read that used to land on a type-compatible neighbouring field now pairs with its own field and fails when the two types do not match, so a query that returned values before this change can return an error instead. That is the point of the fix rather than a side effect, but it is the shape most likely to be reported as a regression. The "Cannot find field at position N" message that positional matching raises now names the position it looked for rather than the position within the projection, which are the same number for an unprojected read. Nothing changes when the option is off, which is the default, and nothing changes on the write path or infrom_avro.How was this patch tested?
Five new tests in
AvroSuite, so each runs on both read paths (AvroV1SuiteandAvroV2Suiteextend it): the renamed-schema shape from the description, with each one-column and two-column projection whose values the fix changes, the ones it leaves alone being the prefixes of the field list, a pushed filter under both settings ofspark.sql.avro.filterPushdown.enabled,count(1), and mixed-case names under both case-sensitivity settings; a partition column sitting between two data columns in the schema; a nested record, which must keep resolving by its own positions, together with theavroSchemaoption supplying the Avro side; a projection that reaches past the end of the Avro schema, which reads null; and a mispaired type, which fails the read rather than returning a neighbouring field's values. One test inAvroSchemaHelperSuitefor the helper itself. Master'sAvroArchiveReadBasecase is not here, since this branch has no archive reader.One more test, in
AvroV1Suite, for a merged read. The file has three columns and the two scalar aggregates read the last two, so the merged projection is a proper subset of the data schema and the read has to resolve against that schema to answer[100, 1000]; the scan is one widenedFileSourceScanExecreading both columns. Unlike master's version it pins only AQE, since the strictness flags matter to a predicate that does not exist on this branch, and it has noAvroV2Suitetwin, since avro never declaresSCAN_MERGINGhere.Mutation check, measured on this branch: with the position mapping disabled, 11 cases fail, the five
SPARK-59108shapes on each read path and the new merge test, which answers[10, 100]where the file has[100, 1000].Regression, measured on this branch: the whole
avromodule, 375 tests, andavro/scalastyle,avro/Test/scalastyle,sql/scalastyleandcatalyst/scalastyle.RocksDBStateEncoderSuiteandStateStoreSuite, which also build anAvroDeserializer, were run on master rather than here; nothing in this backport differs from the master commit in that path. The existingpositionalFieldMatchingtests (SPARK-34365) needed no change, because their projections cover the whole schema.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code