[SPARK-60178][SQL] Do not re-associate floating-point Add/Multiply in canonicalization - #59437

Nit (P3): These two tests used to be the only coverage for the rule's 'canonically equal but evaluated differently' guards. Now that float re-association is canonically distinct, they assert fusion instead, so no test reaches hasSafePhysicalFusion's percentages.forall(_ == first) rejection or the evaluated-accuracy split of a shared PhysicalCompatibilityKey. The 'require structural equality for inputs and filters' test still covers sizeCompare(1) through the input dimension only.

Both guards are still reachable, because decimal Add/Multiply keep flattening. With the default allowPrecisionLoss=true, three DECIMAL(38,10) literals 0.0000000004, 0.0000000001, 0.0000000040 give (a + b) + c = 1E-8 but a + (b + c) = 0E-8. Both have result type DECIMAL(38,8), so they canonicalize to the same tree; cast to DOUBLE they are canonically equal percentages that evaluate differently. DECIMAL(38,7) literals 1.9999994, 0.0000001, 1.0000000 give 3.000000 vs 2.999999, so cast to INT they are canonically equal accuracies 3 and 2. Could you keep the new float expectations and add decimal-based variants of the old assertNotCombined scenarios? Assert the canonical-equality and evaluation preconditions first, so a future canonicalization change fails loudly instead of silently dropping the coverage again.

Verification:

• Regression: Removing hasSafePhysicalFusion's per-canonical-group percentage-evaluation check makes the restored percentage scenario fail.
• Regression: Keying CompatibilityKey by the canonical accuracy expression instead of its evaluated value makes the restored accuracy scenario fail.
• Behavior: The restored scenarios assert that their decimal-derived inputs are canonically equal and evaluate differently. A future canonicalization change that makes them distinct then fails loudly instead of silently dropping guard coverage.

Thanks, good catch. Added two decimal-based tests that bring back the old assertNotCombined scenarios, using your DECIMAL(38,10) and DECIMAL(38,7) literals. Each test first asserts that the two sides are canonically equal and evaluate differently (1E-8 vs 0.0, and 3 vs 2). I checked them by mutation: dropping the percentages.forall(_ == first) check fails the percentage test, and dropping the shared-PhysicalCompatibilityKey sizeCompare(1) check fails the accuracy test.

Non-blocking (P2): This condition sends only FLOAT and DOUBLE results to the per-operator path, so decimal Add and Multiply still flatten and reorder every operand. A decimal step whose result type is capped at 38 digits rounds (with the default spark.sql.decimalOperations.allowPrecisionLoss=true) or can overflow, so its value depends on association just as a floating-point value does. With DECIMAL(38,7) columns a = 1.9999994, b = 0.0000001 and c = 1.0000000, (a + b) + c is 3.000000 and a + (b + c) is 2.999999, yet both canonicalize to the same MultiCommutativeOp and pass the SPARK-40903 type check, so SELECT (a + b) + c, a + (b + c) FROM t with subexpression elimination returns 3.000000 for both. The same equality lets a cached (a + b) + c answer a + (b + c), and lets GROUP BY match the two forms. Multiply behaves the same way and has no type check. Sums of three DECIMAL(38,18) values, the default type of Scala and Java BigDecimal fields, are one common case. Decimal chains that stay below 38 digits are exact and can keep flattening. The PR description lists decimals as out of scope, but these inputs reach the same wrong result the PR fixes for floating point. The new decimal tests in CombineApproximatePercentilesSuite assert this remaining equality, so extending the fix also means rebuilding those fixtures.

This extends an existing discussion.

Recommended change: Extend the non-associative branch of Add.canonicalized and Multiply.canonicalized: a resolved decimal result whose precision equals the 38-digit maximum should also be canonicalized with orderBinaryCommutative instead of being flattened, while decimal chains below 38 digits keep the existing flattening path. Add decimal cases to the canonicalization, subexpression elimination, cached-table and GROUP BY tests. The decimal-based percentile-fusion guard fixtures rely on the old decimal equality, and after this change decimal and floating-point literal chains no longer give canonically equal inputs that evaluate differently, so rebuild those fixtures on a test-only foldable expression whose canonicalized form stays the same while the value it returns differs. Extend the migration-guide entry to cover 38-digit decimal chains.

Why this works: A decimal step whose result type is capped at 38 digits rounds its result (with spark.sql.decimalOperations.allowPrecisionLoss) or can overflow, so moving operands across that step can change the value. Ordering only the two direct children keeps commutation, which is exact for decimals and keeps the result type, and never moves operands between levels. Below 38 digits every step is exact, and decimal precision only grows toward the root, so flattening such a chain still preserves its value.

Scope: Make 38-digit decimal Add and Multiply chains association-aware in canonicalization, and update the tests, guard fixtures, and migration note that depend on decimal re-association.

Compatibility: Decimal chains whose result precision stays below 38 digits, and all integral and interval chains, keep their current canonical forms, plans, and GROUP BY matching.

Risks: GROUP BY queries and persisted views whose 38-digit decimal select and grouping expressions differ only in association will fail with MISSING_AGGREGATION, the same trade-off this PR already makes for FLOAT/DOUBLE, and the change cannot be rolled back without adding a setting. 38-digit decimal chains no longer build a MultiCommutativeOp, so their canonical forms use one binary node per operator; memory stays linear in the chain length. Chains whose 38-digit result is still exact, such as a sum of three DECIMAL(36,0) columns, also stop matching across associations, so they lose shared subexpression results and cached-plan reuse between associations even though their values never differ; like the rest of this change, this has no setting to roll it back.

Constraints: Apply the condition at every node, so a 38-digit root never flattens its operands even when the MultiCommutativeOp threshold would otherwise apply. Keep each canonical node's result type equal to the original node's type; commuting two decimal operands already preserves it.

Exclusions:

• Integral Add and Multiply chains under ANSI or TRY mode stay on the flattening path, because Spark's optimizer already re-associates integral chains under the same eval modes. Interval Add chains stay as well, because their association can only decide whether an overflow error is raised.

Success: Differently associated decimal Add or Multiply chains whose result precision reaches 38 digits are no longer semantically equal, so subexpression elimination, cached-plan reuse, and GROUP BY matching stop substituting one for another. Swapping the two operands of a single decimal operator still canonicalizes to the same form. Decimal chains below 38 digits keep re-association equality, the MultiCommutativeOp optimization, and the SPARK-40903 fallback. The migration-guide entry describes the GROUP BY consequence for 38-digit decimal chains as well as for FLOAT/DOUBLE. The percentile-fusion guards keep regression fixtures that are canonically equal but evaluate differently.

Verification:

• Regression: For decimal Add and Multiply chains whose 38-digit steps round or overflow depending on association, a query that selects two associations returns each one's own result, including null for the overflowing side with ANSI mode off, with subexpression elimination on and off and with whole-stage codegen on and off. For example, with DECIMAL(38,7) columns a = 1.9999994, b = 0.0000001 and c = 1.0000000, (a + b) + c and a + (b + c) return 3.000000 and 2.999999.
• Regression: After (a + b) + c over those columns is cached, a query for a + (b + c) does not match the cache entry and returns its own value.
• Behavior: For 38-digit decimal Add and Multiply chains, re-associated forms are not semantically equal while commuted forms are, at both a low and a high MultiCommutativeOp threshold.
• Compatibility: Decimal chains below 38 digits, such as small-precision decimal literals, still re-associate to equal forms and still build a MultiCommutativeOp once the operand count reaches the threshold.
• Behavior: Removing either percentile-fusion guard, the per-group percentage-evaluation check or the shared physical-key check, still makes a regression test fail.

Nit (P3): This helper introduces a per-operator path, but the central contract text was not updated. The Expression.canonicalized scaladoc (Expression.scala:311-319) still says that adjacent same-class CommutativeExpressions are gathered and reordered from the root and that canonicalized is computed only on that root, and the MultiCommutativeOp scaladoc (Expression.scala:1471-1473) lists Add and Multiply without exception. Resolved FLOAT/DOUBLE Add and Multiply now never gather, never build a MultiCommutativeOp, and compute canonicalized at every node, so a maintainer relying on those docs could reintroduce the flattening this PR removes. Both scaladocs could name the exception and point to orderBinaryCommutative.

Verification:

• Inspection: The Expression.canonicalized and MultiCommutativeOp scaladocs name the Add and Multiply result types that use per-operator ordering, and that set matches the condition implemented in Add.canonicalized and Multiply.canonicalized.