[SPARK-40259][SQL] Support merging equivalent DataSource V2 scans in MergeSubplans - #57360
[SPARK-40259][SQL] Support merging equivalent DataSource V2 scans in MergeSubplans#57360peter-toth wants to merge 20 commits into
Conversation
… package Relocate the MergeSubplans optimizer rule and its PlanMerger helper from sql/catalyst (org.apache.spark.sql.catalyst.optimizer) to a new sql/core package org.apache.spark.sql.execution.planmerging. The ScalarSubqueryReference and NonGroupingAggregateReference placeholder expressions stay in catalyst (MergeSubplansReferences.scala) because BloomFilterMightContain references them. Also move the existing sql/core PlanMergeSuite into the new package as PlanMergingSuite, and move MergeSubplansSuite alongside the rule.
2129bdf to
d91b200
Compare
…MergeSubplans Add a generic, Spark-side merge of two DataSource V2 scans of the same table that differ only in projected columns and/or pushed filters, so subplans reading the same V2 table are fused into one scan (as already happens for V1 file sources). A source opts in with the new SupportsScanMerging marker; PlanMerger rebuilds the merged scan by driving the real V2ScanRelationPushDown, so the iterative PartitionPredicate pass runs and the (equal) strict filters come back fully enforced. DataSourceV2ScanRelation.pushedFilters now records the complete set of fully-pushed filters (referencing the relation's pre-pruning output), fixing a pre-existing bug where a filter on a pruned-out column silently vanished from the field. hasBlockingPushdown blocks the merge whenever a pushdown cannot be reproduced by rebuilding the scan: aggregate/join/limit/offset/top-N/sample, and now a fully-pushed non-deterministic filter (dropped from the deterministic-only pushedFilters, so it cannot be re-applied on the rebuilt scan).
d91b200 to
dfab083
Compare
LuciferYang
left a comment
There was a problem hiding this comment.
This framework is indeed more generic. Will the implementation for the built-in FileScan be handled as a follow-up?
| "the merged scan must keep the shared strict filter on a") | ||
| } | ||
|
|
||
| test("SPARK-40259: a pushed limit blocks merging (hasBlockingPushdown classification)") { |
There was a problem hiding this comment.
hasBlockingPushdown lists five merge-blocking pushdowns: limit, offset, sample, top-N, and the non-deterministic filter. The tests only exercise limit (and only as a flag assertion) plus the non-deterministic filter. Nothing covers pushedOffset, pushedSample, or sortOrders. If one of them ever falls out of the denylist, two scans that pushed a sample or top-N would fuse into one, the result would no longer be reproducible, and no test would fail. Could you add a flag assertion for offset/sample/top-N to match the limit one, and for at least one of them also assert that two such scans don't fuse? That way the whole flag-to-decline path is covered.
| case s: DataSourceV2ScanRelation => s | ||
| } | ||
|
|
||
| test("SPARK-40259: merge two DSv2 scans whose filter is strict only via the second pass") { |
There was a problem hiding this comment.
The Phase 2 OR-widen pruning (rePushMergedScan) only has the plan-shape assertions in MergeSubplansSuite. Both end-to-end tests in DSv2PlanMergingSuite use strict partition filters, so neither one goes through the path where the two post-scan filters differ and the scan gets OR-widened. I think this path is already fairly safe: what gets pushed is Or(np, cp), which is a superset, the Filter above re-checks exactness, and the non-deterministic case is caught by :2262. So this is a nice-to-have, not a blocker. If you want the end-to-end suite to cover it too, you could add a checkAnswer case in DSv2PlanMergingSuite with different post-scan filters (say c1 > 1 / c2 > 1), and throw in a rand() on one side while you're at it.
| s"expected the identical filter a > 1 to be re-pushed to the merged scan, got: $pushed") | ||
| } | ||
|
|
||
| test("SPARK-40259: merge three DSv2 scans with differing filters re-pushes the full OR") { |
There was a problem hiding this comment.
this test uses three distinct filters, so it only hits the new-alias branch of the tagged path. The reuse branch, where the third filter matches one of the earlier sides (existingNPFilter = Some, along with the rePushMergedScan it drives), never runs against a DSv2 scan. The general reuse test at :818 uses a LocalRelation, which doesn't drive rePushMergedScan. It'd be worth adding a 3-way DSv2 case where the third side's filter reuses one of the earlier ones, so the conditions collection in the reuse branch gets exercised too.
| // sides, so keep it unchanged. But if it sits above a merged DSv2 scan, recover the | ||
| // row-group pruning for its condition on that scan (Phase 2). | ||
| val prunedChild = rePushMergedScan(mergedChild, Some(cp.condition)) | ||
| val mergedPlan = Filter(cp.condition, prunedChild) |
There was a problem hiding this comment.
Regarding the MERGED_FILTER_TAG branch below(:349), a non-Project child throws IllegalStateException (this line was migrated in, not added by this PR, but the PR is what first lets a DSv2 scan reach it). Right now the tag is only ever set on a filter whose child is a Project, so the branch is unreachable. Still, since the merge is meant to be best-effort and should fall back to not merging on anything unexpected, throwing here instead of returning None is a bit fragile. Might be worth turning it into a fallback while you're in here.
| private def samePushedFilters( | ||
| np: DataSourceV2ScanRelation, | ||
| cp: DataSourceV2ScanRelation): Boolean = { | ||
| val cpOutputByName = cp.relation.output.map(a => a.name -> a).toMap |
There was a problem hiding this comment.
The output.map(a => a.name -> a).toMap idiom shows up three times, in samePushedFilters, tryMergeScanRelations, and buildMergedScan(:529、:583、:617). A one-line private helper (something like byName(attrs)) would tidy it up. Pure cleanup.
|
Just a question: Are you targeting |
|
Thanks for the reminder about the cut. I will try to conclude it Monday or Tuesday so maybe it might make it into the release, but not a must-have for 4.3. |
Follow-up to the reviewed feature commit, kept separate so the reviewed commit is unchanged. - MergeSubplansSuite: extend the hasBlockingPushdown classification test to drive offset, top-N and sample through the real pushdown (not just limit), plus an end-to-end case asserting a real pushed-sample scan declines to fuse. Limit/offset/sample are isolated; top-N sets sortOrders and pushedLimit together, so it exercises the top-N path without isolating sortOrders (no plan pushes a sort order without a limit). - MergeSubplansSuite: add a three-way merge test where the third side's filter reuses an earlier side's, exercising the propagated-filter reuse branch against a DSv2 scan (the existing all-distinct-filters test does not). - DSv2PlanMergingSuite: add an end-to-end OR-widen test with differing best-effort (post-scan) filters, running the differing-filter path that the plan-shape tests cover only structurally. - PlanMerger: decline the merge (return None) instead of throwing on a non-Project child under MERGED_FILTER_TAG; the case is unreachable but a best-effort merge should not fail. - PlanMerger: extract a byName helper for the repeated name -> attribute maps.
…anmerger-support # Conflicts: # sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2Suite.scala
…undant with SPARK-58207 SPARK-58207 (now on master) makes SupportsPushDownV2Filters never push a non-deterministic filter to the source, and the Catalyst (SPARK-58112) and V1 paths already never do. So a non-deterministic filter can never be fully pushed, and the pushedNonDeterministicFilter guard added for the scan merge can never fire. Remove the now-dead guard: the ScanBuilderHolder.pushedNonDeterministicFilter field, the code that set it and the hasBlockingPushdown term that read it in V2ScanRelationPushDown, the unreachable "a fully pushed non-deterministic filter blocks merging" test, and the enforceAll test knob it was the sole user of.
|
Yes, built-in FileScan is a follow-up. The main thing needed is for the built-in file scans (ParquetScan, OrcScan, ...) to opt into More broadly, DSv2 file sources have lagged V1 for a while — they still fall back to V1 by default ( Somewhat related: I remember a comment on some PR listing the features DSv2 file sources still lack to reach V1 parity, but I can't find it again — do you happen to remember it or have a pointer? It'd help scope the file-source follow-up. Also, I'm keeping this a draft for now — I'm not fully happy with the merge logic yet, so I'll iterate and come back to it in a day or two. |
…ilter Previously the DSv2 scan merge built the scan twice per merge round: strict-only at the leaf, then rebuilt at the enclosing Filter to add the best-effort OR pruning. Defer the leaf build when a Filter sits above it (MergeContext.filterAboveScan) and build the scan once at the Filter, with the strict filters plus the OR pruning together. DSv2 scan pairs are now handled in a single place in tryMergePlans. Also add a DSv2-specific symmetric-filter-propagation config (default true) so a DSv2 scan pair with equal strict filters can merge under differing best-effort filters even when the general symmetric propagation is off -- the equal strict filters mean both sides read the same base rows, so the merge reads that shared set once (with a broadened OR of the best-effort filters) rather than twice.
|
@LuciferYang this is ready for review now. cc @cloud-fan as well. |
|
It’s late here. I’ll come back with feedback tomorrow. |
| return None | ||
| } | ||
| val relOutByName = byName(relation.output) | ||
| val projectList = columns.flatMap(a => relOutByName.get(a.name)) |
There was a problem hiding this comment.
buildMergedScan builds projectList as columns.flatMap(a => relOutByName.get(a.name)), looking each column up by name in relation.output, which hands back the pre-pruning type. Fine for flat columns. For nested ones columns carries the pruned type like struct<b>, the projection substitutes struct<a,b,c>, and identifyRootFields uses att.dataType directly for a bare attribute, so the rebuilt scan reads the whole struct back and nested pruning stops working on the merge path. The worse part is the ordinal: the GetStructField above the scan was computed against the pruned schema, mapAttributes swaps the attribute but leaves the ordinal alone, and ordinal 0 of struct<a,b,c> is a where it used to mean b. So the read returns the wrong field. The cp side is not remapped by design (the comment on line 677), and the struct<b> type it holds no longer matches what the scan produces either. The post-check on lines 733-735 compares only the count and the names of scan.output, never dataType, so the widening passes silently. Two attributes sharing one exprId with different types should have been caught by hasUniqueExprIdsForOutput, but that walks each node's output and the mismatched reference sits in an intermediate Project. Both fixes I reached for first are too weak. Swapping the post-check for DataType.equalsStructurally misses the mixed case: cp selects s whole, np selects only s.b, columns is cp's struct<a,b,c>, the rebuilt type matches, the check passes, and np's ordinal 0 still reads a. Passing columns through as the projection does not work either, because unionAttrs is unioned by name (lines 652-655), so s.b against s.c keeps only cp's struct<b> and np then reads b. The smallest change I can defend is a pre-check in mergeable: decline the merge when two same-named attributes differ in dataType, or when any scan output type differs from relation.output, which is the signal that nested pruning is in play. Keeping the optimization would mean running StructType.merge over the union and re-deriving both sides' GetStructField ordinals against the merged schema, a much larger change. Could you also add a nested-column fixture? TestV2Table and InMemoryScanMergingPartitionFilterTable are both flat, so this path has no test coverage.
There was a problem hiding this comment.
Declined via the new subset-at-same-type gate in mergeable, which rejects exactly this nested-pruned case (66eeda37ef5); I also removed the by-name projection so the merged scan projects relation attributes directly (1d8548db39c). Added the do not merge DSv2 scans when a column is read at a nested-pruned type fixture.
| V2ScanRelationPushDown.rebuildScan(relation, projectList, conds).filter { scan => | ||
| // Every intended-strict filter must be fully enforced by the rebuilt scan (nothing above | ||
| // re-checks it), and the scan must produce exactly the requested union of columns. | ||
| val pushedSet = ExpressionSet(scan.pushedFilters) |
There was a problem hiding this comment.
mergeable checks hasMergeBlockingPushdown carefully on both input scans, but buildMergedScan validates only the strict filters, the column count and the column names on what comes back from the rebuild. It never re-checks the flag. Meanwhile rebuildScan runs the full apply, where all three buildScanWithPushed* rules set the flag to true and replace the exprIds, relying on an outer Project to map them back, and collectFirst throws exactly that wrapper away. Nothing reaches this today: the synthetic projection is bare attributes, the variant rule returns early because its only requested field targets VariantType and gets dropped, and ParquetScanBuilder, the single implementation of variant pushdown, produces a scan that does not implement SupportsScanMerging. It is still a quiet trap. Any future pushdown that fires on Project(attrs, Filter(cond, relation)) yields an accepted but wrong merged scan, and because the cp side is deliberately not remapped, the parent references dangle. Adding !scan.hasMergeBlockingPushdown to the filter on line 733 covers it.
There was a problem hiding this comment.
Done in fef130de72b — added scan.mergeableScan to the post-rebuild filter in tryBuildMergedDSv2Scan.
| * When a new pushdown capability adds state to [[ScanBuilderHolder]], classify it here (or, if it | ||
| * has its own build rule, mark that scan blocking there). | ||
| */ | ||
| private def hasBlockingPushdown(holder: ScanBuilderHolder): Boolean = |
There was a problem hiding this comment.
hasBlockingPushdown covers only the pruneColumns path. The aggregate, join and variant rules each hardcode true, and since the parameter defaults to false, forgetting it does not break the build. The tests happen to miss all three: MergeSubplansSuite.scala:2574 drives limit, offset, top-N and sample through the real pushdown, which is the helper path, and MergeSubplansSuite.scala:2051 fakes the flag with .copy() to exercise the consuming side. I deleted line 891 and everything still compiled. After that, two subqueries with a pushed aggregate fuse, the rebuild does not re-push the aggregate, and the plan expects aggregate columns while the scan hands back raw rows. The holder predicates those three rules match on are all readable from the helper, so folding the four terms into hasBlockingPushdown and calling it from every site makes the omission impossible. Separately, GroupBasedRowLevelOperationScanPlanning.scala:89 is a fourth production construction site that pushed the command condition and set neither field. It cannot reach the merge path today, since the same scan instance takes the identical branch first and row-level rewrites do not produce a scalar subquery or a non-grouping aggregate, but its builder comes from operation.newScanBuilder while rebuildScan calls table.asReadable.newScanBuilder, so it is not rebuildable by construction. Setting true there beats leaving the default.
There was a problem hiding this comment.
Flipped the field to default-safe mergeableScan (default false, granted only on the plain-scan path) instead of folding the terms, so aggregate/join/variant, GroupBasedRowLevelOperationScanPlanning, and any future site are non-mergeable by default (d83d2c01eaa). Added positive and negative classification tests.
| */ | ||
|
|
||
| package org.apache.spark.sql.catalyst.optimizer | ||
| package org.apache.spark.sql.execution.planmerging |
There was a problem hiding this comment.
This moves MergeSubplans from catalyst.optimizer to execution.planmerging, and Rule.ruleName is the fully qualified class name that the exclusion check compares as a string. The rule is in neither nonExcludableRules list. So anyone who turned the optimization off with spark.sql.optimizer.excludedRules=org.apache.spark.sql.catalyst.optimizer.MergeSubplans, possible since 4.2.0, finds the name no longer matches after upgrading. Unknown names are ignored without warning, so the optimization comes back on quietly. The repo already has a precedent for this exact situation: the DynamicJoinSelection rename at docs/sql-migration-guide.md:32, which even spells out "unknown names are silently ignored". Following that format in the 4.2 to 4.3 section should be enough. It is also worth noting in the PR description that the batch position did not change, since I checked and the rule was registered only in SparkOptimizer before the move as well. It reads like an ordering change otherwise.
There was a problem hiding this comment.
Added a 4.2→4.3 migration-guide note (mirroring DynamicJoinSelection, noting unknown excludedRules names are silently ignored) (f7d4414f412); also noted in the PR description that the batch position is unchanged (registered only in SparkOptimizer before and after the move).
| .booleanConf | ||
| .createWithDefault(false) | ||
|
|
||
| val MERGE_SUBPLANS_DSV2_SYMMETRIC_FILTER_PROPAGATION_ENABLED = buildConf( |
There was a problem hiding this comment.
The config doc says equal strict filters mean both sides read the same base rows, but the strict set comes out of filterNot(postScanFilterSet.contains), so a predicate the source hands back for a post-scan re-check never enters it, and line 115 of the same file says that overlap is normal (the parquet row group filter). A source that prunes on p and also returns p therefore reports an empty strict set on both sides, the sets compare equal, and the criterion holds while the property the doc claims does not. I first read this as IO amplification, but following it through, most cases recover: once Or(p='a', p='b') is rejected in the first pass it reaches createPartitionPredicates in the second, gets wrapped as a PartitionPredicate on the partition column and pushed, and the pruning comes back. What actually degrades is a source that is opt-in, partially pushes, cannot express OR, and has no second pass, which is a bounded regression. So the doc is the part that needs work: replace "read the same base rows" with what does hold, that the strict filters are equal and the pruning predicate is best effort with the enclosing Filter guaranteeing correctness. Whether the default should flip to false is your call; I would keep it true and fix the wording. I also withdraw the idea of requiring the post-scan filters to match. After early pushdown the Filter above the scan is the post-scan set, so that amounts to requiring the two Filter conditions to be equal, which lands in the identical branch and removes the feature.
There was a problem hiding this comment.
Reworded the doc (dropped "read the same base rows" → the strict filters are equal and the pruning is best-effort, with the enclosing Filter guaranteeing correctness) (d8e600ec16e). On the default I went the other way from your lean and shipped it false for the freeze (14995973b04); we can flip it on in a later release. Thanks for withdrawing the post-scan-match idea.
| * - `filterAboveScan` is set true ONLY by the `(Filter, Filter)` arm; the leaf defers building | ||
| * the merged DSv2 scan ONLY when it is true. A deferred result then flows leaf -> (pass-through | ||
| * Project arms, which propagate `deferredScan` unchanged) -> the `(Filter, Filter)` arm, which | ||
| * builds it once via `buildDeferredScan`. No other arm ever receives a deferred child: stacked |
There was a problem hiding this comment.
The reasoning in the invariant block, that stacked Filters are combined by CombineFilters before this rule runs, does not hold. CombineFilters itself does run earlier, inside operatorOptimizationRuleSet as part of super.defaultBatches. But PartitionPruning (SparkOptimizer.scala:62) and InjectRuntimeFilter (SparkOptimizer.scala:68) then each insert a Filter on top of an existing one, and both sit ahead of MergeSubplans (SparkOptimizer.scala:70), while the PushDownPredicates pass that would re-combine them waits for the next batch at SparkOptimizer.scala:73. So any query eligible for DPP or a bloom filter arrives here as Filter(DPP, Filter(cond, scan)). The mechanism is still safe (the inner arm consumes the deferral and returns deferredScan = None), just not for the reason the comment gives, and reading the recursion against that comment will mislead you. It also means the outer arm sees deferred = None, so the DSv2 exemption never fires and the merge is abandoned after a scan was already built, which makes DSv2 merging depend on how the Filters happen to stack. If that is intended, it is worth saying so in the comment.
There was a problem hiding this comment.
Good catch on both. Fixed the comment (d8e600ec16e) — the stacked Filters here are DPP / InjectRuntimeFilter runtime filters inserted after pushdown, deferral consumed at the innermost pair. And I fixed the behavior you flagged (the outer arm seeing deferred = None so the exemption never fired): a new dsv2Merged fact on TryMergeResult propagates the merge signal past the innermost pair, so a stacked outer (Filter, Filter) pair recognizes it and OR-widens too, gated on the DSv2-symmetric config (5fb710d3a95). Added a synthetic stacked-Filter test.
| * Context threaded DOWN through [[tryMergePlans]] recursion. | ||
| * | ||
| * Invariants: | ||
| * - `filterAboveScan` is set true ONLY by the `(Filter, Filter)` arm; the leaf defers building |
There was a problem hiding this comment.
The invariant comment on MergeContext is genuinely informative, and walking it point by point I agree nothing is reachable today: the Aggregate, Join and single-sided Filter arms all reset filterAboveScan to false, and by the time (Filter, Filter) returns the deferral has been consumed, so those _ discards can only be None. The problem is that the whole arrangement rests on a comment with no assertion behind it (and one of its three claims does not hold, see the other note), in a file that has grown from 647 to 932 lines with a single 294-line tryMergePlans. If someone later adds an arm that forwards context unchanged, which is exactly what the three Project arms do, the resulting plan keeps a placeholder bare DataSourceV2Relation; the read path in DataSourceV2Strategy only matches DataSourceV2ScanRelation, so planning fails outright. And since merge()'s backstop is to decline silently, a regression shows up as merges quietly not happening, which no test that does not assert scan counts will notice. One line in the existing DSv2 tests, assert(plan.collectFirst { case _: DataSourceV2Relation => }.isEmpty), is cheap insurance. The more thorough option is pulling the DSv2 merge into its own file or class; it touches the generic recursion in eight places and it is the part that drags sql/core imports into what was a pure catalyst utility, which is what forced the cross-module move in the first place.
There was a problem hiding this comment.
Added the cheap insurance — assertNoPlaceholderRelation (asserts no bare DataSourceV2Relation survives a merge) across the DSv2 tests (32f024130e2). Kept the DSv2 merge in the same file for now; the split into its own file is a reasonable follow-up if you feel strongly.
| // merge unsound. See DataSourceV2ScanRelation.pushedFilters. | ||
| val scanRelation = DataSourceV2ScanRelation(sHolder.relation, wrappedScan, output, | ||
| pushedFilters = remappedPushedFilters) | ||
| pushedFilters = sHolder.pushedFilterExpressions, |
There was a problem hiding this comment.
The impact of no longer remapping pushedFilters in pruneColumns is a bit wider than "internal" suggests. canonicalized is what sameResult compares, so two scans differing only in a filter that references a pruned-out column were equivalent before and are not now. Physical reuse is unaffected, since ReuseExchangeAndSubquery compares SparkPlan.canonicalized and BatchScanExec.doCanonicalize never touches this field. But CacheManager.lookupCachedData uses plan.sameResult(cd.plan), so a query that used to hit the cache may stop hitting it. The flip side is that the previous behavior treated two scans with different p values as equivalent, which was a wrong hit, so the direction is right. DataSourceV2Suite.scala:1340 already pins the canonicalization inequality; what I would add is a note in the PR description covering this layer rather than calling it internal. Whether the cache side needs an end-to-end test is your call. I originally wanted an assertion that two subqueries are no longer sameResult, but that pins the behavior at the wrong layer since physical reuse never consulted the field, so I withdraw it.
There was a problem hiding this comment.
Agreed — no code change here; I covered the CacheManager.sameResult layer in the PR description rather than calling it "internal" (physical reuse is unaffected, and the old behavior was a wrong hit, so the direction is right). DataSourceV2Suite:1340 already pins the canonicalization inequality. Thanks for withdrawing the sameResult assertion.
| // Reported partitioning/ordering (e.g. storage-partitioned join, reported sort) is not | ||
| // reconstructed by the rebuilt scan, so decline the merge rather than silently drop it. | ||
| // Merging these can be added as a follow-up. | ||
| np.keyGroupedPartitioning.isEmpty && cp.keyGroupedPartitioning.isEmpty && |
There was a problem hiding this comment.
mergeable gates on the reported keyGroupedPartitioning and ordering from both sides, on the grounds that the rebuild does not reconstruct them. I followed the rebuild path and confirmed rebuildScan runs only V2ScanRelationPushDown; V2ScanPartitioningAndOrdering is a separate entry in earlyScanPushDownRules and does not run again, so both fields are always None on the merged result. I suspected there was a case where both sides pruned the partition key, the union brought it back, and a reportable scan was lost, but I could not construct it: when the union lacks the key the reporting rule's own subsetOf(outputSet) check rejects it anyway, and when it has the key that side was already declined at the gate. So there is no defect here. I would just add a line to the tryMergeScanRelations comment saying rebuilt scans never carry reported partitioning or ordering, since the current wording reads as if this were only an input-side restriction.
There was a problem hiding this comment.
Added a line to the tryMergeScanRelations comment noting the rebuilt scan never carries reported partitioning or ordering (so it's not only an input-side restriction) (d8e600ec16e). Thanks for confirming there's no defect.
| * and do not appear as post-scan filters. These reference the relation's | ||
| * (pre-pruning) output, so they may reference columns pruned out of `output` | ||
| * (e.g. an unselected partition column the source enforces internally). This | ||
| * complete set is what lets [[org.apache.spark.sql.execution.planmerging. |
There was a problem hiding this comment.
The [[org.apache.spark.sql.execution.planmerging. / PlanMerger]] link in the pushedFilters doc seems split across two lines, so scaladoc reads it as target ...planmerging. with label PlanMerger and cannot resolve it. CI will not go red, since SparkBuild.scala:1673 excludes this file from sbt unidoc; the effect is limited to IDE navigation and rendering. Only two other places in all of src/main are written this way, so it is not an established habit either. Putting [[...PlanMerger]] on one line, reflowing the preceding sentence if needed, fixes it. Alternatively use the short name in backticks the way MergeSubplansReferences.scala:25-26 does.
There was a problem hiding this comment.
Fixed — put [[...PlanMerger]] on one line using the short backtick name, the way MergeSubplansReferences does (d8e600ec16e).
Replace `DataSourceV2ScanRelation.hasMergeBlockingPushdown` (default false = mergeable, set true at each non-reproducible build site) with a positive `mergeableScan` (default false = not mergeable). Only the plain column-pruning + filter pushdown path grants mergeability, and only when the scan carries nothing a rebuilt scan cannot reproduce. Every other build site -- the dedicated aggregate/join/variant rules, GroupBasedRowLevelOperationScanPlanning, and any site added later -- stays not-mergeable by default. This makes completeness structural instead of a hand-maintained checklist: the GroupBasedRowLevelOperationScanPlanning scan (which never set the old flag, so it defaulted to mergeable) is now correctly not-mergeable, and a future scan-relation build site no longer has to remember to opt out. Addresses LuciferYang's review: hasBlockingPushdown completeness + the untagged 4th construction site.
buildMergedScan rebuilds the merged scan via V2ScanRelationPushDown.rebuildScan, which re-runs the full pushdown, then verifies the strict filters and output columns -- but not that the rebuilt scan is itself mergeable. Add that check so a non-reproducible pushdown introduced by the rebuild declines the merge instead of producing an unsound plan. Defensive: today's Project-over-Filter rebuild input only triggers the plain (always-mergeable) path, so this re-validates the rebuild's output against the same gate applied to its inputs rather than trusting the rebuild. No reachable test -- the flag cannot be set false on a Project-over-Filter plan. Addresses LuciferYang's review: no mergeable re-check after rebuild.
…lumn buildMergedScan projects whole relation attributes, so the merged scan reads each column at the relation's full type. A side that read a struct/array/map column at a narrower type via nested schema pruning carries positional extractors (e.g. GetStructField ordinals) resolved against that narrow layout; remapping them onto the wider merged column would read the wrong field and return wrong results. Decline the merge unless each side reads every column at its relation's full type (matched by exprId, which is unique within a relation). This costs the optimization only for nested-pruned complex columns; widening the merged scan to the union of nested fields and remapping the ordinals is a possible follow-up. Addresses LuciferYang's review: nested-column pruning wrong results.
…t name PlanMerger related the two scans' columns by name (a `byName` helper feeding samePushedFilters, the column union, the np->cp mapping, and buildMergedScan's projection). Name matching is fragile: it assumes the relation output has unique column names. Replace it with a single positional relation correspondence -- canonically-equal relations list the table's columns in the same order, so pair them by position, and exprId then relates each scan's pruned output back to its own relation. buildMergedScan's projection and verification now match by exprId too. This is the idiomatic Catalyst identity and removes the name assumption from the merge's matching. Behavior is unchanged for valid inputs (the whole DSv2 pushdown pipeline, e.g. PushDownUtils.toOutputAttrs, still assumes unique top-level names, so duplicate names remain unsupported end to end); no new test -- validated by the existing suite preserving behavior. Addresses LuciferYang's review: byName duplicate-name assumption.
Addresses LuciferYang's documentation review comments, no behavior change: - MergeContext invariant: drop the wrong claim that CombineFilters combines stacked Filters so a (Filter, Filter)'s children are never Filters. They can be: PartitionPruning/InjectRuntimeFilter insert a Filter after CombineFilters runs and PushDownPredicates re-combines them only in a later batch. Explain why it stays safe (innermost pair consumes the deferral) and that a stacked outer pair with differing conditions declines intentionally. - dsv2SymmetricFilterPropagation config doc: drop "both sides read the same base rows" (a source that prunes on p and also returns it has an empty strict set); state the actual guarantee -- strict filters equal, differing filters are best-effort pruning, the enclosing Filter re-checks exactness. - Note that rebuilt scans never carry reported partitioning/ordering (not just an input-side restriction). - Put the split [[...PlanMerger]] / [[...V2ScanRelationPushDown]] scaladoc links on one line via short backtick names.
Ship the DSv2-specific symmetric filter-propagation exemption disabled by default for the feature freeze; it can be enabled in a later version. The gate (and any downstream handling of it) is inert while the config is off. Explicitly set by the tests that exercise it. No existing test depends on the default.
…n merge Addresses LuciferYang's review: the deferred-scan build relies on a comment-only invariant (a leaked deferred scan would leave an unbuilt bare DataSourceV2Relation that the read path cannot plan, and merge() would decline it silently). Add a cheap assertion in the DSv2 merge-success tests that the optimized plan contains no bare DataSourceV2Relation, so a regression fails loudly.
The DSv2-symmetric exemption was gated on `deferredScan.isDefined`, which is only set at the innermost (Filter, Filter) pair that builds the merged scan and is consumed there. A stacked outer Filter pair therefore saw `deferredScan = None` and declined a merge it could have made. Add TryMergeResult.dsv2Merged: set true when an (equal-strict) DSv2 scan merge occurs, propagated up through every arm (never consumed), and gate the exemption on it instead. This is correct and non-regressing: an outer filter that survives as a separate stacked Filter was never pushed to the scan (only the adjacent filter chain is; a stacked outer filter is either inserted after pushdown, like DPP/bloom, or blocked by an intermediate operator), so OR-widening it drops no scan pruning. The change is inert while dsv2SymmetricFilterPropagation is off (the default). Synthetic MergeSubplansSuite test; verified it fails on the old `deferredScan.isDefined` gate.
Addresses LuciferYang's review. MergeSubplans moved from org.apache.spark.sql.catalyst.optimizer to org.apache.spark.sql.execution.planmerging, changing its fully qualified name -- which is what spark.sql.optimizer.excludedRules matches on. A user who disabled the rule by its old FQCN would find the name silently ignored after upgrading, quietly re-enabling the optimization. Add a 4.2->4.3 migration-guide entry (following the DynamicJoinSelection precedent), also covering the older MergeScalarSubqueries -> MergeSubplans rename (4.2, whose note was missed). Also document, on the leading DSv2 earlyResult branch, why the merge is tried first even for identical scans under a Filter: the deferred rebuild re-pushes the enclosing Filter's condition as best-effort pruning that plain reuse would drop, so identical scans must not be short-circuited to reuse there.
…t a Scan marker Addresses LuciferYang's review. Replace the SupportsScanMerging Scan marker interface with a new TableCapability.SCAN_MERGING, and gate the merge on np.relation.table.capabilities() instead of np.scan.isInstanceOf[...]. The contract (rebuilding from a fresh ScanBuilder with the same options + pushed filters/pruned columns yields an equivalent scan) is a property of the table/source, not of a per-scan instance -- so the table is the right place to declare it. Putting it on the Scan had real gaps: a V2 source on the V1 scan fallback path (e.g. JDBC V2) has its scan wrapped in Spark's internal V1ScanWrapper, which cannot carry the marker, so such sources could never opt in (silently); and a builder returning different Scan subclasses would carry the marker inconsistently. A TableCapability is declared before build() and is independent of the scan class. Since SupportsScanMerging is new in this release, this avoids shipping a marker we would need to deprecate. Test fixtures: the InMemory source now returns the capability and drops the marker-attaching wrapper (its remaining NonReportingScan only strips reported partitioning, keeping keyGroupedPartitioning unset -- SPJ-across-merge is a follow-up); TestV2Table gains a supportsScanMerging flag driving its capabilities(), replacing the per-scan TestV2ScanNoMerge.
No behavior change. Cleanups from several self-review passes over the DSv2 scan-merge path in PlanMerger: - Inline single-use helpers into their callers: samePushedFilters and relationCorrespondence fold into tryMergeScanRelations (a shared npRelationMapping reused by the pushed-filter comparison and the column union); buildDeferredScan folds into tryBuildFilterDSv2ScanChild; mappedNPPushedFilters folds into the mergeable check. - Consolidate the per-column checks into one up-front gate in `mergeable` -- each side reads a subset of its relation's columns at the same type -- which subsumes the nested-pruned-type guard, the remap-resolves guard, and the projectList-size guard, so those are dropped (columns are relation attributes, projected and remapped directly). - Prefer AttributeSet/AttributeMap/outputSet over raw exprId comparisons and AttributeSet(plan.output); build the merged column union in np.output order. - Drop the redundant p.deterministic guard on the best-effort filter: SPARK-58207 already keeps non-deterministic filters from being pushed to a V2 source. - Name for clarity: methods tryBuildMergedDSv2Scan / tryBuildFilterDSv2ScanChild; the deferred-scan carrier DSv2DeferredScan with a dsv2DeferredScan field; params unionAttrs, strictFilters, bestEffortFilter. - Docs: fix the MergeContext invariant comment (it predated the dsv2Merged flag -- a stacked outer (Filter, Filter) pair OR-widens via dsv2Merged, sound because pushdown only pushes the scan-adjacent Filter); sweep "pruning" that named the best-effort argument to "best-effort filter" (leaving source-side pruning wording alone); document why np.pushedFilters is mapped through the full relation mapping and that the strict-filter guard is defensive. - Tests: strengthen the two DSv2 config-gate tests (single-Filter symmetric and the stacked outer-Filter pair) from fused-scan-count asserts to comparePlans, pinning the fused plan and the per-side AND(inner, outer) aggregate FILTER clauses.
|
Thanks for the very thorough second pass, @LuciferYang — this was really helpful. I've pushed an update (through b4b5207) addressing all of it and refreshed the PR description. Summary, calling out where I went a different way than the literal suggestion: Correctness
Opt-in placement: moved from the Stacked Filters: fixed the comment (these are DPP / Config default: reworded the doc (dropped "read the same base rows"). On the default I went the other way from your lean and shipped it Docs/notes: added the 4.2→4.3 migration-guide note (batch position unchanged, noted in the description); covered the The round-1 test-coverage items are in as well. PTAL when you get a chance. |
Resolved a conflict in docs/sql-migration-guide.md: kept the MergeSubplans package-move note alongside master's new 4.3 aggregation-config notes.
cloud-fan
left a comment
There was a problem hiding this comment.
0 blocking, 0 non-blocking, 2 nits.
The scan-merging safety gates and rebuild path are coherent and well covered; only two documentation nits remain.
Nits: 2 minor items (see inline comments).
Verification
I traced the opt-in from TableCapability.SCAN_MERGING through plain-scan mergeability classification, the MergeSubplans recursion, deferred filter handling, and the production pushdown rebuild. The rebuild is accepted only when it remains mergeable, fully re-enforces every strict filter, and returns exactly the requested union output; tests cover both config modes and the principal decline gates.
| import org.apache.spark.sql.catalyst.trees.TreePattern.{NO_GROUPING_AGGREGATE_REFERENCE, SCALAR_SUBQUERY_REFERENCE, TreePattern} | ||
| import org.apache.spark.sql.types.DataType | ||
|
|
||
| // The temporal reference placeholders below are produced by the `MergeSubplans` rule (now in |
There was a problem hiding this comment.
These placeholders are temporary, not temporal (time-related). Please use temporary here and in the two class Scaladocs below.
There was a problem hiding this comment.
Thanks @cloud-fan! Fixed in 466b6b69b12.
| * the subquery is merged. | ||
| * @param mergedPlanIndex The index of the merged plan in the `PlanMerger`. | ||
| * @param outputIndex The index of the output attribute of the merged plan. | ||
| * @param dataType The dataType of original scalar subquery. |
There was a problem hiding this comment.
Please phrase this as “The data type of the original scalar subquery.” The output parameter below has the same missing-article issue: “The output of the original aggregate.”
|
@LuciferYang, I wonder if you could do another round of review today. If you agree with the changes this feature can make it into 4.3. No worries if not, I will adjust the new configs to 5.0 then. |
I will take a look at it today. |
What changes were proposed in this pull request?
This adds a generic, Spark-side merge of equivalent DataSource V2 scans in the
MergeSubplansrule. When two subplans read the same V2 table, their scans can be fused into a single scan. V1 file sources already get this (viaFileSourceStrategy), but every V2 scan was built independently, so the same table could be read many times.A source opts in with a new
TableCapability.SCAN_MERGINGcapability. Making it a table capability (rather than a marker onScan) lets Spark decide beforebuild(), and lets a V1-fallback source — whoseScanSpark wraps in an internalV1ScanWrapper— still opt in.PlanMergerfuses twoDataSourceV2ScanRelations only when all of the following hold:SCAN_MERGINGcapability;mergeableScanflag onDataSourceV2ScanRelation: it defaults tofalseand is granted only on the plain-scan path, so any other current or future build site is non-mergeable unless it explicitly opts in;The two scans may still differ in projected columns (the merged scan reads the union) and in best-effort / post-scan filters (OR-widened above the merged scan by symmetric filter propagation).
When these hold,
PlanMergerrebuilds the merged scan by driving the realV2ScanRelationPushDown(through a newrebuildScanentry point) over the union of columns and the (equal) strict filters, and verifies each strict filter comes back fully enforced and that the rebuilt scan is itself mergeable. Reusing the production pushdown end to end means the merged scan goes through the same filter translation, column pruning and iterativePartitionPredicatesecond pass. When the merged scan sits under aFilter, the build is deferred to that Filter so it's built once with the strict filters plus the best-effort OR pruning together. Mechanically, a smallMergeContextthreads down through the recursion, andTryMergeResultgains adsv2DeferredScanfield carrying the pending scan's union columns and strict filters back up to thatFilter. A separatedsv2Mergedfact propagates the "a DSv2 merge happened below" signal all the way up (past the innermostFilterthat builds the scan), so a stacked outerFilter— e.g. a DPP or runtime-bloomFilterthatPartitionPruning/InjectRuntimeFilterinsert above the scan-adjacent one after pushdown — still recognizes the merge and can OR-widen.Because a DSv2 merge's strict filters are equal on both sides, the best-effort OR-widen (which only broadens the enclosing
Filtercondition offered to the source for data skipping — theFilterabove still guarantees correctness) is allowed under a new DSv2-specific configspark.sql.optimizer.mergeSubplans.filterPropagation.dsv2SymmetricFilterPropagation.enabledeven when the general symmetric propagation is off. It ships defaultfalsefor now; it can be flipped on in a later release once more sources have exercised the path.To make the merge sound, this PR also completes
DataSourceV2ScanRelation.pushedFilters, which previously dropped fully-pushed filters referencing a column pruned out of the scan output (e.g. an unselected partition column the source enforces internally); the merge compares and re-enforces this set, so it must be complete. This widens the field's effect slightly beyond the merge:pushedFiltersparticipates incanonicalized, which backssameResult, so two scans differing only in a filter over a pruned-out column are no longersameResult. Physical reuse is unaffected (BatchScanExec.doCanonicalizedoesn't consult the field), butCacheManager.lookupCachedDatausessameResult, so such a query may correctly stop hitting the cache — the previous behavior treated the two as equivalent, which was a wrong hit.DataSourceV2Suitepins this canonicalization inequality. Non-deterministic filters cannot reach the merge: they aren't pushed to V2 sources (SPARK-58207).This is the generic alternative to #56264, which added scan merging per file format; here any V2 source gets it with a single capability.
Note on structure: the first commit is a pure move of
MergeSubplans/PlanMerger(and suites) fromsql/catalystto a newsql/coreexecution.planmergingpackage — required because the merge now calls sql/core-only pushdown — and can be split into its own PR. The move does not change the rule's batch position (it was and is registered only inSparkOptimizer); because the rule's fully-qualified class name changed, a 4.2→4.3 migration-guide note is added (mirroring theDynamicJoinSelectionrename), sincespark.sql.optimizer.excludedRulesmatches by that name. The remaining commits are the feature, the build-deferral, theTableCapabilityopt-in, the outer-Filterdsv2Mergedfact, and several self-review passes.Why are the changes needed?
Under DataSource V2, subplans that read the same table each build their own scan, so the table is scanned once per subplan. The motivating case is TPC-DS q9, whose 15 scalar subqueries over
store_salescollapse to a single scan under V1 but read the table 15 times under V2. This closes that gap generically for any V2 source.Does this PR introduce any user-facing change?
No behavior change for existing sources: the merge is opt-in through the new
TableCapability.SCAN_MERGINGcapability, which no built-in source declares, so plans and results are unchanged. This PR adds one new SQL config (spark.sql.optimizer.mergeSubplans.filterPropagation.dsv2SymmetricFilterPropagation.enabled, defaultfalse), which only affects sources that opt intoSCAN_MERGING.How was this patch tested?
New tests:
MergeSubplansSuite(plan tests,comparePlanswhere the merge changes the plan): column-union merge; differing-filter OR-widen (2- and 3-way, including a reuse-branch case); a stacked outer-Filterpair fusing via the propagateddsv2Mergedfact; identical-filter re-push; equal-strict re-push (2- and 3-way); mixed equal-strict + differing-post-scan; the DSv2 symmetric config gate (merges when it is on and the general one is off, not when both are off); the decline gates (noSCAN_MERGINGcapability, a merge-blocking pushdown, a column read at a nested-pruned type, reported key-grouped partitioning/ordering, a strict filter not re-enforced on rebuild); amergeableScanclassification check driving a pushed limit/offset/top-N/sample through the real pushdown; a non-deterministic pruning-predicate drop; a negative canonicalization check; and an invariant assertion that no placeholderDataSourceV2Relationsurvives a merge.DSv2PlanMergingSuite(end-to-end with a real session): two scalar subqueries whose partition filter is strict only via the iterative second pass fuse into one scan (checkAnswerverifies correctness); two subqueries with different partition filters are not fused; and two subqueries with differing best-effort filters OR-widen into one scan.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8)