[SPARK-60749][SQL] Do not reorder associative operators across different evaluation modes by joelrobin18 · Pull Request #59954 · apache/spark

joelrobin18 · 2026-10-30T08:37:45Z

What changes were proposed in this pull request?

ReorderAssociativeOperator flattens chains of integral Add / Multiply to fold their literals. Its flattenAdd / flattenMultiply helpers matched Add(l, r, _) / Multiply(l, r, _) without looking at the evaluation mode, and the rule then rebuilt the whole chain with the root operator's mode. Since SPARK-40222, numeric try_add and try_multiply are replaced by TRY-mode Add / Multiply, so combining them with regular arithmetic changed their semantics.

This PR:

Flattens only operators whose evaluation mode equals the root's; an operator in a different mode is kept as a single operand. Only the mode is compared, so operators that differ only in allowDecimalPrecisionLoss (which does not affect integral types) still fold.
Keeps TRY chains (try_add / try_multiply) in their written grouping, apart from two rewrites that cannot change the result. A TRY step returns NULL exactly when its own operation overflows, so regrouping a TRY chain can change which step overflows. The two exact rewrites are:
- A step that adds 0 or multiplies by 1 never overflows, so it is replaced by its other operand. For example, try_add(a, 0) becomes a.
- Literals on the path from the top of the chain through steps with one literal operand are folded into one step above the sub-expression where that path ends, if they are not NULL, all move away from zero (same sign for try_add, at least 1 for try_multiply), and fold to a value that fits the type. For example, try_add(try_add(try_add(a, b), 1), 2) becomes try_add(try_add(a, b), 3), while try_add(try_add(a, 5), -3) and try_add(try_add(a, 1), b) stay as written.
Makes Add / Multiply canonicalization keep a nested step as one operand when regrouping it can change the result: a TRY step, or a non-ANSI decimal step whose result type is bounded by the maximum precision, which returns NULL on its own overflow or rounds. Each step's two operands are still ordered. Previously such differently grouped chains were semanticEquals, so subexpression elimination could evaluate one chain for both, and with ANSI off a select-list decimal chain could be matched to a differently grouped GROUP BY chain.

This also resolves the TODO in the rule about not reordering operators with different failOnError flags.

Why are the changes needed?

The optimizer changes query results when try_add / try_multiply over non-foldable inputs is combined with regular arithmetic, or chained with itself:

CREATE TABLE t1 (x INT, y INT) USING parquet;
INSERT INTO t1 VALUES (2147483647, 1);

SELECT try_add(x, y) + 1 FROM t1;              -- ARITHMETIC_OVERFLOW, expected NULL
SELECT try_multiply(x, 2) * 2 FROM t1;         -- ARITHMETIC_OVERFLOW, expected NULL
SELECT try_add(x + y, 1) FROM t1;              -- NULL, expected ARITHMETIC_OVERFLOW from `x + y` (SPARK-40222)
SELECT try_add(try_add(x, -5), y) FROM t1;     -- NULL, expected 2147483643

For the first query the analyzed plan is (try_add(x, y) + 1), but the optimized plan is ((x + y) + 1) with the inner add evaluated in ANSI mode. For the TRY chain, the rule regrouped try_add(try_add(x, -5), y) into try_add(try_add(x, y), -5), where x + y overflows. Literal inputs are not affected because ConstantFolding evaluates them before this rule runs.

Canonicalization had the same blind spot. With ANSI off and DECIMAL(38, 0) columns holding a = 99999999999999999999999999999999999999, b = 1, c = -5, SELECT (a + b) + c, (a + c) + b returned NULL twice, and SELECT (a + c) + b ... GROUP BY (a + b) + c returned the grouping value NULL.

Does this PR introduce any user-facing change?

Yes, it fixes a bug. With non-foldable inputs:

try_add / try_multiply nested in a regular + / * return NULL on overflow instead of failing, and a regular + / * nested in try_add / try_multiply raises its overflow instead of returning NULL.
A chain of try_add / try_multiply returns NULL exactly when one of its written steps overflows.
Differently grouped TRY chains, and differently grouped non-ANSI decimal chains with a step that can overflow or round, are no longer the same expression, for example for subexpression elimination. With ANSI off, a query that selects such a regrouped copy of a GROUP BY decimal chain now fails analysis with MISSING_AGGREGATION instead of returning the grouping value.

TRY chains outside the exact rewrites, such as try_add(try_add(a, 5), -3), are no longer folded, so their optimized plans keep one more operation. ANSI and LEGACY integral chains, and ANSI decimal chains, fold and canonicalize as before. The SQL and Structured Streaming migration guides describe the changes. The incorrect behavior exists since Spark 3.4.0 (SPARK-40222).

How was this patch tested?

ReorderAssociativeOperatorSuite, under both ANSI settings: TRY-mode operators inside regular ones and vice versa; TRY chains that keep their written form (mixed-sign literals, literals between non-literal operands, multipliers of -1 or 0, a typed NULL literal, and folded literals that overflow INT, BIGINT, SMALLINT or TINYINT); TRY chains that drop identity steps or fold literals exactly, including above a step with two non-literal operands, a BIGINT fold outside the INT range, and a filter try_add(a, 0) > 5 that becomes a > 5; and operators that differ only in allowDecimalPrecisionLoss.
CanonicalizeSuite, with and without the multi-operand canonical form: differently grouped TRY chains (including TryAdd / TryMultiply) and non-ANSI DECIMAL(38, 0) chains are not semantically equal; operand swaps within a step still are; non-ANSI decimal chains whose steps fit the maximum precision exactly, ANSI chains, and integral chains are unchanged.
try_arithmetic.sql and decimalArithmeticOperations.sql (with and without ANSI through their nonansi variants): nested and top-level TRY chains, exact folding over rows that overflow at different steps, projections that select two groupings of a TRY or decimal chain, and GROUP BY with a regrouped decimal chain. With the base branch's Add / Multiply canonicalization, the projections return the same value twice and the non-ANSI GROUP BY query passes analysis, so these golden results fail.
Ran all suites in org.apache.spark.sql.catalyst.optimizer plus ArithmeticExpressionSuite, CanonicalizeSuite, DecimalExpressionSuite, ExpressionSetSuite, SubexpressionEliminationSuite and TryEvalSuite under both ANSI settings (1789 tests each), the full SQLQueryTestSuite (806 tests), the TPC-DS plan stability suites with ANSI off (322 tests, no plan changes), and catalyst and sql scalastyle. All pass; only the try_arithmetic.sql and decimalArithmeticOperations.sql golden results change.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Isaac (Claude Opus 5.5)

This pull request and its description were written by Isaac.

…n modes ReorderAssociativeOperator flattened nested Add/Multiply regardless of their evaluation mode and rebuilt the chain in the root's mode. Numeric try_add and try_multiply are TRY-mode Add/Multiply since SPARK-40222, so `try_add(x, y) + 1` over columns threw ARITHMETIC_OVERFLOW instead of returning NULL, and `try_add(x + y, 1)` returned NULL instead of raising the overflow of `x + y`. Only flatten operators evaluated in the same mode as the root. Co-authored-by: Isaac <[email protected]>

Cover ANSI-mode operators under a LEGACY root, LEGACY chains under an ANSI root, and TRY-mode operators under a LEGACY root with ANSI disabled. Co-authored-by: Isaac <[email protected]>

cloud-fan

Review summary

This PR fixes a real correctness bug. On master, ReorderAssociativeOperator rebuilt try_add/try_multiply operands in the enclosing operator's mode, and the reverse, and the new guard now keeps operands of a different mode as separate units. Two issues remain in the same rule. First, TRY-mode chains are still regrouped, and their literals folded, when the rule reaches them as their own root. The new guard now sends every TRY chain nested in a regular + or * down that path. For some inputs, a correct value or an overflow error becomes a silent NULL, and top-level TRY chains keep the wrong results they already had on master. Leaving TRY-mode roots unchanged fixes this. Second, the guard compares the whole evaluation context rather than the eval mode. As a result, integral chains whose operators differ only in spark.sql.decimalOperations.allowPrecisionLoss lose folding that was safe on master; results are unchanged. The two fixes are independent and do not conflict.

Findings

2 total: 0 P0, 1 P1, 1 P2, 0 P3.

Blocking (P1)

Nested try_add/try_multiply chains are still regrouped, now returning wrong NULLs — sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala:282 — see inline.

Non-blocking (P2)

Guard compares the whole eval context, not just the mode, and loses safe folding — sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala:282 — see inline.

A TRY-mode Add or Multiply returns NULL only when its own step overflows, so regrouping a TRY chain or folding its literals changes which rows are NULL. Exclude TRY-mode roots from ReorderAssociativeOperator so these chains keep their written grouping. Compare only the eval mode when flattening, so operators that differ only in allowDecimalPrecisionLoss still fold. Add optimizer tests and try_arithmetic SQL coverage for top-level and nested TRY chains. Co-authored-by: Isaac <[email protected]>

TRY arithmetic is commutative but not associative, yet Add and Multiply canonicalization flattened same-mode TRY chains, so a regrouped TRY chain was semanticEquals to the written one. A SELECT expression could then be matched to a differently grouped GROUP BY chain and return its NULL. Canonicalize a TRY-mode Add or Multiply by ordering only its two direct operands. ReorderAssociativeOperator now folds a TRY chain when that cannot change the result: one non-literal operand, literals that all move away from zero (same sign for Add, at least 1 for Multiply), and a folded literal that fits the type. Every other TRY chain keeps its written form. Co-authored-by: Isaac <[email protected]>

SQLQueryTestSuite reads results/nonansi/try_arithmetic.sql.out.java21 instead of the plain file on Java 21, which CI now uses. Add the new try_add and try_multiply chain queries to it; it still differs from the plain file only in the Java 21 double formatting. Co-authored-by: Isaac <[email protected]>

Co-authored-by: Isaac <[email protected]>

cloud-fan

Review summary

Both issues from the previous review are fixed. TRY-mode roots no longer take the regrouping path, and the one TRY fold that remains (a single non-literal operand, literals that all move away from zero, and a folded literal that fits the type) is exact. The flatten guard now compares only the eval mode, so operators that differ only in allowDecimalPrecisionLoss fold again. The new optimizer and SQL tests cover the cases the earlier review listed.

Excluding TRY roots, as the earlier review suggested, went further than correctness needs: it also dropped exact rewrites master applied to TRY chains, namely removing try_add(x, 0) and try_multiply(x, 1) steps and folding literal tails such as try_add(try_add(try_add(a, b), 1), 2). Those inputs were already correct, so they now pay extra TRY operations per row and can lose filter pushdown. Separately, the new TRY-only nesting in Add and Multiply canonicalization leaves non-ANSI decimal chains, whose steps also return NULL on their own overflow, regrouped as on master. The other findings are small: the rule's scaladoc still promises to fold all constants into one, foldTryChain's non-INT fit checks and NULL guard have no tests, and the TRY GROUP BY queries and TryAdd/TryMultiply assertions already pass on master, so no SQL test can fail if the canonicalization change regresses.

One question for the author, about documenting the upgrade impact, follows under Decision challenges.

Cross-cutting observations

The optimizer rule and Add and Multiply canonicalization both decide whether regrouping is safe from the evaluation mode alone. Safety actually depends on whether a step can overflow and on where the literals sit. As a result, the rule's TRY path refuses some rewrites that cannot change results, and canonicalization still regroups non-ANSI decimal steps that return NULL on their own overflow.

For both findings, a regrouping is safe exactly when no step it moves can overflow or round, so each fix and its tests can be checked against that one condition.

Findings

5 total: 0 P0, 1 P1, 1 P2, 3 P3.

Blocking (P1)

Excluding TRY roots drops exact rewrites master applied to try_add/try_multiply — sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala:422 — see inline.

Non-blocking (P2)

Non-ANSI decimal chains still regroup in canonicalization and match across groupings — sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:486 — see inline.

Nit (P3)

ReorderAssociativeOperator scaladoc still says it folds all constants into one — sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala:373 — see inline.
foldTryChain's BIGINT, SMALLINT and TINYINT fit checks and NULL guard have no tests — sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala:427 — see inline.
TRY GROUP BY tests and TryAdd/TryMultiply assertions already pass on master — sql/core/src/test/resources/sql-tests/inputs/try_arithmetic.sql:132 — see inline.

Re-review status

Prior AI findings: 2 no longer raised, 0 still raised; additional unresolved findings in this review: 5.

New attribution: 4 newly introduced, 1 late catch, 0 previously raised, 0 unattributed.

PR description suggestions

The GROUP BY claims do not match master. Drop the fifth example in "Why are the changes needed?" (SELECT try_add(try_add(x, -5), y) FROM t1 GROUP BY try_add(try_add(x, y), -5)), the sentence in the first section saying a SELECT expression could be matched to a differently grouped GROUP BY chain, and the user-facing bullet saying such a query now fails with MISSING_AGGREGATION instead of returning the grouping value: master already rejects it, because during analysis try_add is still a TryAdd wrapper whose canonical form keeps the nesting. Describe the canonicalization change instead as keeping differently grouped TRY chains apart after optimization, for example in subexpression elimination, and update the testing section's mention of GROUP BY coverage to match.

Decision challenges

Document that try_add/try_multiply mixed with + or * can newly fail after upgrade

Users who upgrade get correct results, but some queries that used to succeed now fail: ANSI batch queries where a regular + or * overflows inside try_add or try_multiply, and stateful streaming queries on older checkpoints whose key comes from a regular + or * over a TRY operand.

In batch, under ANSI, SELECT try_add(x + y, 1) with an overflowing x + y returned NULL on master and now raises ARITHMETIC_OVERFLOW. For streaming, take groupBy(expr("try_add(coalesce(value, 0), 1) + 1")) over a rate source: master rewrote the key to coalesce(value, 0) + 2, which is non-nullable, and a checkpoint from before always-nullable stateful output pins spark.sql.streaming.statefulOperator.alwaysNullableOutput.enabled to false. With this PR the key is nullable, and the state schema check rejects a nullable field over the stored non-nullable one, so the restart fails. Both new behaviors are correct, and a rollback flag would only switch the wrong rewrite back on, so the open question is only whether to document them; the PR adds no migration-guide entry.

If you treat this as a bug fix without a note, nothing changes, and users find out when try_add(x + y, 1) starts raising or a restart fails. If you add a note, add a docs/sql-migration-guide.md entry saying that try_add and try_multiply combined with regular + or * now follow each operator's own overflow behavior (for example, try_add(x + y, 1) raises under ANSI when x + y overflows, and try_add(x, y) + 1 returns NULL when x + y overflows) and that a TRY chain returns NULL exactly when a written step overflows, plus a docs/streaming/ss-migration-guide.md entry saying that such a stateful query on an older checkpoint can fail the state schema check on restart and needs a new checkpoint.

cloud-fan · 2026-11-09T02:32:52Z

 * Reorder associative integral-type operators and fold all constants into one.
 */
 object ReorderAssociativeOperator extends Rule[LogicalPlan] {
+  // Flatten only operators evaluated in the same mode as the root: rebuilding a TRY-mode Add or


Nit (P3): The scaladoc just above (line 370) still says the rule will "fold all constants into one". With this change the rule keeps an operator of another mode whole, so in Add(Add(a, 1, EvalMode.TRY), 2) the TRY literal 1 is not folded with the outer 2, and it leaves both literals in try_add(try_add(a, 5), -3). Could you reword it to match this comment, for example: the rule reorders and folds the literals of integral Add and Multiply chains whose operators share an evaluation mode, keeps an operator of another mode as one operand, and leaves TRY chains in their written grouping apart from rewrites that cannot change the result?

Verification:

Inspection: The scaladoc and the comment above the flatten helpers describe the same behavior as the rule's TRY and non-TRY branches, including different-mode operands kept whole and TRY chains folded only by rewrites that cannot change the result.

Reworded the scaladoc: the rule reorders and folds the literals of integral Add and Multiply chains whose operators share an evaluation mode, keeps an operator of another mode as one operand, and leaves TRY chains in their written grouping apart from the exact rewrites in rewriteTryChain.

Addressed in c94ab88.

### What changes were proposed in this pull request? Following on to #59829, refactor the scheduled build status updater (`update_build_status.yml`) completely: - Use batched GraphQL requests instead of per-PR REST requests. - Only update PR checks that need to be changed. Do a full sync periodically to catch build updates not associated with PR updates (e.g. a job is rerun with no new commit). - Clean up the log to only show key metrics and updates made. - Move the core logic into its own JavaScript file next to the workflow. Example of cleaned up log: ```text #60090 1dac863: queued -> in_progress #59954 48a5471: in_progress -> success 726 PRs checked, 36 skipped. Build checks: 7 updated, 2 created, 681 unchanged. ``` ### Why are the changes needed? Currently, an update pass takes about 10 minutes on a 15-minute schedule. That slows down status updates and makes changes to the workflow painful to test. Nearly all of that time goes to per-PR REST reads for PRs that haven't changed in weeks, and to rewriting checks that haven't changed. Fork runs usually finish within a few hours of a push. Of the latest fork runs on 690 open PRs, 8 were rerun more than 3 days after the push. The full sync picks those up. | | Before | After | |---|---|---| | Time per pass | ~10 min | ~45-60 s (~3-5 min for a full sync) | | PRs synced per pass | ~725 (all open) | ~170 | | API reads per pass | ~1,480 REST | 7 GraphQL | | Check updates per pass | ~685 | ~3 | | Log lines per pass | ~3,400 | ~10-15 | | Runner time per day | ~16 h | ~3 h | ### Does this PR introduce _any_ user-facing change? No, not for end-users. For contributors: `Build` checks update the same way, just sooner. A fork run rerun more than 3 days after the PR was last updated shows up on the next full sync, which is scheduled every 4 hours, or on the next pass after any PR activity (e.g. a comment). Contributor guidance now explains enabling Actions, using a non-master branch, and retriggering builds. To keep fork master as an upstream-sync branch, new Build checks for PRs from master provide `action_required` guidance rather than linking to a build, even if an older workflow ran there; existing run-linked checks continue updating. ### How was this patch tested? Via an agent: I replayed the old and new scripts read-only against live `apache/spark` PRs, intercepting writes. The new script made no write the old one didn't. All but one of the updates it skipped were no-ops. The remaining one was a stuck rerun on a PR outside the 3-day window, which the full sync covers. A full-sync replay synced all 727 open PRs. I also replayed both scripts with existing checks hidden, so every check had to be created. 724 of 725 creates matched, and each new check had its fork run's state. The one difference was a PR pushed a few minutes earlier with no fork run yet, which the new script left for the next pass. I ran quick and full syncs locally through `actions/github-script` against live PR data, intercepting writes and simulating responses, including ignored rerun updates. They checked 169 and 729 PRs in 57 seconds and 264 seconds, and logged 7 and 8 lines. All 26 focused tests passed, covering unchanged checks, applied and ignored updates, new checks, PRs from a fork's `master`, and errors. The test and dry-run harnesses load the JavaScript module directly. Replaying the 3 open PRs from a fork's `master` with their checks hidden, each got `action_required`. Tested on GitHub against my personal fork with a real `GITHUB_TOKEN`: a full read-only sync checked 728 PRs in 4m30s using 18 GraphQL points. A temporary fork PR verified check creation, updating the same check from queued to cancelled, and zero API writes on an unchanged repeat, with no duplicate checks. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: GitHub Copilot Closes #60098 from nchammas/build-status-optimization. Authored-by: Nicholas Chammas <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> skip-checks: true

…sted ReorderAssociativeOperator again removes TRY steps that add 0 or multiply by 1, and folds the literals on the path from the top of a TRY chain through steps with one literal operand into one step above the sub-expression where that path ends, when the literals are not NULL, move away from zero, and fold to a value that fits the type. These rewrites cannot change which written step overflows, and master already applied them, so filters such as try_add(a, 0) > 5 can be pushed down again. Add and Multiply canonicalization now keeps a nested non-ANSI decimal step whose result type is bounded by the maximum precision as one operand, as it does for TRY steps, because such a step returns NULL on its own overflow or rounds. Differently grouped chains of such steps are no longer semantically equal. Also add plan tests for the BIGINT, SMALLINT and TINYINT fit checks and the NULL-literal guard, replace the TRY GROUP BY SQL tests, which already passed on master, with projections that select both groupings, add non-ANSI decimal SQL tests, reword the rule's scaladoc, and document the behavior changes in the SQL and Structured Streaming migration guides. Co-authored-by: Isaac <[email protected]>

joelrobin18 · 2026-11-11T17:11:20Z

Added docs/sql-migration-guide.md entries for the TRY behavior change and the non-ANSI decimal GROUP BY change, and a docs/streaming/ss-migration-guide.md entry for stateful queries started before Spark 4.3 (whose checkpoints pin always-nullable stateful output off).

Addressed in c94ab88.

cloud-fan

Review summary

Four of the previous review's five findings are fixed. rewriteTryChain restores the exact rewrites the base branch applied to TRY chains, removing identity steps and folding literal tails, while every other TRY chain keeps its written grouping. The rule's scaladoc now matches its behavior, the BIGINT, SMALLINT, TINYINT and NULL-literal folds have plan tests, and the TRY GROUP BY queries were replaced by projections that fail under the base branch's canonicalization. The migration-guide entries asked for in the previous review are in place, and the Structured Streaming entry's Spark 4.3 bound is correct.

The fifth finding, non-ANSI decimal chains regrouping in canonicalization, is fixed for nested steps: a LEGACY decimal step whose exact result can exceed 38 digits now stays one operand. One residual remains. A chain's top step is still gathered with its exact nested steps, so when the internal multi-operand canonicalization threshold is raised, such a chain can still match a grouping that keeps a bounded nested step whole.

ANSI decimal steps also need the same treatment. Under precision loss they round silently just like the LEGACY steps the fix now keeps whole, so in the default configuration differently grouped ANSI DECIMAL(38,10) chains can still share a canonical form while computing different values. The previous review's suggestion to leave ANSI chains unchanged considered only overflow. One repair to the regrouping check covers both canonicalization cases. The last finding is a test gap: the grouping-expression guard of the TRY path in rewriteTryChain has no Aggregate test.

Findings

3 total: 0 P0, 0 P1, 2 P2, 1 P3.

Non-blocking (P2)

ANSI decimal steps that round under precision loss are still regrouped in canonicalization — sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:464 — see inline.
A top step that can overflow still gathers its exact nested steps, so groupings can match under a raised threshold (still raised) — sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:504 — see inline.

Nit (P3)

rewriteTryChain's grouping-expression guard has no Aggregate test — sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala:431 — see inline.

Shared repair plans

Shared repair plan 1

Covered findings:

ANSI decimal steps that round under precision loss are still regrouped in canonicalization — sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:464
A top step that can overflow still gathers its exact nested steps, so groupings can match under a raised threshold — sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:504

Recommended change: In Add.canBeRegrouped and Multiply.canBeRegrouped, also return false for an ANSI step with decimal operands whose result scale is below its exact scale (the larger operand scale for Add, the sum of the operand scales for Multiply), because such a step rounds silently under precision loss. In Add.canonicalized and Multiply.canonicalized, when the top step itself cannot be regrouped and gathering yields more than two operands, build the multi-operand canonical form regardless of spark.sql.analyzer.canonicalization.multiCommutativeOpMemoryOptThreshold, so a gathered list is never reduced into a nested binary tree that looks like a step kept as one operand. Update the comment on the regrouping check, add canonicalization tests and decimal SQL query tests with scaled decimal columns under both ANSI settings, and extend the SQL migration-guide entry for decimal chains, and the PR description, to ANSI steps that round.

Why this works: Canonical forms then agree only when regrouping cannot change the result. A nested step that can return NULL on its own overflow or round, in any evaluation mode, stays one operand. A chain whose top step can do so still gathers its exact nested steps, which is safe because every grouping of those steps feeds the same exact sum into the top step, but it represents them as one multi-operand node. A chain that gathers both operands of a step that another grouping keeps whole always has a top step that cannot be regrouped either, so its multi-operand form, whose operands are the leaves, can no longer coincide with the other grouping's form, which contains the kept step.

Scope: Make the Add and Multiply canonical form treat every step that can round or return NULL on its own overflow as non-regroupable in every mode, and keep the gathered operands under such a top step in multi-operand form, with canonicalization and decimal SQL tests and matching migration-guide text.

Compatibility: At the default multi-operand threshold, every chain without a nested ANSI decimal step that rounds keeps its current canonical form, so integral chains in every mode, ANSI decimal chains that cannot round, LEGACY decimal chains whose nested steps fit 38 digits exactly, and TRY chains keep their current subexpression sharing, GROUP BY matching and plan reuse.

Risks: Differently grouped ANSI decimal chains with a nested step that rounds under precision loss, which includes sums of DECIMAL(38, s) columns with s above 6 and many decimal products in the default configuration, no longer share a subexpression, a cached plan or an exchange, and a query that selects a regrouped copy of such a GROUP BY chain fails with MISSING_AGGREGATION where the base branch returned the grouping value; reverting the ANSI case of the regrouping check restores the previous matching. With the multi-operand threshold raised, a chain whose top step cannot be regrouped gets a multi-operand canonical form instead of a nested binary one, so it no longer matches a differently grouped chain over the same operands whose top step can be regrouped, even when both compute the same value. For example, with ANSI off and a DECIMAL(37,0) x and DECIMAL(1,0) y and z, (x + y) + z and (y + z) + x match on the base branch at every threshold, but with this change they match only at thresholds of 3 or less, including the default, where both already use the multi-operand form. Under a raised threshold, a query that relies on such a match loses subexpression sharing or fails with MISSING_AGGREGATION; it never returns a wrong result.

Constraints: Keep TRY steps, and non-ANSI decimal steps whose exact result can exceed 38 digits, non-regroupable as they are now. No multi-operand threshold may make two chains semantically equal when their results can differ. Leave the canonical forms of And, Or and the bitwise operators unchanged.

Exclusions:

Floating-point Add and Multiply steps stay regroupable in canonicalization, as on the base branch, because floating-point results are approximate and their evaluation order already varies, for example across partial aggregation.
Regrouping ANSI decimal or integral steps that cannot round, which can move or suppress an overflow error, stays as on the base branch, as the earlier review decided.

Success: With ANSI on and precision loss allowed, differently grouped DECIMAL(38,10) Add chains and DECIMAL(38,10) Multiply chains are not semantically equal at the default and at a raised multi-operand threshold. With ANSI on, SELECT (a + b) + c, (a + c) + b over DECIMAL(38,10) columns returns 0.00000001 and 0.00000000 for the row (0.0000000044, 0.0000000001, 0), and selecting (a + c) + b with GROUP BY (a + b) + c fails with MISSING_AGGREGATION. With ANSI off, (p + q) + r and (p + r) + q for DECIMAL(37,0) p and q and DECIMAL(38,0) r are not semantically equal at any multi-operand threshold, while (p + q) + r and (q + p) + r still are. ANSI DECIMAL(38,0) chains, integral chains in every mode, and operand swaps within a step remain semantically equal at both thresholds; LEGACY decimal chains whose nested steps fit 38 digits exactly remain semantically equal at the default threshold, and at a raised threshold when both top steps can be regrouped or neither can, as in the existing DECIMAL(37,0) Add and DECIMAL(18,0) and DECIMAL(19,0) Multiply assertions.

Verification:

Regression: Canonicalization tests under ANSI with precision loss allowed show that (a + b) + c and (a + c) + b over DECIMAL(38,10) columns, and the analogous DECIMAL(38,10) Multiply chains, are not semantically equal both below and above the multi-operand threshold.
Regression: Decimal SQL query tests over a table row (0.0000000044, 0.0000000001, 0) of DECIMAL(38,10) columns show that selecting both groupings returns 0.00000001 and 0.00000000 and that selecting a regrouped copy of the GROUP BY chain fails with MISSING_AGGREGATION, with and without ANSI.
Regression: Canonicalization tests with ANSI off and the multi-operand threshold raised show that a chain whose top step can overflow, such as (p + q) + r with p and q DECIMAL(37,0) and r DECIMAL(38,0), gets a multi-operand canonical form and is not semantically equal to (p + r) + q for several sets of freshly created attributes, while (q + p) + r still matches it.
Regression: A canonicalization test with ANSI off shows that a chain over DECIMAL(38,10) columns keeps its nested step as one operand, which exercises the scale terms of the non-ANSI precision check.
Compatibility: ANSI DECIMAL(38,0) Add and Multiply chains, integral chains in every mode, and operand swaps within a step remain semantically equal at both thresholds, LEGACY decimal chains whose nested steps fit 38 digits exactly remain equal at the default threshold, and the existing canonicalization assertions, including the SPARK-42162 check that a chain below a raised threshold keeps the binary form, and the decimal golden results keep their outcomes.
Inspection: The SQL migration-guide entry for decimal chains and the comment on the regrouping check name ANSI decimal steps that round under precision loss alongside non-ANSI steps that can overflow or round.

Re-review status

Prior AI findings: 4 no longer raised, 1 still raised; additional unresolved findings in this review: 2.

New attribution: 1 newly introduced, 1 late catch, 0 previously raised, 0 unattributed.

cloud-fan · 2026-11-12T10:39:59Z

+  // precision loss. Regrouping such a step can change the result.
+  private def canBeRegrouped: Boolean = evalMode match {
+    case EvalMode.TRY => false
+    case EvalMode.LEGACY if childrenResolved => (left.dataType, right.dataType) match {


Non-blocking (P2): This gate keeps a nested decimal step whole only in LEGACY mode, so every ANSI step falls through to case _ => true and is still gathered. But ANSI decimal steps also round silently. With spark.sql.decimalOperations.allowPrecisionLoss=true, the default, adjustPrecisionScale lowers the scale of a step whose unbounded precision exceeds 38 and whose scale exceeds 6, and checkDecimalOverflow rounds with ROUND_HALF_UP and raises only when the precision overflows. The comment above names rounding under precision loss as a reason a step cannot be regrouped, but the gate applies that reason only to LEGACY steps.

For example, take ANSI on and DECIMAL(38,10) columns with a = 0.0000000044, b = 0.0000000001 and c = 0. By Spark's decimal type rules, a + b has type DECIMAL(38,9) and rounds 0.0000000045 to 0.000000005, so (a + b) + c is 0.00000001 as DECIMAL(38,8). a + c rounds to 0.000000004, so (a + c) + b is 0.0000000041, which rounds to 0.00000000. Both chains have type DECIMAL(38,8), so the SPARK-40903 fallback does not separate them, and both canonicalize to the operand list [a, b, c]. Reading the code, SELECT (a + c) + b FROM t GROUP BY (a + b) + c passes analysis and returns the grouping value, and subexpression elimination can evaluate one chain for both columns of SELECT (a + b) + c, (a + c) + b. Multiply behaves the same way: for DECIMAL(38,10) columns with x = 0.0000005, y = 1 and z = 3, (x * y) * z is 0.000003 and (x * z) * y is 0.000002, both DECIMAL(38,6).

This is the wrong result the PR fixes for non-ANSI steps, and it reaches every nested ANSI decimal Add or Multiply step whose result scale is below its exact scale. My earlier suggestion to leave ANSI chains unchanged considered only overflow, which raises under ANSI, and missed rounding. The new tests cannot catch it, because the ANSI assertion and every decimal operand in CanonicalizeSuite use scale 0.

This extends an existing discussion.

See Shared repair plan 1 in the review body.

cloud-fan · 2026-11-12T10:39:59Z

    val reorderResult = buildCanonicalizedPlan(
-      { case Add(l, r, em) if em == evalContext => Seq(l, r) },
+      // A nested step that cannot be regrouped stays one operand, canonicalized on its own.
+      { case a @ Add(l, r, em) if em == evalContext && ((a eq this) || a.canBeRegrouped) =>


Non-blocking (P2): The top step is always gathered here (a eq this), and so are its exact nested steps, even when the top step itself can overflow or round. At the default threshold of 3, a gathered list of three or more operands becomes a MultiCommutativeOp, so it cannot equal a chain that keeps a bounded nested step whole. With spark.sql.analyzer.canonicalization.multiCommutativeOpMemoryOptThreshold raised above 3, as the PR's own tests do, the list is instead reduced into a binary tree in hash order, and that tree can be the same one.

For example, take ANSI off, p and q of type DECIMAL(37,0), r of type DECIMAL(38,0), and the threshold at 100. In (p + q) + r the nested p + q fits 38 digits and is gathered, so when the hash order of the operands is p, r, q, they reduce to Add(Add(p, r), q). In (p + r) + q the nested p + r can overflow and stays one operand, Add(p, r), and when it sorts before q this chain also canonicalizes to Add(Add(p, r), q). Both chains have type DECIMAL(38,0), so the result-type fallback accepts both. Reading the code, for p = 5E36, q = -5E36 and r = 10^38 - 1, (p + q) + r is 10^38 - 1 while (p + r) + q is NULL, yet the two are semanticEquals. Whether they match depends on the internal threshold and on attribute hash order, which follows expression IDs.

This extends an existing discussion.

See Shared repair plan 1 in the review body.

cloud-fan · 2026-11-12T10:39:59Z

+      combine: (BigInt, BigInt) => BigInt,
+      build: (Expression, Expression) => Expression): Expression = {
+    def operands(e: Expression): Option[(Expression, Expression)] =
+      if (groupSet.contains(e)) None else tryStepOperands.lift(e)


Nit (P3): This groupSet check is what keeps a TRY chain in an Aggregate derivable from its grouping expressions. For SELECT try_add(try_add(a, 1), 2) FROM t GROUP BY try_add(a, 1), it stops the literal path at the grouping step, so only one literal is collected and the chain stays as written. Without the check, the path would continue to a and fold 1 and 2, turning the aggregate expression into try_add(a, 3), which no longer derives from the grouping expression and fails at planning. No test exercises it: every new TRY test in ReorderAssociativeOperatorSuite runs on a Project or a Filter, and the only Aggregate test, "nested expression with aggregate operator", uses default-mode Add and reaches flattenAdd's guard instead. Could you add a plan test with an Aggregate that groups by a TRY step and selects a TRY chain built on it, for both try_add and try_multiply?

Verification:

Regression: An optimizer plan test over an Aggregate that groups by a TRY step and selects a TRY chain built on that step keeps the aggregate expression as written for try_add and try_multiply under both ANSI settings, and fails when the TRY rewrite no longer stops at grouping expressions.

joelrobin18 changed the title ~~[SQL] Do not reorder associative operators across different evaluation modes~~ [SPARK-60749][SQL] Do not reorder associative operators across different evaluation modes Oct 30, 2026

joelrobin18 marked this pull request as ready for review October 30, 2026 14:45

joelrobin18 force-pushed the fix-reorder-associative-eval-mode branch from 687bcd0 to 7c6cd2a Compare October 30, 2026 14:51

joelrobin18 force-pushed the fix-reorder-associative-eval-mode branch from 7c6cd2a to 687b368 Compare October 30, 2026 14:57

Add LEGACY-mode cases to ReorderAssociativeOperatorSuite

6954f13

Cover ANSI-mode operators under a LEGACY root, LEGACY chains under an ANSI root, and TRY-mode operators under a LEGACY root with ANSI disabled. Co-authored-by: Isaac <[email protected]>

cloud-fan reviewed Nov 2, 2026

View reviewed changes

Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala Resolved

Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala Resolved

joelrobin18 and others added 2 commits November 3, 2026 05:37

joelrobin18 requested a review from cloud-fan November 4, 2026 06:14

joelrobin18 and others added 4 commits November 4, 2026 06:14

Merge branch 'master' into fix-reorder-associative-eval-mode

b642b51

Merge branch 'master' into fix-reorder-associative-eval-mode

168e857

Co-authored-by: Isaac <[email protected]>

Re-trigger CI

fa6a662

Co-authored-by: Isaac <[email protected]>

cloud-fan reviewed Nov 9, 2026

View reviewed changes

joelrobin18 requested a review from cloud-fan November 11, 2026 17:18

github-actions Bot added SQL DOCS labels Nov 11, 2026

cloud-fan reviewed Nov 12, 2026

View reviewed changes

Navigation Menu

Repository navigation

[SPARK-60749][SQL] Do not reorder associative operators across different evaluation modes - #59954

[SPARK-60749][SQL] Do not reorder associative operators across different evaluation modes#59954
joelrobin18 wants to merge 9 commits into
apache:masterfrom
joelrobin18:fix-reorder-associative-eval-mode

joelrobin18 commented Oct 30, 2026 •

edited

Loading

Uh oh!

cloud-fan left a comment

Uh oh!

Uh oh!

Uh oh!

cloud-fan left a comment

Uh oh!

Uh oh!

Uh oh!

cloud-fan Nov 9, 2026

Uh oh!

joelrobin18 Nov 11, 2026

Uh oh!

Uh oh!

Uh oh!

joelrobin18 commented Nov 11, 2026

Uh oh!

cloud-fan left a comment

Uh oh!

cloud-fan Nov 12, 2026

Uh oh!

cloud-fan Nov 12, 2026

Uh oh!

cloud-fan Nov 12, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

joelrobin18 commented Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

cloud-fan left a comment

Choose a reason for hiding this comment

Review summary

Findings

Blocking (P1)

Non-blocking (P2)

Uh oh!

Uh oh!

Uh oh!

cloud-fan left a comment

Choose a reason for hiding this comment

Review summary

Cross-cutting observations

Findings

Blocking (P1)

Non-blocking (P2)

Nit (P3)

Re-review status

PR description suggestions

Decision challenges

Document that try_add/try_multiply mixed with + or * can newly fail after upgrade

Uh oh!

Uh oh!

Uh oh!

cloud-fan Nov 9, 2026

Choose a reason for hiding this comment

Uh oh!

joelrobin18 Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joelrobin18 commented Nov 11, 2026

Uh oh!

cloud-fan left a comment

Choose a reason for hiding this comment

Review summary

Findings

Non-blocking (P2)

Nit (P3)

Shared repair plans

Shared repair plan 1

Re-review status

Uh oh!

cloud-fan Nov 12, 2026

Choose a reason for hiding this comment

Uh oh!

cloud-fan Nov 12, 2026

Choose a reason for hiding this comment

Uh oh!

cloud-fan Nov 12, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joelrobin18 commented Oct 30, 2026 •

edited

Loading