perf: memoize Comet-subtree check in EliminateRedundantTransitions - #5213
perf: memoize Comet-subtree check in EliminateRedundantTransitions#5213andygrove wants to merge 1 commit into
Conversation
hasCometNativeChild scans the subtree below every ColumnarToRowExec the rule visits, so stacked transitions rescan the same nodes and the combined cost is quadratic in plan size. The scan stops at the first Comet operator it finds, which keeps the common case cheap, but a stack of transitions over a Comet-free subtree walks all of it every time. Memoize the scan for the duration of a single rule invocation. On a 6403-node chain of transitions over a vanilla Parquet scan this cuts the nodes visited from 10.2M to 6402 and the rule from 264 ms to 0.9 ms. The memo is keyed on identity because SparkPlan equality and hash are themselves subtree walks, and it is a local of _apply because the rule instance lives for the whole session and must not retain plans. Part of apache#5199.
|
|
||
| import org.apache.comet.CometConf | ||
|
|
||
| class EliminateRedundantTransitionsSuite extends CometTestBase { |
There was a problem hiding this comment.
[P2] Register the new suite in both CI workflow matrices
Could you add org.apache.comet.rules.EliminateRedundantTransitionsSuite to the suite lists in pr_build_linux.yml and pr_build_macos.yml? dev/ci/check-suites.py requires every non-exempt suite in both workflows. Running it against this exact head exits 255 with Suite not found in workflow .github/workflows/pr_build_linux.yml: org.apache.comet.rules.EliminateRedundantTransitionsSuite. The live Preflight job fails for the same reason, which leaves the main Linux/macOS builds and Spark SQL jobs skipped, so neither the new regression test nor the main build matrix runs.
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
The existing [P2] missing CI suite registration finding still applies to this head and needs to be addressed before merge. This approval does not mark that finding resolved.
Preflight still fails because the new suite is absent from the Linux and macOS workflow matrices. The main builds and Spark SQL jobs remain skipped.
Withdrawing this approval because I approved the wrong PR in error. The existing P1/P2 findings remain unresolved and still need to be addressed before merge.
2 participants
Which issue does this PR close?
Part of #5199 (item 5:
EliminateRedundantTransitions.hasCometNativeChildis quadratic).Rationale for this change
EliminateRedundantTransitionsaskshasCometNativeChildwhether the subtree under aColumnarToRowExeccontains a Comet operator, and that check is aTreeNode.existsscan of thesubtree. Because the rule runs as a
transformUp, the scan is repeated at everyColumnarToRowExecand each one re-walks everything below it, so stacked transitions rescan thesame nodes and the combined cost is quadratic in plan size. The rule runs on the driver for every
query, and again for every query stage under AQE.
The scan stops at the first Comet operator it finds, so the quadratic behavior only shows up over
a Comet-free subtree. That case is real: the rule runs on every plan, including one where Comet
took nothing but Spark still inserted columnar transitions.
Measured with a throwaway probe that applies the rule to a chain of
ColumnarToRowExec/RowToColumnarExecpairs over a vanilla (non-Comet) Parquet scan, counting the nodes visited bythe subtree scan and timing the rule (mean of 20 applications after warm-up, Spark 4.1 / JDK 17):
When the subtree does contain a Comet operator both versions are already linear and measure the
same: the rule rewrites the innermost
ColumnarToRowExecinto aCometColumnarToRowExec, whichis itself a
CometPlan, so every ancestor's scan short-circuits a node or two down.What changes are included in this PR?
Memoizes the subtree scan for the duration of a single rule invocation.
SparkPlanequality and hash are themselves subtreewalks and a
HashMapwould reintroduce the quadratic cost in the lookup._applyrather than a field, because the rule instance lives for the wholesession and must not retain plans.
transformUppreserves the identity of subtrees it does not rewrite, so a rebuilt node stillhits the memo one level down.
Behavior is unchanged.
containsCometPlanreproducesop.exists(_.isInstanceOf[CometPlan])exactly, and
QueryStageExec/ReusedExchangeExecare still unwrapped only at the root of thechecked subtree, as before.
How are these changes tested?
Existing coverage: every Comet query test exercises this rule, and
RevertNativeForTransitionHeavyStagesSuite/CometMapInBatchSuiteassert on its outputdirectly.
Added
EliminateRedundantTransitionsSuite, which puts a Comet columnar branch and a Spark-onlycolumnar branch under one plan and asserts the rule rewrites only the Comet one. A memo that
leaked a result between branches would fail it. The probe used for the numbers above was
throwaway and is not included.