Uh oh!
There was an error while loading. Please reload this page.
[SPARK-59107][SQL] Do not widen a projection-sensitive V1 file read - #58411
[SPARK-59107][SQL] Do not widen a projection-sensitive V1 file read#58411LuciferYang wants to merge 15 commits into
Conversation
… file read asks for Subplan merging reuses one LogicalRelation for two subqueries that project different columns, and V1 column pruning happens in physical planning, so the shared scan reads the union of the two column sets. For CSV, JSON and XML that changes which records the parser treats as malformed, and under ignoreCorruptFiles or ignoreMissingFiles a failure in a column only the sibling projects is swallowed together with the rest of that file's rows. Decline the merge in those cases.
A self join reads the same relation twice with a different column set each time, and keeping one set per relation compared the wrong pair, so two subqueries could merge and widen one leg's read. Also leave partition columns out of the comparison, since their values come from the path.
The suite asserted a scan count, which is an indirect proxy: it depends on which scans physical reuse hid behind a leaf node, and two of its comments described that wrongly. It now asserts the columns each scan reads, which is the property the change is about, and covers two shapes it had missed, an identical pair of subqueries and a partition column reference. hasStrictFileReads moves to FileSourceOptions, where both FileScanRDD and the new predicate can use it instead of spelling it out twice.
…ds in InMemoryRelation
…t the merged plan Merging rebuilds projections from a side's whole output, which for a V1 relation is its full schema, and the ColumnPruning that narrows it again runs after this rule. Re-deriving the cached side's read set from the merged plan therefore read the full schema, so a third subquery that reads every column compared equal, joined the pair, and widened their read: measured [14, 8, 88] where three separate scans give [18, 10, 88]. MergedPlan now carries the set recorded when the entry was cached, which every plan merged into it had to match.
…all-through A review doubted that anything prunes columns after this rule, reading only the catalyst batches; SparkOptimizer's Extract Python UDFs batch reruns ColumnPruning after the MergeSubplans batch, so the comments now say which one. The new test covers a refusal falling through to a later cache entry, and the file-axis note says why an OR of a partition and a data predicate prunes nothing.
sunchao
left a comment
There was a problem hiding this comment.
Reviewed 00b48e2 against 024120d with five independent review passes covering merge/cache invariants, read-column tracking and self-joins, parser/file-option semantics, optimizer/AQE/reuse interactions, and tests/compatibility. No actionable P1/P2 findings.
The per-occurrence read sets and preservation of the original cached signature cover the important self-join and multi-subquery cases, while retaining identical-plan reuse and compatible scan sharing.
Verified CI on this exact commit: 29 checks passed and 2 were skipped, with no failing or pending checks. The test report records 72,229 tests run, 1,100 skipped, and 0 failures. Kubernetes integration was skipped. I did not independently execute the tests locally; this approval is based on source review and the verified CI results.
LGTM.
peter-toth
left a comment
There was a problem hiding this comment.
Thanks for the PR, @LuciferYang!
V1 top-level column pruning happens in physical planning, so two LogicalRelations over the same files canonicalize equal whatever each side projects, and PlanMerger is the only place that can notice. Recording the read column sets per occurrence, and keeping the record taken when the entry was first cached rather than re-deriving it from a merged plan, is what makes the self join and the third-subquery cases come out right. The suite passes 17/17 on 00b48e2 here.
One thing to fix before merge: docs/sql-performance-tuning.md spells out when two subplans merge, and it gains a condition here that it says nothing about. Below that, AvroFileFormat is projection-sensitive under positionalFieldMatching and is not on the list, so that shape still merges and still changes the answer.
Blocking
- 2.The merge conditions in the tuning doc are now incomplete:
docs/sql-performance-tuning.md:343enumerates when two subplans merge and does not learn the new condition. This is a default-on optimization that now silently does not fire, and #58340 updates the same file for its half. [inline:sql/core/src/main/scala/org/apache/spark/sql/execution/planmerging/PlanMerger.scala:192]
Non-blocking
- 1.V1 avro still widens a projection-sensitive read:
AvroFileFormatbuilds its deserializer from the prunedrequiredSchema, so underpositionalFieldMatchinga column's values depend on the projection, measured[10, 100]merged against[10, 10]unmerged. Not a regression, since positional matching against a pruned schema is already wrong, so this is a silent result change rather than correct-to-wrong. [inline:sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceUtils.scala:193] - 3.Mutation-check counts in the description: with
isProjectionSensitiveReadforced to false the suite reports 11 failed and 6 succeeded, not "10 of the 17" and "the 7 that pass". "Eight shapes that must keep merging" does not line up either, since 6 tests assert a merge still happens and the sentence names 5 of them.
Alternatives
- 4.Name the formats by short name rather than by class:
supportNestedPredicatePushdown, twenty lines above, answers the same kind of question withhs.toStringagainst aSQLConflist. That would cover avro and any third-party format without the three imports. [inline:sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceUtils.scala:192]
Minor
- 5.Dead first clause: two empty maps are already
==, so thenonEmptydisjunction never changes the result. [inline:sql/core/src/main/scala/org/apache/spark/sql/execution/planmerging/PlanMerger.scala:287] - 6.
referencedrebuilt per occurrence: it is the same set every time, so a plan with N sensitive relations walks itself N times. [inline:sql/core/src/main/scala/org/apache/spark/sql/execution/planmerging/PlanMerger.scala:298] - 7.
optionsis now a field onFileScanRDD: reading it from a method body retains the constructor parameter, so it goes into every task closure. [inline:sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileScanRDD.scala:94] - 8.Three missing blank lines in the new suite. [inline:
sql/core/src/test/scala/org/apache/spark/sql/execution/planmerging/FileSourceV1PlanMergingSuite.scala:89] - 9.This scaladoc claims the swallowing mechanism for both strictness flags: only
ignoreCorruptFileshas it, and #58340'sFileTable.hasStrictFileReadssays so. [inline:sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceUtils.scala:177]
| } | ||
| private def hasProjectionSensitiveParser(fileFormat: FileFormat): Boolean = fileFormat match { | ||
| case _: CSVFileFormat | _: JsonFileFormat | _: XmlFileFormat => true |
There was a problem hiding this comment.
Finding 1.AvroFileFormat.buildReader hands the pruned requiredSchema to AvroDeserializer, and under positionalFieldMatching catalyst field i takes Avro field i of the full file schema: AvroUtils.AvroSchemaHelper.getAvroField returns avroFieldArray.lift(catalystPos) (sql/core/src/main/scala/org/apache/spark/sql/avro/AvroUtils.scala:462). Widening the read therefore changes the values the columns already being read come back with.
Measured on 00b48e24, on the default configuration (avro is in the spark.sql.sources.useV1SourceList default, so this is the V1 path):
spark.range(0, 5).selectExpr("id AS a", "id * 10 AS b").write.format("avro").save(path)
spark.read.option("positionalFieldMatching", "true").format("avro").load(path)
.createOrReplaceTempView("t")
sql("SELECT (SELECT sum(a) FROM t), (SELECT sum(b) FROM t)")
// merged [10, 100] one scan reading {a, b}// MergeSubplans excluded [10, 10] two scans, {a} and {b}Non-blocking because merging cannot turn a correct answer into a wrong one here. Pruning keeps the fields' relative order, so a pruned schema is a subsequence of the full one, and in the union of two subsequences every retained field's index moves toward its true index without passing it. Both [10, 10] and [10, 100] are wrong against the file, which is SPARK-59108. What the merge does change is that one subquery's values depend on what its sibling projects, and that is the invariant this PR is about.
#58340 withholds SCAN_MERGING from AvroTable under this option, so the two read paths still disagree on it. AvroFileFormat lives in sql/core under org.apache.spark.sql.avro, so the module argument in the description does not cover it and this object can name it directly:
private[sql] defisProjectionSensitiveRead(relation: BaseRelation):Boolean= relation match {
casehs: HadoopFsRelation=>!newFileSourceOptions(hs.options).hasStrictFileReads ||
hasProjectionSensitiveParser(hs.fileFormat, hs.options)
case _ =>false
}
privatedefhasProjectionSensitiveParser(
fileFormat: FileFormat, options: Map[String, String]):Boolean= fileFormat match {
case_: CSVFileFormat|_: JsonFileFormat|_: XmlFileFormat=>true// Positional matching resolves each catalyst field against the Avro field at the same// position, so pruning shifts which Avro field a column reads. See SPARK-59108.case_: AvroFileFormat=>CaseInsensitiveMap(options).get(AvroOptions.POSITIONAL_FIELD_MATCHING).exists(_.toBoolean)
case _ =>false
}Naming AvroFileFormat unconditionally would do too, at the cost of every avro merge.
| origAttr -> mergedPlan.output.indexWhere(_.exprId == mergedAttr.exprId) | ||
| }.toSeq) | ||
| MergeResult(newMergedPlan, i, outputMap) | ||
| if (widensProjectionSensitiveRead(projectionSensitiveReads, mp)) { |
There was a problem hiding this comment.
Finding 2.docs/sql-performance-tuning.md:343 lists what two subplans have to agree on to merge, ending with "and the leaves must read the same input". This line adds a condition that is not there. It is a default-on optimization that now silently does not fire for a whole class of relations, so the tuning page is where someone will look, and #58340 updates the same file for its half of this.
Something like this, appended to that paragraph:
A V1 file relation whose rows depend on the columns the read asked for is merged only when both subplans read the same columns of it. That covers
csv,jsonandxml, whose parsers decide what counts as a malformed record from the required schema, and any file relation read withspark.sql.files.ignoreCorruptFilesorspark.sql.files.ignoreMissingFilesset, where a failure in a column only one side reads is swallowed together with the rest of that file's rows.
| case _ => false | ||
| } | ||
| private def hasProjectionSensitiveParser(fileFormat: FileFormat): Boolean = fileFormat match { |
There was a problem hiding this comment.
Finding 4.supportNestedPredicatePushdown at line 158 of this file answers the same kind of question by short name. hs.toString is the format's registered short name, so a list covers every format the same way, including the ones this object cannot import:
privatedefhasProjectionSensitiveParser(hs: HadoopFsRelation):Boolean= {
valsensitive=Utils.stringToSeq(
SQLConf.get.getConf(SQLConf.PROJECTION_SENSITIVE_FILE_SOURCE_LIST).toLowerCase(Locale.ROOT))
sensitive.contains(hs.toString) // "csv", "json", "xml", "avro"
}That drops the three imports, covers avro (finding 1), and lets someone add a third-party format they know to be sensitive.
The counter-argument is real. NESTED_PREDICATE_PUSHDOWN_FILE_SOURCE_LIST gates an optimization, so shortening it costs performance, while shortening this one returns wrong rows. A hard-coded Set("csv", "json", "xml", "avro") of short names keeps the module independence without that, and is what I would take if you do not want a new conf.
| private def widensProjectionSensitiveRead( | ||
| reads: Map[LogicalPlan, Seq[Set[String]]], | ||
| cachedPlan: MergedPlan): Boolean = { | ||
| (reads.nonEmpty || cachedPlan.projectionSensitiveReads.nonEmpty) && |
There was a problem hiding this comment.
Finding 5. Two empty maps are already ==, so this clause never changes the result. When both are empty the comparison is false anyway, and when exactly one is empty the comparison is true and so is the disjunction.
privatedefwidensProjectionSensitiveRead(
reads: Map[LogicalPlan, Seq[Set[String]]],
cachedPlan: MergedPlan):Boolean= reads != cachedPlan.projectionSensitiveReads| * than from the file, so referencing one does not widen what the reader parses. | ||
| */ | ||
| private def readColumnNames(plan: LogicalPlan, relation: LogicalRelation): Set[String] = { | ||
| val referenced = AttributeSet(plan.flatMap(_.references)) ++ AttributeSet(plan.output) |
There was a problem hiding this comment.
Finding 6.referenced does not depend on relation, but it is rebuilt for each occurrence, so a plan with N projection-sensitive relations walks itself N times and builds N AttributeSets. Computing it once at the only caller makes the independence visible too:
privatedefcollectProjectionSensitiveReads(
plan: LogicalPlan):Map[LogicalPlan, Seq[Set[String]]] = {
lazyvalreferenced=AttributeSet(plan.flatMap(_.references)) ++AttributeSet(plan.output)
plan.collect {
casel: LogicalRelationifDataSourceUtils.isProjectionSensitiveRead(l.relation) =>
l.canonicalized -> readColumnNames(referenced, l)
}.groupMap(_._1)(_._2)
}| /** Whether this reader fails instead of silently skipping missing or corrupt input files. */ | ||
| private[sql] def hasStrictFileReads: Boolean = !ignoreCorruptFiles && !ignoreMissingFiles | ||
| private[sql] def hasStrictFileReads: Boolean = options.hasStrictFileReads |
There was a problem hiding this comment.
Finding 7. Reading options from a method body turns the constructor parameter into a field, so every FileScanRDD now carries a FileSourceOptions into the task closure. javap -p on the two builds:
base private final boolean ...FileScanRDD$$ignoreCorruptFiles;
private final boolean ...FileScanRDD$$ignoreMissingFiles;
head private final org.apache.spark.sql.catalyst.FileSourceOptions options;
(plus both booleans)
Tiny in bytes, since parameters is @transient. Still, the two vals right above already hold what this needs, so the dedup can keep the parameter constructor-scoped:
| private[sql] defhasStrictFileReads:Boolean= options.hasStrictFileReads | |
| private[sql] valhasStrictFileReads:Boolean= options.hasStrictFileReads |
| .collectWithSubqueries { case s: FileSourceScanExec => s } | ||
| .map(_.requiredSchema.fieldNames.sorted.toSeq) | ||
| .sortBy(_.mkString(",")) | ||
| // One test per format rather than `gridTest`, so that the format reads in the middle of the name. |
There was a problem hiding this comment.
Finding 8. Missing blank line after scanColumns. Same at the two other places where a Seq/test follows a closing brace: FileSourceV1PlanMergingSuite.scala:140 and :199.
| * from the columns it was asked for, which lets a wider read drop or rewrite rows that the | ||
| * narrower one returned: CSV, JSON and XML all build their parser from the required schema and | ||
| * take `mode` and the corrupt-record column from it. Or the read is not strict, in which case a | ||
| * failure in a column that only the wider read touches is swallowed together with the rest of |
There was a problem hiding this comment.
Finding 9. This attributes the swallowing to both halves of hasStrictFileReads, and only ignoreCorruptFiles has it. A missing file is skipped whatever is projected, so it cannot make a read projection-sensitive. Your own FileTable.hasStrictFileReads in #58340 says exactly that:
ignoreMissingFilesdrops the same rows whatever is projected, and is included to matchFileScanRDD.hasStrictFileReads, the same predicate on the physical side.
Worth carrying that sentence over here, because this PR is what makes hasStrictFileReads shared: FileSourceOptions now answers it for the cache-repeatability question in InMemoryRelation, for the reader in FileScanRDD, and for this one, and only here is one of the two flags carried for consistency rather than for a mechanism. No behaviour change asked for.
… condition Avro is projection-sensitive under positionalFieldMatching, measured [10, 100] merged against [10, 10] unmerged, and AvroFileFormat is private[sql] in sql/core, so this object can name it; the test lives in AvroV1Suite because avro does not resolve from the sql/core test classpath. The tuning doc enumerates what two subplans must agree on to merge, so it gains this condition. Also: drop a disjunction that never changed the result, compute the referenced set once rather than per relation, keep FileScanRDD's options constructor-scoped, three blank lines, and say that only ignoreCorruptFiles has the swallowing mechanism.
The tuning doc gave ignoreMissingFiles a mechanism it does not have and sat between a paragraph and its elaboration, so the condition moves into the sentence that enumerates the conditions. The avro test comment called the file-correct value wrong. The suite scaladoc said two tests cannot assert rows when what they cannot do is discriminate on them.
LuciferYang
commented
Aug 30, 2026
Thanks @peter-toth. Finding 2 is fixed and finding 1 took a gate with it. 1. Confirmed, and the module argument in my description was wrong: 2. Added to the paragraph that enumerates the conditions, naming the three parsers, avro under the option, and both strictness flags. 3. Both numbers were arithmetic rather than measured: I added the seventeenth test and adjusted the counts instead of re-running. Re-measured, 11 of the 17 fail and 6 pass, and the sentence about shapes that keep merging now names the six it means. A pass over the rest of the description turned up more of the same class, all corrected: the table legend collided with its own header, the FAILFAST shape is CSV-specific rather than one of three, the mutation on the cached record fails two tests rather than one ( 4. I kept the class match. 5. Gone. 6. Computed once at the caller and passed in. 7. Locally: the |
There was a problem hiding this comment.
Re-checked through 0942427984 - findings 1, 2, 3, 5, 6, 7, 8 and 9 resolved, nothing regressed. I re-ran the suite at this head: 17/17 plus the avro test, and with isProjectionSensitiveRead forced to false 11 of the 17 fail and the avro test with them, so the description's counts hold. javap -p on this build shows no options field on FileScanRDD, only the boolean, so finding 7 landed as you describe.
Finding 4 I withdraw. HadoopFsRelation.toString:61 really does fall back to HadoopFiles for a format with no DataSourceRegister, so a short name is not an identity and a list keyed on it can only be set too coarsely. On your offer of a configuration: I would not add one. It inherits the same broken key, and a wrong value there returns wrong rows rather than losing an optimization. If a third-party format ever needs this, a FileFormat method defaulting to false is the extension point, and it preserves today's behaviour so it can go in on demand.
Nothing blocking from my side. Both below are small, and 10 is a sequencing decision rather than a defect in this diff.
Non-blocking
- 10.The avro case and its test collide with #58409 (new): Measured at #58409's head, that query returns
[10, 100]merged and unmerged, soRow(10L, 10L)fails there and the avro case becomes dead. Say in the description which lands first and what the other one deletes.connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala:3747
Minor
- 11.
MergedPlan's record has an unused default (new): Both construction sites pass it, and= Map.emptyis the value that lets a merge through.sql/core/src/main/scala/org/apache/spark/sql/execution/planmerging/PlanMerger.scala:68
Thanks for working through all nine of the round-1 findings, @LuciferYang - nothing left open from that round.
| // shared with sum(a) reads both fields, pairs them by coincidence and answers 100. The 10 | ||
| // is wrong against the file, which is SPARK-59108; what merging must not do is make one | ||
| // subquery's value depend on what the other one projects. | ||
| checkAnswer(sql("SELECT (SELECT sum(a) FROM t), (SELECT sum(b) FROM t)"), Row(10L, 10L)) |
There was a problem hiding this comment.
Finding 10. Your description says #58409 "removes that at the root, and this case can go with it". It takes this test with it too, and not by making it redundant - by failing it.
I ran this exact query in a worktree at #58409's head a3358708e60:
merged = [10, 100]
unmerged = [10, 100]
sum(b) on its own is 100 there, because the fix resolves b against its position in the data schema rather than in the projection. So Row(10L, 10L) fails, and the avro arm of hasProjectionSensitiveParser is dead at the same moment - the read stops being projection-sensitive, which is exactly what those two numbers being equal shows.
Whichever of the two lands second pays for it, so it is worth deciding now and saying so in the description. Two ways out:
- Land [SPARK-59108][SQL] Fix Avro positional matching under column pruning #58409 first and drop the avro arm plus this test from here. That also drops the only reason
DataSourceUtilsimportsorg.apache.spark.sql.avro. - Keep them, land this first, and have [SPARK-59108][SQL] Fix Avro positional matching under column pruning #58409 delete both.
If you would rather keep the test through the transition, the assertion that survives is the one the sql/core suite already uses - the columns each scan reads, which stay Seq(Seq("a"), Seq("b")) under the gate whatever sum(b) evaluates to. AvroV1Suite would need spark.sql.adaptive.enabled off for that, for the reason FileSourceV1PlanMergingSuite:369 gives.
| case class MergedPlan( | ||
| plan: LogicalPlan, | ||
| merged: Boolean, | ||
| projectionSensitiveReads: Map[LogicalPlan, Seq[Set[String]]] = Map.empty) |
There was a problem hiding this comment.
Finding 11. Nothing uses the default. MergedPlan(plan, false, projectionSensitiveReads) at line 210 is the only construction, and the two other sites are mp.copy(...), which carries the record over.
The value it defaults to is also the permissive one: an entry built without a record compares equal to any plan that reads no projection-sensitive relation, so the merge goes through. Dropping the default makes a future construction site say what it means.
caseclassMergedPlan(
plan: LogicalPlan,
merged: Boolean,
projectionSensitiveReads: Map[LogicalPlan, Seq[Set[String]]])LuciferYang
commented
Aug 30, 2026
Thanks for re-running it, @peter-toth. Both new items are in at 10. I kept the test and kept a value assertion with it, by taking the expected row from the same query with The order is now in all three descriptions: this one, then #58340, then #58409, which deletes the avro arm, the 11. Dropped. 210 is the only construction site and the other two are 4. Agreed, no configuration. If a third-party format ever needs one, the |
uros-b
left a comment
There was a problem hiding this comment.
+1, thank you @LuciferYang and @sunchao@peter-toth!
### What changes were proposed in this pull request? `FileTable` gains a `supportsScanMerging` seam, and `ParquetTable`, `OrcTable`, `TextTable` and `AvroTable` override it. Those four then take part in the DSv2 scan merging added by #57360 (SPARK-40259): `PlanMerger` drives `V2ScanRelationPushDown.rebuildScan` to rebuild a merged scan, and a source supplies no merge logic of its own. It only declares that widening the set of columns pruned on its builder, with the scan options and pushed filters held constant, changes neither which rows the scan returns nor the values it returns for the columns it was already asked for; it may at most surface a read error. `TableCapability.SCAN_MERGING`'s javadoc stated only a determinism contract, which a CSV table satisfies as written, since its rows are fully determined by the pruned column set and that is exactly the dependence, so this monotonicity criterion is now stated there too, where a connector author will read it. `CSVTable` and `JsonTable` do not override it. Their parsers are handed the columns the scan asked for and decide from that set what counts as a malformed record, so a merged scan reading the union of two column sets can drop or rewrite rows the narrower scan returned. Measured, csv and json alike: with `mode=DROPMALFORMED` and a record malformed only in the other subquery's column, `sum(a)` is 8 where two separate scans give 10; with `PERMISSIVE`, the default, and `_corrupt_record` in the schema, the column is populated for a row the narrow scan counted as clean; with `FAILFAST` and a CSV row carrying fewer tokens than the schema has columns, the merged scan throws where the unmerged one returned rows. Those numbers come from the V1 path, which merges all three shapes today and is where SPARK-59107 (#58411) fixes them. The seam defaults to false, because a format that does not merge misses an optimization while a format that merges when its parser is projection-sensitive returns wrong rows. Two further gates keep the contract true of the formats that do declare it. `FileTable` withholds the capability when `spark.sql.files.ignoreCorruptFiles` or `spark.sql.files.ignoreMissingFiles` is set, matching `FileScanRDD.hasStrictFileReads` on the physical side. Under a non-strict read a failure in a column that only the sibling subquery projects is swallowed and the rest of that file's rows go with it, so the merged scan returns fewer rows than the narrow one did. Measured on parquet, V1 and V2 alike: `SELECT (SELECT sum(a) FROM t), (SELECT count(b) FROM t)` over data whose `b` is written as a string and read as a long returns `[45, 0]` with merging off and `[null, 0]` with it on. The gate is evaluated per call rather than cached, so a table built before the configuration was set still answers for the read that is running. `AvroTable` withholds it under `positionalFieldMatching`. `AvroPartitionReaderFactory` builds the deserializer from the pruned read schema while the Avro side stays the full Avro schema, so under that option catalyst field *i* of the projection takes Avro field *i* of that schema, and widening the projection changes the values a column comes back with. That is a bug in its own right, filed as SPARK-59108 (#58409), which lands after this one and deletes this gate with it. ORC needs no equivalent gate: `OrcUtils.requestedColumnIds` maps both the `_col*` case and `orc.force.positional.evolution` through `dataSchema.fieldIndex(name)` and disables pruning in that branch, so its positional path is projection-independent. The rest is a new test suite, a package-private `V2ScanMergingTestHelper` shared with `DSv2PlanMergingSuite`, one test in `AvroV2Suite`, and one documentation update. Landing order: #58411 (SPARK-59107) first, then this one, then #58409 (SPARK-59108), which deletes the `AvroTable` gate. Nothing here depends on the first step, since this PR no longer asserts anything about how V1 merges CSV and JSON. Once #58411 is in, `FileTable.hasStrictFileReads` should call the `FileSourceOptions.hasStrictFileReads` it adds rather than spell the predicate out a third time, beside `FileScanRDD` and the cache-repeatability check in `InMemoryRelation`. ### Why are the changes needed? Two scans of the same file table that differ only in their projected columns cannot be reused today. A file source folds its data filters into the `FileScan` object, where they are used to list files and prune row groups, and `FileScan.equals` compares them, so two subquery scans over the same path are not canonically identical and `PlanMerger`'s identical-plan fast path does not fire. On the V1 path those filters sit in a `Filter` above an identical `LogicalRelation`, so it does fire. Declaring the capability closes most of that gap for the formats where the merge is sound. On TPC-DS at scale factor 100, with `spark.sql.sources.useV1SourceList` cleared, two queries change: q9 goes from 16 distinct scans to 6 under the default configuration and to 2 with `dsv2SymmetricFilterPropagation` on, and q28 from 6 to 1 with that configuration on. Of the 99 v1.4 queries, 95 ran and the other 93 of those are unchanged. Wall clock for q9 went from 78.1s to 40.3s by default and to 21.6s with the configuration on, and q28 from 61.3s to 34.4s; measured on `local[1]` with AQE off, two runs per configuration, against a build with the overrides removed as the baseline. Two samples on one machine put the noise around 20%, so the scan counts are the reproducible part and the timings show the order of magnitude. Both queries read `store_sales`, which is parquet, so leaving CSV and JSON out does not affect these numbers. Four queries could not be measured here because `DataSourceV2Relation.computeStats` raises a testing-only assertion when stats are read before pushdown, and q30 does not run. ### Does this PR introduce _any_ user-facing change? Yes, on the V2 file source read path, which a format reaches only when it is removed from `spark.sql.sources.useV1SourceList`. Plan shape only: subqueries over the same Parquet, ORC, text or Avro table that differ only in their projected columns now collapse into a single scan reading the union of those columns. No query result changes, which is what the exclusions and the two gates above are for, and why this needs no migration-guide entry. The `dsv2SymmetricFilterPropagation` entry in `docs/sql-performance-tuning.md` said "no built-in source does", which this PR falsifies. The four formats and the two withholding rules now sit in the prose of the Merging Subplans section rather than in that entry, since they hold whatever the configuration is set to, and the prose sentence that said the leaves must read the same input is qualified there for a source that declares the capability. Four gaps are left in place. The first three are follow-ups #57360 already lists. 1. Two scans with different partition filters do not merge, while V1 merges them. A partition filter is fully enforced by the V2 scan and reported as strict, so widening it to `OR` would leave the merged scan returning rows nothing above it filters out. 2. Parquet and ORC nested columns do not merge while nested schema pruning is on. Each side narrows the struct to the field it reads, so the read column is no longer a same-type subset of the relation's column. V1 does not merge this shape either, because `SchemaPruning` rewrites each side's `dataSchema` and the two relations stop being canonically equal. 3. Differing data filters need `spark.sql.optimizer.mergeSubplans.filterPropagation.dsv2SymmetricFilterPropagation.enabled`, which defaults to false. On the V2 path that configuration alone is enough, where V1 needs the broader `symmetricFilterPropagation`. 4. CSV and JSON get no merging at all, which is deliberate rather than a follow-up. It costs them two shapes: scans whose projected columns differ, and scans reading the same columns under different filters, since `FileScan.equals` compares the normalized data filters so those are not canonically equal either and do not fall back to plain reuse. The V1 path merges both today and answers `[8, 80]` where two separate scans answer `[10, 80]`, so one subquery's result depends on what a sibling subquery projects. That is a V1 bug rather than a target to copy; SPARK-59107 (#58411) fixes it, and with that in both read paths decline these shapes alike. ### How was this patch tested? New suite `FileSourceV2PlanMergingSuite`, 16 tests. Every built-in file table is checked on the side it belongs to; a non-strict read is checked to withhold the capability on both strictness configurations, and separately to keep its scans separate and return `[45, 0]`, with the table and the temp view built outside the configuration scope so that a cached gate would fail the test. Those are two tests rather than one so that a mutation names which half it broke, the capability assertion having aborted before the rows could report. Scans differing only in projected columns merge for Parquet and ORC and decline for CSV and JSON in the same shape; text merges the one shape a single-column table can differ in, which also needs both aggregates to be hash-aggregatable or `PlanMerger.supportedAggregateMerge` declines above the scans; scans over the same partition filter merge with that filter still enforced on the rebuilt scan; three scans merge into one; differing data filters merge only with the dsv2 configuration on, and the merged scan is checked to carry the OR-widened predicate rather than only to exist. Declines are covered too: differing partition filters, nested-pruned columns, a pushed aggregate (with the aggregate itself asserted, not just the scan count), and two different tables holding different rows so that a cross-table merge would change the answer. Then V1/V2 parity on three shapes and the partition-filter gap pinned as the one shape where they disagree, both on parquet, whose reads this suite pins strict and whose parser does not depend on the projection, so SPARK-59107 leaves them merging on V1. The CSV and JSON parse behaviour is pinned on the V2 side alone, on three shapes: `DROPMALFORMED` gives `[10, 80]`, `PERMISSIVE` with `_corrupt_record` in the schema gives `[0, 80]`, and `FAILFAST` with a short CSV row returns rows rather than throwing. Subquery counts are asserted alongside the rows, so the result is attributed to the decline rather than inferred from the values, and the configurations the expectations depend on are pinned rather than assumed. These three used to assert the V1 numbers beside the V2 ones, and those arms pinned exactly what SPARK-59107 removes, so they are gone; its own suite covers that side. Measured at #58411's head, with this suite copied in: the CSV and JSON test passes there, and so does the parquet partition-filter one. Every test asserts which read path the plan took before asserting anything about merging. SQL-on-file and catalog tables resolve to the V1 `FileFormat` regardless of `spark.sql.sources.useV1SourceList`, so a suite driven by that configuration alone would quietly run V1 on both sides. One test in `AvroV2Suite`, because `AvroTable` lives in the module that has it on the classpath: it asserts the capability, that `positionalFieldMatching` withholds it, and that two scans differing only in their projected columns fuse into one reading the union. Mutation checks. Turning the three sql/core overrides to `false` fails 8 of the 16 tests; of the 8 that pass, 6 assert a decline, one asserts that V1 merges differing partition filters where V2 does not and V2 declines under the mutation too, and one asserts that a non-strict read keeps its scans separate, which the mutation also produces. Dropping `&& hasStrictFileReads` from `capabilities` fails both strictness tests, the capability one at its first assertion and the rows one with `[null, 0]` against `[45, 0]`, which is what splitting them was for. Regression: the `planmerging` suites, `ExplainSuite` and `ExplainSuiteAE`, `FileBasedDataSourceSuite`, `FileTableSuite`, `OrcV2SchemaPruningSuite`, `ParquetV2SchemaPruningSuite`, `ParquetV2FilterSuite`, `SubquerySuite`, `SameResultSuite`, `ParquetV2AggregatePushDownSuite`, `OrcV2AggregatePushDownSuite`, `DataSourceV2Suite` and `AvroV2Suite`. No golden file or `PlanStabilitySuite` plan needed regenerating. Those build their tables with `CREATE TABLE ... USING <format>`, which resolves to the V1 `FileFormat`, so none of them reaches a V2 file scan. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code Closes#58340 from LuciferYang/SPARK-57205. Authored-by: YangJie <yangjie01@baidu.com> Signed-off-by: yangjie01 <yangjie01@baidu.com>
### What changes were proposed in this pull request? `FileTable` gains a `supportsScanMerging` seam, and `ParquetTable`, `OrcTable`, `TextTable` and `AvroTable` override it. Those four then take part in the DSv2 scan merging added by #57360 (SPARK-40259): `PlanMerger` drives `V2ScanRelationPushDown.rebuildScan` to rebuild a merged scan, and a source supplies no merge logic of its own. It only declares that widening the set of columns pruned on its builder, with the scan options and pushed filters held constant, changes neither which rows the scan returns nor the values it returns for the columns it was already asked for; it may at most surface a read error. `TableCapability.SCAN_MERGING`'s javadoc stated only a determinism contract, which a CSV table satisfies as written, since its rows are fully determined by the pruned column set and that is exactly the dependence, so this monotonicity criterion is now stated there too, where a connector author will read it. `CSVTable` and `JsonTable` do not override it. Their parsers are handed the columns the scan asked for and decide from that set what counts as a malformed record, so a merged scan reading the union of two column sets can drop or rewrite rows the narrower scan returned. Measured, csv and json alike: with `mode=DROPMALFORMED` and a record malformed only in the other subquery's column, `sum(a)` is 8 where two separate scans give 10; with `PERMISSIVE`, the default, and `_corrupt_record` in the schema, the column is populated for a row the narrow scan counted as clean; with `FAILFAST` and a CSV row carrying fewer tokens than the schema has columns, the merged scan throws where the unmerged one returned rows. Those numbers come from the V1 path, which merges all three shapes today and is where SPARK-59107 (#58411) fixes them. The seam defaults to false, because a format that does not merge misses an optimization while a format that merges when its parser is projection-sensitive returns wrong rows. Two further gates keep the contract true of the formats that do declare it. `FileTable` withholds the capability when `spark.sql.files.ignoreCorruptFiles` or `spark.sql.files.ignoreMissingFiles` is set, matching `FileScanRDD.hasStrictFileReads` on the physical side. Under a non-strict read a failure in a column that only the sibling subquery projects is swallowed and the rest of that file's rows go with it, so the merged scan returns fewer rows than the narrow one did. Measured on parquet, V1 and V2 alike: `SELECT (SELECT sum(a) FROM t), (SELECT count(b) FROM t)` over data whose `b` is written as a string and read as a long returns `[45, 0]` with merging off and `[null, 0]` with it on. The gate is evaluated per call rather than cached, so a table built before the configuration was set still answers for the read that is running. `AvroTable` withholds it under `positionalFieldMatching`. `AvroPartitionReaderFactory` builds the deserializer from the pruned read schema while the Avro side stays the full Avro schema, so under that option catalyst field *i* of the projection takes Avro field *i* of that schema, and widening the projection changes the values a column comes back with. That is a bug in its own right, filed as SPARK-59108 (#58409), which lands after this one and deletes this gate with it. ORC needs no equivalent gate: `OrcUtils.requestedColumnIds` maps both the `_col*` case and `orc.force.positional.evolution` through `dataSchema.fieldIndex(name)` and disables pruning in that branch, so its positional path is projection-independent. The rest is a new test suite, a package-private `V2ScanMergingTestHelper` shared with `DSv2PlanMergingSuite`, one test in `AvroV2Suite`, and one documentation update. Landing order: #58411 (SPARK-59107) first, then this one, then #58409 (SPARK-59108), which deletes the `AvroTable` gate. Nothing here depends on the first step, since this PR no longer asserts anything about how V1 merges CSV and JSON. Once #58411 is in, `FileTable.hasStrictFileReads` should call the `FileSourceOptions.hasStrictFileReads` it adds rather than spell the predicate out a third time, beside `FileScanRDD` and the cache-repeatability check in `InMemoryRelation`. ### Why are the changes needed? Two scans of the same file table that differ only in their projected columns cannot be reused today. A file source folds its data filters into the `FileScan` object, where they are used to list files and prune row groups, and `FileScan.equals` compares them, so two subquery scans over the same path are not canonically identical and `PlanMerger`'s identical-plan fast path does not fire. On the V1 path those filters sit in a `Filter` above an identical `LogicalRelation`, so it does fire. Declaring the capability closes most of that gap for the formats where the merge is sound. On TPC-DS at scale factor 100, with `spark.sql.sources.useV1SourceList` cleared, two queries change: q9 goes from 16 distinct scans to 6 under the default configuration and to 2 with `dsv2SymmetricFilterPropagation` on, and q28 from 6 to 1 with that configuration on. Of the 99 v1.4 queries, 95 ran and the other 93 of those are unchanged. Wall clock for q9 went from 78.1s to 40.3s by default and to 21.6s with the configuration on, and q28 from 61.3s to 34.4s; measured on `local[1]` with AQE off, two runs per configuration, against a build with the overrides removed as the baseline. Two samples on one machine put the noise around 20%, so the scan counts are the reproducible part and the timings show the order of magnitude. Both queries read `store_sales`, which is parquet, so leaving CSV and JSON out does not affect these numbers. Four queries could not be measured here because `DataSourceV2Relation.computeStats` raises a testing-only assertion when stats are read before pushdown, and q30 does not run. ### Does this PR introduce _any_ user-facing change? Yes, on the V2 file source read path, which a format reaches only when it is removed from `spark.sql.sources.useV1SourceList`. Plan shape only: subqueries over the same Parquet, ORC, text or Avro table that differ only in their projected columns now collapse into a single scan reading the union of those columns. No query result changes, which is what the exclusions and the two gates above are for, and why this needs no migration-guide entry. The `dsv2SymmetricFilterPropagation` entry in `docs/sql-performance-tuning.md` said "no built-in source does", which this PR falsifies. The four formats and the two withholding rules now sit in the prose of the Merging Subplans section rather than in that entry, since they hold whatever the configuration is set to, and the prose sentence that said the leaves must read the same input is qualified there for a source that declares the capability. Four gaps are left in place. The first three are follow-ups #57360 already lists. 1. Two scans with different partition filters do not merge, while V1 merges them. A partition filter is fully enforced by the V2 scan and reported as strict, so widening it to `OR` would leave the merged scan returning rows nothing above it filters out. 2. Parquet and ORC nested columns do not merge while nested schema pruning is on. Each side narrows the struct to the field it reads, so the read column is no longer a same-type subset of the relation's column. V1 does not merge this shape either, because `SchemaPruning` rewrites each side's `dataSchema` and the two relations stop being canonically equal. 3. Differing data filters need `spark.sql.optimizer.mergeSubplans.filterPropagation.dsv2SymmetricFilterPropagation.enabled`, which defaults to false. On the V2 path that configuration alone is enough, where V1 needs the broader `symmetricFilterPropagation`. 4. CSV and JSON get no merging at all, which is deliberate rather than a follow-up. It costs them two shapes: scans whose projected columns differ, and scans reading the same columns under different filters, since `FileScan.equals` compares the normalized data filters so those are not canonically equal either and do not fall back to plain reuse. The V1 path merges both today and answers `[8, 80]` where two separate scans answer `[10, 80]`, so one subquery's result depends on what a sibling subquery projects. That is a V1 bug rather than a target to copy; SPARK-59107 (#58411) fixes it, and with that in both read paths decline these shapes alike. ### How was this patch tested? New suite `FileSourceV2PlanMergingSuite`, 16 tests. Every built-in file table is checked on the side it belongs to; a non-strict read is checked to withhold the capability on both strictness configurations, and separately to keep its scans separate and return `[45, 0]`, with the table and the temp view built outside the configuration scope so that a cached gate would fail the test. Those are two tests rather than one so that a mutation names which half it broke, the capability assertion having aborted before the rows could report. Scans differing only in projected columns merge for Parquet and ORC and decline for CSV and JSON in the same shape; text merges the one shape a single-column table can differ in, which also needs both aggregates to be hash-aggregatable or `PlanMerger.supportedAggregateMerge` declines above the scans; scans over the same partition filter merge with that filter still enforced on the rebuilt scan; three scans merge into one; differing data filters merge only with the dsv2 configuration on, and the merged scan is checked to carry the OR-widened predicate rather than only to exist. Declines are covered too: differing partition filters, nested-pruned columns, a pushed aggregate (with the aggregate itself asserted, not just the scan count), and two different tables holding different rows so that a cross-table merge would change the answer. Then V1/V2 parity on three shapes and the partition-filter gap pinned as the one shape where they disagree, both on parquet, whose reads this suite pins strict and whose parser does not depend on the projection, so SPARK-59107 leaves them merging on V1. The CSV and JSON parse behaviour is pinned on the V2 side alone, on three shapes: `DROPMALFORMED` gives `[10, 80]`, `PERMISSIVE` with `_corrupt_record` in the schema gives `[0, 80]`, and `FAILFAST` with a short CSV row returns rows rather than throwing. Subquery counts are asserted alongside the rows, so the result is attributed to the decline rather than inferred from the values, and the configurations the expectations depend on are pinned rather than assumed. These three used to assert the V1 numbers beside the V2 ones, and those arms pinned exactly what SPARK-59107 removes, so they are gone; its own suite covers that side. Measured at #58411's head, with this suite copied in: the CSV and JSON test passes there, and so does the parquet partition-filter one. Every test asserts which read path the plan took before asserting anything about merging. SQL-on-file and catalog tables resolve to the V1 `FileFormat` regardless of `spark.sql.sources.useV1SourceList`, so a suite driven by that configuration alone would quietly run V1 on both sides. One test in `AvroV2Suite`, because `AvroTable` lives in the module that has it on the classpath: it asserts the capability, that `positionalFieldMatching` withholds it, and that two scans differing only in their projected columns fuse into one reading the union. Mutation checks. Turning the three sql/core overrides to `false` fails 8 of the 16 tests; of the 8 that pass, 6 assert a decline, one asserts that V1 merges differing partition filters where V2 does not and V2 declines under the mutation too, and one asserts that a non-strict read keeps its scans separate, which the mutation also produces. Dropping `&& hasStrictFileReads` from `capabilities` fails both strictness tests, the capability one at its first assertion and the rows one with `[null, 0]` against `[45, 0]`, which is what splitting them was for. Regression: the `planmerging` suites, `ExplainSuite` and `ExplainSuiteAE`, `FileBasedDataSourceSuite`, `FileTableSuite`, `OrcV2SchemaPruningSuite`, `ParquetV2SchemaPruningSuite`, `ParquetV2FilterSuite`, `SubquerySuite`, `SameResultSuite`, `ParquetV2AggregatePushDownSuite`, `OrcV2AggregatePushDownSuite`, `DataSourceV2Suite` and `AvroV2Suite`. No golden file or `PlanStabilitySuite` plan needed regenerating. Those build their tables with `CREATE TABLE ... USING <format>`, which resolves to the V1 `FileFormat`, so none of them reaches a V2 file scan. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code Closes#58340 from LuciferYang/SPARK-57205. Authored-by: YangJie <yangjie01@baidu.com> Signed-off-by: yangjie01 <yangjie01@baidu.com> (cherry picked from commit 484866b) Signed-off-by: yangjie01 <yangjie01@baidu.com>
# Conflicts: # connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala
### What changes were proposed in this pull request? `PlanMerger` no longer merges two plans that read different sets of columns from a shared V1 file relation when the rows that relation returns depend on which columns the read asked for. `DataSourceUtils.isProjectionSensitiveRead` answers that question for a `HadoopFsRelation`, and `merge` compares, per shared relation and per occurrence of it, the columns each side reads before it tries to merge them. The two records have to match exactly rather than one containing the other: a cache entry's record stays true of it only because every plan merged in read the same columns, so admitting a narrower plan would make the record stale. Reads of the same columns still merge, so plain reuse is untouched. Two things make a read projection-sensitive. Its parser may resolve or validate a column against the set of columns it was asked for, which lets a wider read drop or rewrite rows the narrower one returned, or return different values for a column it was already reading: CSV, JSON and XML build their parser from the required schema and take `mode` and the corrupt-record column from it, and Avro under `positionalFieldMatching` pairs a column with the Avro field at its position in that schema, measured `[10, 100]` merged against `[10, 10]` unmerged. SPARK-59108 (#58409) removes that at the root, which retires the avro arm here along with the `AvroTable` gate #58340 adds on the V2 side; the landing order is below. Or the read may not be strict, in which case a failure in a column that only the wider read touches is swallowed together with the rest of that file's rows, whatever the format. The strictness half is `FileSourceOptions.hasStrictFileReads`, which this PR lifts out of `FileScanRDD` and points the other two spellings of it at, `InMemoryRelation`'s cache-repeatability check and the capability gate #58340 added to `FileTable`. It is evaluated per merge rather than cached, so a relation built before `ignoreCorruptFiles` was set still answers for the read that is running. The formats are named in `DataSourceUtils` rather than declared by each `FileFormat`, the way `SchemaPruning.canPruneDataSchema` names Parquet and ORC. A capability method on `FileFormat` would have to pick a default: "safe to widen" repeats this same list as the overrides, and "not safe" stops merging for every format that does not override, including Hive's ORC reader and every third-party format, for no correctness gain. A third-party format whose parser is projection-sensitive therefore keeps merging, as it does today. #58340 (SPARK-57205), now in master, recorded this V1 behaviour as a gap on its V2 side, where the same four shapes are declined; with this in, the two read paths agree on them. Landing order: this one, then #58409, which deletes the avro arm, the `org.apache.spark.sql.avro` import it needs, and the `AvroV1Suite` test. A branch cannot delete code that is not in master yet, which is the only reason the order matters. Neither direction breaks the build: that test compares the merged answer against the same query with `MergeSubplans` excluded rather than against a literal row, and asserts one column per scan, both of which hold whatever `sum(b)` returns once #58409 changes it, so #58409 landing first leaves the arm over-conservative rather than turning a test red. ### Why are the changes needed? Top-level column pruning for a V1 file source happens in physical planning, from the attributes referenced above the relation (`FileSourceStrategy` computes `readDataColumns` from `filterAttributes ++ projects`), so two `LogicalRelation`s over the same files canonicalize equal whatever each side projects. `PlanMerger`'s identical-plan path therefore reuses one of them and the union of the two column sets is formed one level up, which means one subquery's result can depend on what a sibling subquery projects. Measured on master, where all four shapes merge; each cell holds the two subqueries' values, and the second column is what this head returns: | shape | merged | not merged | |---|---|---| | `mode=DROPMALFORMED`, a record malformed only in `b`: `SELECT (SELECT sum(a) FROM t), (SELECT sum(b) FROM t)` | `[8, 80]` | `[10, 80]` | | `PERMISSIVE` with `_corrupt_record` in the schema: `count(_corrupt_record)` beside `sum(b)` | `[1, 80]` | `[0, 80]` | | `FAILFAST` with a CSV row carrying fewer tokens than the schema has columns | throws | `[10, 80]` | | `spark.sql.files.ignoreCorruptFiles=true`, parquet, `b` written as a string and read as a long: `sum(a)` beside `count(b)` | `[null, 0]` | `[45, 0]` | The first two reproduce for csv, json and xml alike, and the third does too with a format-appropriate malformation, a token count being a CSV notion. The fourth is not about parsing, so it reaches every format. The bug is not new. Measured on the maintenance branches with the same two csv shapes: on `branch-4.2`, where the rule is `catalyst.optimizer.MergeSubplans`, `DROPMALFORMED` answers `[8, 80]` against `[10, 80]` with the rule excluded and `PERMISSIVE` answers `[1, 80]` against `[0, 80]`; on `branch-3.5`, where it is `MergeScalarSubqueries`, the two-subquery query answers `[8, 80]` and `[1, 80]` while the first subquery run on its own answers `10` and `0`. `branch-4.0` and `branch-4.1` carry the same rule as 3.5 and were not run. This fix only reaches back to 4.3, though: from 4.2 down the rule lives in `sql/catalyst`, which cannot see `HadoopFsRelation`, so those branches would need a different seam. ### Does this PR introduce _any_ user-facing change? Yes. A query with two subqueries over the same CSV, JSON or XML relation that project different columns, over an avro relation read with `positionalFieldMatching`, or over any file relation under `ignoreCorruptFiles` or `ignoreMissingFiles`, now returns what two separate scans return, which is what the same query returned before subplan merging learned to merge it. The cost is one extra scan for those shapes. Everything else keeps merging, including two subqueries over the same CSV relation that read the same columns. ### How was this patch tested? New suite `FileSourceV1PlanMergingSuite`, 17 tests. The four shapes above, with `DROPMALFORMED` covered for csv, json and xml alike; a read that is not strict on both configurations, with the temp view built outside the configuration scope so that a cached answer would fail the test; a self join, where each of the two reads of the relation has to be compared on its own; a third subquery that reads a column the merged pair does not, and a fourth that joins the entry the refused third one opened; and six shapes that must keep merging: parquet subqueries projecting different columns, csv subqueries reading the same columns, an identical pair of subqueries, which are reused rather than merged, a third subquery that reads the same columns as the merged pair, a csv read where the two sides differ only in a partition column reference, which is not a column read at all, and a csv read the two sides merge through filter propagation, which rebuilds the projection above the relation. One test in `AvroV1Suite`, because `avro` does not resolve from the `sql/core` test classpath; it pins both strictness configurations rather than inheriting them, so that positional matching is the only reason its read is projection-sensitive, and it takes its expected values from the same query with `MergeSubplans` excluded, because the value `sum(b)` returns is a property of what its own subquery projects and SPARK-59108 changes it. Every test in the suite asserts the columns each `FileSourceScanExec` in the plan reads, rather than a scan count: the count depends on which scans physical reuse hid behind a leaf node, while the columns are the property this change is about. Finding a `FileSourceScanExec` at all is also what pins these tests to the V1 path. Filter propagation is on by default, and it merges two subqueries over such a relation by rebuilding the projection above it. Measured on six query shapes with symmetric propagation on and off, twelve runs in all, the merged answers match the answers with `MergeSubplans` excluded and no scan reads a column neither side asked for, because the rebuilt projection is over the pruned child. One of those shapes is the test above. Beyond the suite, ten shapes with three or four subqueries over one relation, including a self join, two different views, a mix of csv and parquet, the corrupt-record column, a partitioned table and a subquery that reads no column at all, were run with symmetric filter propagation on and off, twenty runs in all: every one returns what the same query returns with `MergeSubplans` excluded. Four mutation checks, each measured rather than inferred. Turning `isProjectionSensitiveRead` to false fails 11 of the 17 here and the avro test as well; the 6 that pass are the ones whose only assertion is that a merge still happens. Removing the avro case alone fails the avro test. Keeping one column set per relation instead of one per occurrence, which is what the first version of this did, fails only the self-join test, with `[84, 84]` where the fix gives `[85, 84]`. Keeping partition columns in the read set fails only the partition-reference test. Deriving the cached side's read set from the merged plan rather than from the record taken when it was cached fails the three-subquery and four-subquery tests, with `[14, 8, 88]` and `[14, 8, 88, 4]` where separate scans give `[18, 10, 88]` and `[18, 10, 88, 4]`. Regression, re-measured after merging master rather than carried over: the whole `planmerging` package, which now includes #58340's `FileSourceV2PlanMergingSuite`, plus `SubquerySuite`, `DataFrameSubquerySuite`, `ReuseExchangeAndSubquerySuite`, `ExplainSuite`, `ExplainSuiteAE`, `FileBasedDataSourceSuite` and `InMemoryColumnarQuerySuite`, 12 suites and 439 tests; the `datasources.csv`, `datasources.json` and `datasources.xml` packages, 19 suites and 1718 tests; `catalyst/scalastyle`, `sql/scalastyle`, `sql/Test/scalastyle` and `avro/Test/scalastyle`. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code Closes#58411 from LuciferYang/SPARK-59107. Authored-by: YangJie <yangjie01@baidu.com> Signed-off-by: yangjie01 <yangjie01@baidu.com> (cherry picked from commit b82f987) Signed-off-by: yangjie01 <yangjie01@baidu.com>
LuciferYang
commented
Sep 2, 2026
LuciferYang
commented
Sep 2, 2026
Thank you @peter-toth@uros-b@sunchao There were conflicts against |
### What changes were proposed in this pull request? Backport of `b82f9872d1cf` (#58411) to `branch-4.3`, with one hunk dropped and one sentence reworded; everything else is byte-identical to what landed on master. `PlanMerger` no longer merges two plans that read different sets of columns from a shared V1 file relation when the rows that relation returns depend on which columns the read asked for. `DataSourceUtils.isProjectionSensitiveRead` answers that question for a `HadoopFsRelation`, and `merge` compares, per shared relation and per occurrence of it, the columns each side reads before it tries to merge them. The two records have to match exactly rather than one containing the other: a cache entry's record stays true of it only because every plan merged in read the same columns, so admitting a narrower plan would make the record stale. Reads of the same columns still merge, so plain reuse is untouched. Two things make a read projection-sensitive. Its parser may resolve or validate a column against the set of columns it was asked for, which lets a wider read drop or rewrite rows the narrower one returned, or return different values for a column it was already reading: CSV, JSON and XML build their parser from the required schema and take `mode` and the corrupt-record column from it, and Avro under `positionalFieldMatching` pairs a column with the Avro field at its position in that schema. Or the read may not be strict, in which case a failure in a column that only the wider read touches is swallowed together with the rest of that file's rows, whatever the format. The strictness half becomes `FileSourceOptions.hasStrictFileReads`, which this lifts out of `FileScanRDD` so that the cache-repeatability check in `InMemoryRelation` shares it, and it is evaluated per merge rather than cached, so a relation built before `ignoreCorruptFiles` was set still answers for the read that is running. Two differences from the master commit, both because SPARK-57205 (#58340) is not on this branch: - The `FileTable.scala` hunk is dropped. On master that hunk points `FileTable.hasStrictFileReads` at the shared predicate; here `FileTable` has no such method, and no built-in file table declares the `SCAN_MERGING` capability, so a V2 file scan is never merged on this branch and needs no gate. The predicate's callers here are `FileScanRDD`, `InMemoryRelation` and the new method, which is exactly what its scaladoc names. - The scaladoc sentence about the Avro case calls SPARK-59108 a proposal rather than a landed fix, and names the arm it would retire, since that fix is on no branch and a maintenance branch should not carry an instruction whose precondition may never hold. ### Why are the changes needed? Top-level column pruning for a V1 file source happens in physical planning, from the attributes referenced above the relation (`FileSourceStrategy` computes `readDataColumns` from `filterAttributes ++ projects`), so two `LogicalRelation`s over the same files canonicalize equal whatever each side projects. `PlanMerger`'s identical-plan path therefore reuses one of them and the union of the two column sets is formed one level up, which means one subquery's result can depend on what a sibling subquery projects. Measured on this branch with the gate forced off, which is what 4.3.0 does; each cell holds the two subqueries' values, and the second column is what this head returns: | shape | merged | not merged | |---|---|---| | `mode=DROPMALFORMED`, a record malformed only in `b`: `SELECT (SELECT sum(a) FROM t), (SELECT sum(b) FROM t)` | `[8, 80]` | `[10, 80]` | | `PERMISSIVE` with `_corrupt_record` in the schema: `count(_corrupt_record)` beside `sum(b)` | `[1, 80]` | `[0, 80]` | | `FAILFAST` with a CSV row carrying fewer tokens than the schema has columns | throws | `[10, 80]` | | `spark.sql.files.ignoreCorruptFiles=true`, parquet, `b` written as a string and read as a long: `sum(a)` beside `count(b)` | `[null, 0]` | `[45, 0]` | The bug is old rather than new, and 4.3.0 shipped it. Measured with the first two shapes on the other maintenance branches as well: on `branch-4.2`, where the rule is `catalyst.optimizer.MergeSubplans`, `DROPMALFORMED` answers `[8, 80]` against `[10, 80]` with the rule excluded and `PERMISSIVE` answers `[1, 80]` against `[0, 80]`; on `branch-3.5`, where it is `MergeScalarSubqueries`, the two-subquery query answers `[8, 80]` and `[1, 80]` while the first subquery run on its own answers `10` and `0`. `branch-4.0` and `branch-4.1` carry the same rule as 3.5 and were not run. This patch does not reach those branches: from 4.2 down the rule lives in `sql/catalyst`, which cannot see `HadoopFsRelation`, so they would need a different seam. ### Does this PR introduce _any_ user-facing change? Yes, and on a maintenance branch that is worth spelling out. A query with two subqueries over the same CSV, JSON or XML relation that project different columns, over an avro relation read with `positionalFieldMatching`, or over any file relation under `ignoreCorruptFiles` or `ignoreMissingFiles`, now returns what two separate scans return. That is what the same query returned before subplan merging learned to merge it, and what 4.3.0 does not return, so the values those shapes answer with change in 4.3.1. The cost is one extra scan for those shapes. `MergeSubplans` runs unconditionally, and the gate declines merging for any V1 file relation whose reads are not strict, which includes parquet and orc under `ignoreMissingFiles` where the scaladoc itself concedes there is no correctness mechanism, only predicate parity with the reader. Users who have either flag on therefore lose scan sharing they had in 4.3.0. The direction is safe, since declining a merge cannot change an answer, but it is the part most likely to be noticed. Everything else keeps merging, including two subqueries over the same CSV relation that read the same columns. ### How was this patch tested? New suite `FileSourceV1PlanMergingSuite`, 17 tests, and one test in `AvroV1Suite`, both as they landed on master; the whole test diff is byte-identical to `b82f9872d1cf`. The four shapes above, with `DROPMALFORMED` covered for csv, json and xml alike; a read that is not strict on both configurations, with the temp view built outside the configuration scope so that a cached answer would fail the test; a self join, where each of the two reads of the relation has to be compared on its own; a third subquery that reads a column the merged pair does not, and a fourth that joins the entry the refused third one opened; and six shapes that must keep merging, so that the gate is not simply switching merging off. Every test asserts the columns each `FileSourceScanExec` in the plan reads, rather than a scan count: the count depends on which scans physical reuse hid behind a leaf node, while the columns are the property this change is about. Finding a `FileSourceScanExec` at all is also what pins these tests to the V1 path. Run on this branch: `FileSourceV1PlanMergingSuite` 17 of 17, the `AvroV1Suite` case, and `catalyst/scalastyle`, `sql/scalastyle`, `sql/Test/scalastyle` and `avro/Test/scalastyle`. Turning `isProjectionSensitiveRead` to false here fails 11 of the 17, with the values in the table above, and the 6 that pass are the ones whose only assertion is that a merge still happens; that is also where the table's numbers come from. The remaining mutation checks and the multi-subquery sweeps behind the design are on #58411 and were not re-run, since the code under them is identical. What was re-verified for this branch rather than carried over: the optimizer reruns `ColumnPruning` after the `MergeSubplans` batch, `tryMergePlans` pairs relation occurrences in plan order, every configuration the suite sets exists here with the same default, `MergeSubplans` is excludable so the avro test's expected value really is the unmerged one, and `javap` shows the `def`-to-`val` change on `FileScanRDD` retains one boolean rather than the options map. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code Closes#58472 from LuciferYang/SPARK-59107-4.3. Authored-by: YangJie <yangjie01@baidu.com> Signed-off-by: yangjie01 <yangjie01@baidu.com>
### What changes were proposed in this pull request? `AvroDeserializer` now takes the schema its Catalyst schema was projected from, and under `positionalFieldMatching` it resolves a Catalyst field against that field's position in the data schema rather than its position in the projection. `AvroUtils.AvroSchemaHelper` takes 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). The three read call sites pass the data schema: `AvroPartitionReaderFactory` on the V2 path, `AvroFileFormat.buildReader` and `AvroFileFormat.readArchive` on V1. A nested record keeps resolving by its own positions, since neither read path prunes nested fields: `FileScanBuilder.supportsNestedSchemaPruning` is false and `AvroScanBuilder` does not override it, and `SchemaPruning.canPruneDataSchema` covers only Parquet and ORC. ORC already does this for `orc.force.positional.evolution`: `OrcUtils.requestedColumnIds` maps the required schema through `dataSchema.fieldIndex(name)`, which makes its positional path projection-independent. Avro decodes the whole record whatever the projection asks for, so nothing extra is read. Two gates kept avro out of scan merging because merging widens the projection, and this commit retires both, since both landed while this was open: #58340 (SPARK-57205, `484866bd80d`) withheld the `SCAN_MERGING` capability from `AvroTable` on the V2 path, and #58411 (SPARK-59107, `b82f9872d1c`) named avro in `DataSourceUtils.isProjectionSensitiveRead` on the V1 path. `AvroTable.supportsScanMerging` becomes an unconditional `true` rather than a deleted override, because `FileTable` defaults it to false and dropping it would take the capability away from Avro V2 altogether; `hasProjectionSensitiveParser` loses its avro arm and, with it, its `options` parameter and the `org.apache.spark.sql.avro` import. Two tests go with them: the `AvroV1Suite` case #58411 added, whose assertion that each subquery keeps its own scan stops being true, and the capability assertion in the `AvroV2Suite` SPARK-57205 case. The documentation has to change whether or not the predicates come off, because it states the old behaviour in two places: `docs/sql-performance-tuning.md` listed avro among the projection-sensitive V1 relations and said Avro withholds the capability under `positionalFieldMatching`, and after this fix the position is the data schema's. `FileTable`'s class doc names the same shape as a general statement of the contract, so that one stays. A backport has to carry the same removal wherever both prerequisites are, which today is `master` and `branch-4.x`. One shape stays broken, with or without this change: `recursiveFieldMaxDepth` makes `SchemaConverters` drop 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=true` the deserializer is built from the projected read schema while the Avro side stays the full Avro schema, and `AvroUtils.AvroSchemaHelper.getAvroField` pairs 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 fields `a`, `b`, `c` hold `id`, `100 * id`, `10000 * id` for ids 0 to 4, read with the option on: ``` sql("SELECT sum(a), sum(b), sum(c) FROM t").show() // 10, 1000, 100000 -- all correct sql("SELECT sum(c) FROM t").show() // 10 -- should be 100000 sql("SELECT sum(b) FROM t").show() // 10 -- should be 1000 sql("SELECT sum(a), sum(c) FROM t").show() // 10, 1000 -- sum(c) should be 100000 ``` 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. ### Does this PR introduce _any_ user-facing change? Yes, a bug fix on the Avro read path, both V1 and V2. A read that sets `positionalFieldMatching` and 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 in `from_avro`. ### How was this patch tested? Five new tests in `AvroSuite`, so each runs on both read paths (`AvroV1Suite` and `AvroV2Suite` extend 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 of `spark.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 the `avroSchema` option 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 in `AvroSchemaHelperSuite` for the helper itself, and one in `AvroArchiveReadBase`, which runs in the tar, zip and 7z suites, because the archive reader builds its own deserializer per entry. Two more tests, one in `AvroV1Suite` and one in `AvroV2Suite`, for the shape the removed gates used to decline. 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 scans are one widened read of both columns, `FileSourceScanExec` on V1 and one canonically distinct `DataSourceV2ScanRelation` on V2, and the V2 case also asserts that the positional table now advertises `SCAN_MERGING`. Both pin the strictness flags, since a non-strict read is projection-sensitive for the other reason. Mutation checks. With the position mapping disabled, all ten of the original `AvroSuite` cases fail (five on each path), so does the archive one, and so do the two new ones, which answer `[10, 100]` where the file has `[100, 1000]`. With the two gates put back instead, the two new ones fail the other way, the V1 one with a scan per column rather than one reading both and the V2 one at the capability assertion. Between the two mutations they pin both halves: that merging is allowed here, and that what makes it safe is the mapping. The two columns in the archive test have different types on purpose, so a wrong pairing fails the read there rather than returning plausible values. Regression, re-measured at this head after the merge and the gate removal: the whole `avro` module, 501 tests, the `planmerging` package, 134 tests, since removing the avro arm touches the shared predicate, and `avro/scalastyle`, `avro/Test/scalastyle`, `sql/scalastyle`, `sql/Test/scalastyle` and `catalyst/scalastyle`. `RocksDBStateEncoderSuite` plus `StateStoreSuite`, 840 tests, were run on the pre-merge head, because the state-store encoder builds an `AvroDeserializer` too and nothing since has touched it. The existing `positionalFieldMatching` tests (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 Closes#58409 from LuciferYang/SPARK-59108. Authored-by: YangJie <yangjie01@baidu.com> Signed-off-by: yangjie01 <yangjie01@baidu.com>
### What changes were proposed in this pull request? `AvroDeserializer` now takes the schema its Catalyst schema was projected from, and under `positionalFieldMatching` it resolves a Catalyst field against that field's position in the data schema rather than its position in the projection. `AvroUtils.AvroSchemaHelper` takes 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). The three read call sites pass the data schema: `AvroPartitionReaderFactory` on the V2 path, `AvroFileFormat.buildReader` and `AvroFileFormat.readArchive` on V1. A nested record keeps resolving by its own positions, since neither read path prunes nested fields: `FileScanBuilder.supportsNestedSchemaPruning` is false and `AvroScanBuilder` does not override it, and `SchemaPruning.canPruneDataSchema` covers only Parquet and ORC. ORC already does this for `orc.force.positional.evolution`: `OrcUtils.requestedColumnIds` maps the required schema through `dataSchema.fieldIndex(name)`, which makes its positional path projection-independent. Avro decodes the whole record whatever the projection asks for, so nothing extra is read. Two gates kept avro out of scan merging because merging widens the projection, and this commit retires both, since both landed while this was open: #58340 (SPARK-57205, `484866bd80d`) withheld the `SCAN_MERGING` capability from `AvroTable` on the V2 path, and #58411 (SPARK-59107, `b82f9872d1c`) named avro in `DataSourceUtils.isProjectionSensitiveRead` on the V1 path. `AvroTable.supportsScanMerging` becomes an unconditional `true` rather than a deleted override, because `FileTable` defaults it to false and dropping it would take the capability away from Avro V2 altogether; `hasProjectionSensitiveParser` loses its avro arm and, with it, its `options` parameter and the `org.apache.spark.sql.avro` import. Two tests go with them: the `AvroV1Suite` case #58411 added, whose assertion that each subquery keeps its own scan stops being true, and the capability assertion in the `AvroV2Suite` SPARK-57205 case. The documentation has to change whether or not the predicates come off, because it states the old behaviour in two places: `docs/sql-performance-tuning.md` listed avro among the projection-sensitive V1 relations and said Avro withholds the capability under `positionalFieldMatching`, and after this fix the position is the data schema's. `FileTable`'s class doc names the same shape as a general statement of the contract, so that one stays. A backport has to carry the same removal wherever both prerequisites are, which today is `master` and `branch-4.x`. One shape stays broken, with or without this change: `recursiveFieldMaxDepth` makes `SchemaConverters` drop 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=true` the deserializer is built from the projected read schema while the Avro side stays the full Avro schema, and `AvroUtils.AvroSchemaHelper.getAvroField` pairs 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 fields `a`, `b`, `c` hold `id`, `100 * id`, `10000 * id` for ids 0 to 4, read with the option on: ``` sql("SELECT sum(a), sum(b), sum(c) FROM t").show() // 10, 1000, 100000 -- all correct sql("SELECT sum(c) FROM t").show() // 10 -- should be 100000 sql("SELECT sum(b) FROM t").show() // 10 -- should be 1000 sql("SELECT sum(a), sum(c) FROM t").show() // 10, 1000 -- sum(c) should be 100000 ``` 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. ### Does this PR introduce _any_ user-facing change? Yes, a bug fix on the Avro read path, both V1 and V2. A read that sets `positionalFieldMatching` and 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 in `from_avro`. ### How was this patch tested? Five new tests in `AvroSuite`, so each runs on both read paths (`AvroV1Suite` and `AvroV2Suite` extend 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 of `spark.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 the `avroSchema` option 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 in `AvroSchemaHelperSuite` for the helper itself, and one in `AvroArchiveReadBase`, which runs in the tar, zip and 7z suites, because the archive reader builds its own deserializer per entry. Two more tests, one in `AvroV1Suite` and one in `AvroV2Suite`, for the shape the removed gates used to decline. 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 scans are one widened read of both columns, `FileSourceScanExec` on V1 and one canonically distinct `DataSourceV2ScanRelation` on V2, and the V2 case also asserts that the positional table now advertises `SCAN_MERGING`. Both pin the strictness flags, since a non-strict read is projection-sensitive for the other reason. Mutation checks. With the position mapping disabled, all ten of the original `AvroSuite` cases fail (five on each path), so does the archive one, and so do the two new ones, which answer `[10, 100]` where the file has `[100, 1000]`. With the two gates put back instead, the two new ones fail the other way, the V1 one with a scan per column rather than one reading both and the V2 one at the capability assertion. Between the two mutations they pin both halves: that merging is allowed here, and that what makes it safe is the mapping. The two columns in the archive test have different types on purpose, so a wrong pairing fails the read there rather than returning plausible values. Regression, re-measured at this head after the merge and the gate removal: the whole `avro` module, 501 tests, the `planmerging` package, 134 tests, since removing the avro arm touches the shared predicate, and `avro/scalastyle`, `avro/Test/scalastyle`, `sql/scalastyle`, `sql/Test/scalastyle` and `catalyst/scalastyle`. `RocksDBStateEncoderSuite` plus `StateStoreSuite`, 840 tests, were run on the pre-merge head, because the state-store encoder builds an `AvroDeserializer` too and nothing since has touched it. The existing `positionalFieldMatching` tests (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 Closes#58409 from LuciferYang/SPARK-59108. Authored-by: YangJie <yangjie01@baidu.com> Signed-off-by: yangjie01 <yangjie01@baidu.com> (cherry picked from commit c809c28) Signed-off-by: yangjie01 <yangjie01@baidu.com>
…ning ### What changes were proposed in this pull request? Backport of `c809c283c2d` (#58409) to `branch-4.3`. The Avro fix and the V1 half of the gate removal come over as they landed; the V2 half is dropped, for the reason in the fourth paragraph. `AvroDeserializer` now takes the schema its Catalyst schema was projected from, and under `positionalFieldMatching` it resolves a Catalyst field against that field's position in the data schema rather than its position in the projection. `AvroUtils.AvroSchemaHelper` takes 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). The three read call sites pass the data schema: `AvroPartitionReaderFactory` on the V2 path, `AvroFileFormat.buildReader` and `AvroFileFormat.readArchive` on V1. A nested record keeps resolving by its own positions, since neither read path prunes nested fields: `FileScanBuilder.supportsNestedSchemaPruning` is false and `AvroScanBuilder` does not override it, and `SchemaPruning.canPruneDataSchema` covers only Parquet and ORC. ORC already does this for `orc.force.positional.evolution`: `OrcUtils.requestedColumnIds` maps the required schema through `dataSchema.fieldIndex(name)`, which makes its positional path projection-independent. Avro decodes the whole record whatever the projection asks for, so nothing extra is read. One gate kept avro out of V1 scan merging because merging widens the projection, and this commit retires it: #58411 (SPARK-59107, on this branch as `04cdb66c83d`) named avro in `DataSourceUtils.isProjectionSensitiveRead`. `hasProjectionSensitiveParser` loses its avro arm and, with it, its `options` parameter and the `org.apache.spark.sql.avro` import; the `AvroV1Suite` case that PR added goes too, since its assertion that each subquery keeps its own scan stops being true. `docs/sql-performance-tuning.md` no longer lists avro among the projection-sensitive V1 relations, which it has to stop doing whether or not the predicate comes off, because after this fix the position is the data schema's. The V2 half is not here. SPARK-57205 (#58340) is not on `branch-4.3`, so `AvroTable` has no `supportsScanMerging` to turn on, the `AvroV2Suite` capability case it added does not exist, the performance guide has no V2 paragraph, and the V2 twin of the new merge test cannot hold on a branch where avro never declares `SCAN_MERGING`. Those four hunks are the whole difference from the master commit. One shape stays broken, with or without this change: `recursiveFieldMaxDepth` makes `SchemaConverters` drop 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=true` the deserializer is built from the projected read schema while the Avro side stays the full Avro schema, and `AvroUtils.AvroSchemaHelper.getAvroField` pairs 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 fields `a`, `b`, `c` hold `id`, `100 * id`, `10000 * id` for ids 0 to 4, read with the option on: ``` sql("SELECT sum(a), sum(b), sum(c) FROM t").show() // 10, 1000, 100000 -- all correct sql("SELECT sum(c) FROM t").show() // 10 -- should be 100000 sql("SELECT sum(b) FROM t").show() // 10 -- should be 1000 sql("SELECT sum(a), sum(c) FROM t").show() // 10, 1000 -- sum(c) should be 100000 ``` 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. ### Does this PR introduce _any_ user-facing change? Yes, a bug fix on the Avro read path, both V1 and V2, and 4.3.0 shipped the bug: positional matching has resolved against the projection since 3.2.0 (SPARK-34365). A read that sets `positionalFieldMatching` and 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 in `from_avro`. ### How was this patch tested? Five new tests in `AvroSuite`, so each runs on both read paths (`AvroV1Suite` and `AvroV2Suite` extend 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 of `spark.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 the `avroSchema` option 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 in `AvroSchemaHelperSuite` for the helper itself, and one in `AvroArchiveReadBase`, which runs in the tar, zip and 7z suites, because the archive reader builds its own deserializer per entry. One more test, in `AvroV1Suite`, for the shape the removed gate used to decline. 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 scans are one widened `FileSourceScanExec` reading both columns. It pins the strictness flags, since a non-strict read is projection-sensitive for the other reason. Master's `AvroV2Suite` twin is not here, for the reason above. Mutation check, measured on this branch: with the position mapping disabled, 14 cases fail, the five `SPARK-59108` shapes on each read path, the archive one in each of the tar, zip and 7z suites, and the new merge test, which answers `[10, 100]` where the file has `[100, 1000]`. The two columns in the archive test have different types on purpose, so a wrong pairing fails the read there rather than returning plausible values. Regression, measured on this branch: the whole `avro` module, 499 tests, the `planmerging` package, 108 tests, since removing the avro arm touches the shared predicate, and `avro/scalastyle`, `avro/Test/scalastyle`, `sql/scalastyle` and `catalyst/scalastyle`. `RocksDBStateEncoderSuite` and `StateStoreSuite`, which also build an `AvroDeserializer`, were run on master rather than here; nothing in this backport differs from the master commit in that path. The existing `positionalFieldMatching` tests (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 Closes#58513 from LuciferYang/SPARK-59108-4.3. Authored-by: YangJie <yangjie01@baidu.com> Signed-off-by: yangjie01 <yangjie01@baidu.com>
What changes were proposed in this pull request?
PlanMergerno longer merges two plans that read different sets of columns from a shared V1 file relation when the rows that relation returns depend on which columns the read asked for.DataSourceUtils.isProjectionSensitiveReadanswers that question for aHadoopFsRelation, andmergecompares, per shared relation and per occurrence of it, the columns each side reads before it tries to merge them. The two records have to match exactly rather than one containing the other: a cache entry's record stays true of it only because every plan merged in read the same columns, so admitting a narrower plan would make the record stale. Reads of the same columns still merge, so plain reuse is untouched.Two things make a read projection-sensitive. Its parser may resolve or validate a column against the set of columns it was asked for, which lets a wider read drop or rewrite rows the narrower one returned, or return different values for a column it was already reading: CSV, JSON and XML build their parser from the required schema and take
modeand the corrupt-record column from it, and Avro underpositionalFieldMatchingpairs a column with the Avro field at its position in that schema, measured[10, 100]merged against[10, 10]unmerged. SPARK-59108 (#58409) removes that at the root, which retires the avro arm here along with theAvroTablegate #58340 adds on the V2 side; the landing order is below. Or the read may not be strict, in which case a failure in a column that only the wider read touches is swallowed together with the rest of that file's rows, whatever the format. The strictness half isFileSourceOptions.hasStrictFileReads, which this PR lifts out ofFileScanRDDand points the other two spellings of it at,InMemoryRelation's cache-repeatability check and the capability gate #58340 added toFileTable. It is evaluated per merge rather than cached, so a relation built beforeignoreCorruptFileswas set still answers for the read that is running.The formats are named in
DataSourceUtilsrather than declared by eachFileFormat, the waySchemaPruning.canPruneDataSchemanames Parquet and ORC. A capability method onFileFormatwould have to pick a default: "safe to widen" repeats this same list as the overrides, and "not safe" stops merging for every format that does not override, including Hive's ORC reader and every third-party format, for no correctness gain. A third-party format whose parser is projection-sensitive therefore keeps merging, as it does today.#58340 (SPARK-57205), now in master, recorded this V1 behaviour as a gap on its V2 side, where the same four shapes are declined; with this in, the two read paths agree on them.
Landing order: this one, then #58409, which deletes the avro arm, the
org.apache.spark.sql.avroimport it needs, and theAvroV1Suitetest. A branch cannot delete code that is not in master yet, which is the only reason the order matters. Neither direction breaks the build: that test compares the merged answer against the same query withMergeSubplansexcluded rather than against a literal row, and asserts one column per scan, both of which hold whateversum(b)returns once #58409 changes it, so #58409 landing first leaves the arm over-conservative rather than turning a test red.Why are the changes needed?
Top-level column pruning for a V1 file source happens in physical planning, from the attributes referenced above the relation (
FileSourceStrategycomputesreadDataColumnsfromfilterAttributes ++ projects), so twoLogicalRelations over the same files canonicalize equal whatever each side projects.PlanMerger's identical-plan path therefore reuses one of them and the union of the two column sets is formed one level up, which means one subquery's result can depend on what a sibling subquery projects. Measured on master, where all four shapes merge; each cell holds the two subqueries' values, and the second column is what this head returns:mode=DROPMALFORMED, a record malformed only inb:SELECT (SELECT sum(a) FROM t), (SELECT sum(b) FROM t)[8, 80][10, 80]PERMISSIVEwith_corrupt_recordin the schema:count(_corrupt_record)besidesum(b)[1, 80][0, 80]FAILFASTwith a CSV row carrying fewer tokens than the schema has columns[10, 80]spark.sql.files.ignoreCorruptFiles=true, parquet,bwritten as a string and read as a long:sum(a)besidecount(b)[null, 0][45, 0]The first two reproduce for csv, json and xml alike, and the third does too with a format-appropriate malformation, a token count being a CSV notion. The fourth is not about parsing, so it reaches every format.
The bug is not new. Measured on the maintenance branches with the same two csv shapes: on
branch-4.2, where the rule iscatalyst.optimizer.MergeSubplans,DROPMALFORMEDanswers[8, 80]against[10, 80]with the rule excluded andPERMISSIVEanswers[1, 80]against[0, 80]; onbranch-3.5, where it isMergeScalarSubqueries, the two-subquery query answers[8, 80]and[1, 80]while the first subquery run on its own answers10and0.branch-4.0andbranch-4.1carry the same rule as 3.5 and were not run. This fix only reaches back to 4.3, though: from 4.2 down the rule lives insql/catalyst, which cannot seeHadoopFsRelation, so those branches would need a different seam.Does this PR introduce any user-facing change?
Yes. A query with two subqueries over the same CSV, JSON or XML relation that project different columns, over an avro relation read with
positionalFieldMatching, or over any file relation underignoreCorruptFilesorignoreMissingFiles, now returns what two separate scans return, which is what the same query returned before subplan merging learned to merge it. The cost is one extra scan for those shapes. Everything else keeps merging, including two subqueries over the same CSV relation that read the same columns.How was this patch tested?
New suite
FileSourceV1PlanMergingSuite, 17 tests. The four shapes above, withDROPMALFORMEDcovered for csv, json and xml alike; a read that is not strict on both configurations, with the temp view built outside the configuration scope so that a cached answer would fail the test; a self join, where each of the two reads of the relation has to be compared on its own; a third subquery that reads a column the merged pair does not, and a fourth that joins the entry the refused third one opened; and six shapes that must keep merging: parquet subqueries projecting different columns, csv subqueries reading the same columns, an identical pair of subqueries, which are reused rather than merged, a third subquery that reads the same columns as the merged pair, a csv read where the two sides differ only in a partition column reference, which is not a column read at all, and a csv read the two sides merge through filter propagation, which rebuilds the projection above the relation. One test inAvroV1Suite, becauseavrodoes not resolve from thesql/coretest classpath; it pins both strictness configurations rather than inheriting them, so that positional matching is the only reason its read is projection-sensitive, and it takes its expected values from the same query withMergeSubplansexcluded, because the valuesum(b)returns is a property of what its own subquery projects and SPARK-59108 changes it.Every test in the suite asserts the columns each
FileSourceScanExecin the plan reads, rather than a scan count: the count depends on which scans physical reuse hid behind a leaf node, while the columns are the property this change is about. Finding aFileSourceScanExecat all is also what pins these tests to the V1 path.Filter propagation is on by default, and it merges two subqueries over such a relation by rebuilding the projection above it. Measured on six query shapes with symmetric propagation on and off, twelve runs in all, the merged answers match the answers with
MergeSubplansexcluded and no scan reads a column neither side asked for, because the rebuilt projection is over the pruned child. One of those shapes is the test above.Beyond the suite, ten shapes with three or four subqueries over one relation, including a self join, two different views, a mix of csv and parquet, the corrupt-record column, a partitioned table and a subquery that reads no column at all, were run with symmetric filter propagation on and off, twenty runs in all: every one returns what the same query returns with
MergeSubplansexcluded.Four mutation checks, each measured rather than inferred. Turning
isProjectionSensitiveReadto false fails 11 of the 17 here and the avro test as well; the 6 that pass are the ones whose only assertion is that a merge still happens. Removing the avro case alone fails the avro test. Keeping one column set per relation instead of one per occurrence, which is what the first version of this did, fails only the self-join test, with[84, 84]where the fix gives[85, 84]. Keeping partition columns in the read set fails only the partition-reference test. Deriving the cached side's read set from the merged plan rather than from the record taken when it was cached fails the three-subquery and four-subquery tests, with[14, 8, 88]and[14, 8, 88, 4]where separate scans give[18, 10, 88]and[18, 10, 88, 4].Regression, re-measured after merging master rather than carried over: the whole
planmergingpackage, which now includes #58340'sFileSourceV2PlanMergingSuite, plusSubquerySuite,DataFrameSubquerySuite,ReuseExchangeAndSubquerySuite,ExplainSuite,ExplainSuiteAE,FileBasedDataSourceSuiteandInMemoryColumnarQuerySuite, 12 suites and 439 tests; thedatasources.csv,datasources.jsonanddatasources.xmlpackages, 19 suites and 1718 tests;catalyst/scalastyle,sql/scalastyle,sql/Test/scalastyleandavro/Test/scalastyle.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code