HyukjinKwon commented on code in PR #59437:
URL: https://github.com/apache/spark/pull/59437#discussion_r4277966569
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/CombineApproximatePercentilesSuite.scala:
##########
@@ -249,11 +249,16 @@ class CombineApproximatePercentilesSuite extends PlanTest
{
assert(firstAccuracy.eval() == 3)
assert(secondAccuracy.eval() == 4)
- assert(firstAccuracy.canonicalized == secondAccuracy.canonicalized)
- assertNotCombined(result)
+ assert(firstAccuracy.canonicalized != secondAccuracy.canonicalized)
Review Comment:
**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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
