[SPARK-59261][SQL] Memoize outputPartitioning on the projection and SPJ grouping nodes - #58542
Open
peter-toth wants to merge 1 commit into
Open
Conversation
…PJ grouping nodes
### What changes were proposed in this pull request?
`PartitioningPreservingUnaryExecNode.outputPartitioning` and `GroupPartitionsExec.outputPartitioning` become `lazy val`s. The projection body also binds `child.outputPartitioning` to a local, because it read it twice.
### Why are the changes needed?
Both recompute a non-trivial value on every call, and the planner asks a node for its partitioning many times.
Counted by instrumenting the two bodies and running `KeyGroupedPartitioningSuite`:
PartitioningPreservingUnaryExecNode 23,512 -> 3,663
GroupPartitionsExec 11,110 -> 1,326
Both figures are this PR against its own base, with the two changes applied together.
`PartitioningPreservingUnaryExecNode` is the expensive one. For every partitioning it projects each expression through the output aliases and deduplicates the results by `canonicalized`. For a `KeyedPartitioning` child it also allocates an `ExpressionSet` per key position and cross-products the per-position alternatives through `LazyList`s. It is the biggest single caller of the V2 scan's `outputPartitioning` too. On the same instrumented run, 19,105 of that method's 33,051 reads arrive through it, mostly as `Project -> Filter -> scan`.
`GroupPartitionsExec` rebuilds every `KeyedPartitioning` in the child's partitioning through `p.transform`, on top of the `grouping` it has already memoized.
### Why memoizing is safe
Neither body reads a live config, so neither freezes one. `PartitioningPreservingUnaryExecNode` reads `child.outputPartitioning`, `outputExpressions` and `aliasCandidateLimit`, and that last one is already a `protected val` evaluated at node construction. `GroupPartitionsExec.outputPartitioning` reads `child.outputPartitioning` and its own `grouping`, and no config at all.
The planner already treats `outputPartitioning` as a property of the node rather than a question to re-ask. `ValidateRequirements` reads one child's partitioning twice and compares specs built from the two reads. `EnsureRequirements` reads it repeatedly and threads the results between the reads, and at `EnsureRequirements.scala:335` it is the pruning predicate of a `multiTransformDownWithPruning`, re-evaluated per generated alternative.
A child's partitioning is not fixed in every case. `InMemoryTableScanExec.outputPartitioning` reports `UnknownPartitioning(0)` while its AQE cached plan is not final, and sharpens once it is. Memoizing here is still right, for two reasons. In the executed plan the ancestor instances are fresh, because `CollapseCodegenStages` and `ApplyColumnarRulesAndInsertTransitions` insert nodes and every ancestor above an insertion is copied, so the memo is taken no earlier than the old `def` was first called. Where an instance does survive such a change, the pinned answer is the one `EnsureRequirements` planned against, which is the answer a later reader should see.
The memo retains one `Partitioning` per node for the plan's lifetime, holding up to `aliasCandidateLimit` alternatives. That is a deliberate trade against the recomputations counted above. An identity key projection retains nothing new, because `KeyedPartitioning.project` returns `this` when it drops no position.
### Why a plain `lazy val`
`BroadcastHashJoinExec`, `AQEShuffleReadExec` and `FileSourceScanExec` already override `outputPartitioning` as a plain `lazy val`, and all three postdate SPARK-50705, which added `BestEffortLazyVal` for `QueryPlan` members that a tree walk can reach from two directions at once. That hazard needs two lock orders. Both bodies here only descend into `child`, and a physical plan node holds no parent pointer that any `outputPartitioning` implementation follows, so every lock order is parent then child.
### Why not the `outputOrdering` twins
`AliasAwareQueryOutputOrdering.outputOrdering` runs the same alias machinery per call, but it is declared `final` in catalyst and is shared with logical plans, where the optimizer creates and discards nodes constantly. That is a wider change than this one.
`GroupPartitionsExec.outputOrdering` reads `conf.v2BucketingPreserveKeyOrderingOnCoalesceEnabled`, so memoizing it would change when that config is read. That is a separate question.
### Does this PR introduce _any_ user-facing change?
No.
`PartitioningPreservingUnaryExecNode.outputPartitioning` was `final override def` and becomes `final override lazy val`. Scala forbids a `def` overriding a `lazy val`, but this member is `final`, so no subclass could have overridden it either way.
`GroupPartitionsExec.outputPartitioning` was not final. An out-of-tree subclass overriding it with a `def` would need to become a `lazy val`. Binary compatibility is unaffected.
### How was this patch tested?
Existing suites, since the values are unchanged. `KeyGroupedPartitioningSuite`, `GroupPartitionsExecSuite`, `ProjectedOrderingAndPartitioningSuite`, `EnsureRequirementsSuite` and `PlannerSuite`, 311 tests.
One of them covers the config claim directly. `ProjectedOrderingAndPartitioningSuite`'s "SPARK-46367: narrowing projection with duplicate keys requires allowKeysSubsetOfPartitionKeys to satisfy ClusteredDistribution" reads the same `ProjectExec` instance with `V2_BUCKETING_ALLOW_KEYS_SUBSET_OF_PARTITION_KEYS` off and then on, and asserts a different answer each time. It passes with the partitioning memoized, because that config is read by `mayGroupToSatisfy` on the returned value rather than by the body.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
PartitioningPreservingUnaryExecNode.outputPartitioningandGroupPartitionsExec.outputPartitioningbecomelazy vals. The projection body also bindschild.outputPartitioningto a local, because it read it twice.Why are the changes needed?
Both recompute a non-trivial value on every call, and the planner asks a node for its partitioning many times.
Counted by instrumenting the two bodies and running
KeyGroupedPartitioningSuite:Both figures are this PR against its own base, with the two changes applied together.
PartitioningPreservingUnaryExecNodeis the expensive one. For every partitioning it projects each expression through the output aliases and deduplicates the results bycanonicalized. For aKeyedPartitioningchild it also allocates anExpressionSetper key position and cross-products the per-position alternatives throughLazyLists. It is the biggest single caller of the V2 scan'soutputPartitioningtoo. On the same instrumented run, 19,105 of that method's 33,051 reads arrive through it, mostly asProject -> Filter -> scan.GroupPartitionsExecrebuilds everyKeyedPartitioningin the child's partitioning throughp.transform, on top of thegroupingit has already memoized.Why memoizing is safe
Neither body reads a live config, so neither freezes one.
PartitioningPreservingUnaryExecNodereadschild.outputPartitioning,outputExpressionsandaliasCandidateLimit, and that last one is already aprotected valevaluated at node construction.GroupPartitionsExec.outputPartitioningreadschild.outputPartitioningand its owngrouping, and no config at all.The planner already treats
outputPartitioningas a property of the node rather than a question to re-ask.ValidateRequirementsreads one child's partitioning twice and compares specs built from the two reads.EnsureRequirementsreads it repeatedly and threads the results between the reads, and atEnsureRequirements.scala:335it is the pruning predicate of amultiTransformDownWithPruning, re-evaluated per generated alternative.A child's partitioning is not fixed in every case.
InMemoryTableScanExec.outputPartitioningreportsUnknownPartitioning(0)while its AQE cached plan is not final, and sharpens once it is. Memoizing here is still right, for two reasons. In the executed plan the ancestor instances are fresh, becauseCollapseCodegenStagesandApplyColumnarRulesAndInsertTransitionsinsert nodes and every ancestor above an insertion is copied, so the memo is taken no earlier than the olddefwas first called. Where an instance does survive such a change, the pinned answer is the oneEnsureRequirementsplanned against, which is the answer a later reader should see.The memo retains one
Partitioningper node for the plan's lifetime, holding up toaliasCandidateLimitalternatives. That is a deliberate trade against the recomputations counted above. An identity key projection retains nothing new, becauseKeyedPartitioning.projectreturnsthiswhen it drops no position.Why a plain
lazy valBroadcastHashJoinExec,AQEShuffleReadExecandFileSourceScanExecalready overrideoutputPartitioningas a plainlazy val, and all three postdate SPARK-50705, which addedBestEffortLazyValforQueryPlanmembers that a tree walk can reach from two directions at once. That hazard needs two lock orders. Both bodies here only descend intochild, and a physical plan node holds no parent pointer that anyoutputPartitioningimplementation follows, so every lock order is parent then child.Why not the
outputOrderingtwinsAliasAwareQueryOutputOrdering.outputOrderingruns the same alias machinery per call, but it is declaredfinalin catalyst and is shared with logical plans, where the optimizer creates and discards nodes constantly. That is a wider change than this one.GroupPartitionsExec.outputOrderingreadsconf.v2BucketingPreserveKeyOrderingOnCoalesceEnabled, so memoizing it would change when that config is read. That is a separate question.Does this PR introduce any user-facing change?
No.
PartitioningPreservingUnaryExecNode.outputPartitioningwasfinal override defand becomesfinal override lazy val. Scala forbids adefoverriding alazy val, but this member isfinal, so no subclass could have overridden it either way.GroupPartitionsExec.outputPartitioningwas not final. An out-of-tree subclass overriding it with adefwould need to become alazy val. Binary compatibility is unaffected.How was this patch tested?
Existing suites, since the values are unchanged.
KeyGroupedPartitioningSuite,GroupPartitionsExecSuite,ProjectedOrderingAndPartitioningSuite,EnsureRequirementsSuiteandPlannerSuite, 311 tests.One of them covers the config claim directly.
ProjectedOrderingAndPartitioningSuite's "SPARK-46367: narrowing projection with duplicate keys requires allowKeysSubsetOfPartitionKeys to satisfy ClusteredDistribution" reads the sameProjectExecinstance withV2_BUCKETING_ALLOW_KEYS_SUBSET_OF_PARTITION_KEYSoff and then on, and asserts a different answer each time. It passes with the partitioning memoized, because that config is read bymayGroupToSatisfyon the returned value rather than by the body.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)