github-actions[bot] commented on code in PR #67895:
URL: https://github.com/apache/doris/pull/67895#discussion_r4002113306
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/expression/rules/SimplifyArithmeticComparisonRuleTest.java:
##########
@@ -94,23 +94,57 @@ public void testNumeric() {
assertRewriteAfterSimplify("-2.22 / IA > 1.1", "((-2.22 / IA) > 1.1)");
assertRewriteAfterSimplify("IA * 2.22 > 1.1", "IA * 2.22 > 1.1");
assertRewriteAfterSimplify("IA * (-2.22) > 1.1", "IA * (-2.22) > 1.1");
- assertRewriteAfterSimplify("IA / 2.22 > 1.1", "(cast(IA as
DECIMALV3(13, 3)) > cast((1.1 * 2.22) as DECIMALV3(13, 3)))");
- assertRewriteAfterSimplify("IA / (-2.22) > 1.1", "(cast((1.1 * -2.22)
as DECIMALV3(13, 3)) > cast(IA as DECIMALV3(13, 3)))");
+ assertDivisionPreservedAfterConstantFolding("IA / 2.22 > 1.1");
+ assertDivisionPreservedAfterConstantFolding("IA / (-2.22) > 1.1");
// test (1 + IA) can be processed
assertRewriteAfterSimplify("2 - (1 + IA) > 3", "(IA < cast(((2 - 3) -
1) as INT))");
- assertRewriteAfterSimplify("(1 - IA) / 2 > 3", "(IA < cast((1 - 6) as
INT))");
- assertRewriteAfterSimplify("1 - IA / 2 > 3", "(IA < cast(((1 - 3) * 2)
as INT))");
- assertRewriteAfterSimplify("(1 - (IA + 4)) / 2 > 3", "(IA < cast(((1 -
6) - 4) as INT))");
+ assertDivisionPreservedAfterConstantFolding("(1 - IA) / 2 > 3");
+ assertRewriteAfterSimplify("1 - IA / 2 > 3", "((IA / 2) < cast((1 - 3)
as DOUBLE))");
Review Comment:
**[P1] Do not retain additive inversion around a `Divide`**
This expected partial rewrite is still unsafe for analyzed DOUBLE values.
With `x=10000000000000000`, `(x / 1.0 + 1.0) >= CAST('10000000000000002' AS
DOUBLE)` is false because the left rounds to `10000000000000000`; the retained
Add-to-Subtract rewrite produces `x / 1.0 >= (CAST('10000000000000002' AS
DOUBLE) - 1.0)`, whose right side rounds to the same `10000000000000000`, so it
becomes true. Preserving the inner node is insufficient: please fence
add/subtract rearrangement when the comparison contains `Divide` (unless
exactness is proven) and add an enabled/disabled runtime boundary case.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/SimplifyArithmeticComparisonRule.java:
##########
@@ -53,17 +50,16 @@
/**
* Simplify arithmetic comparison rule.
* a + 1 > 1 => a > 0
- * a / -2 > 1 => a < -2
*/
public class SimplifyArithmeticComparisonRule implements
ExpressionPatternRuleFactory {
public static SimplifyArithmeticComparisonRule INSTANCE = new
SimplifyArithmeticComparisonRule();
- // don't rearrange multiplication because divide may loss precision
+ // Do not rearrange multiplication or division because their inverse
operations can change
Review Comment:
**[P1] Fence the earlier arithmetic simplifier too**
Removing `Divide` here is too late for division-containing trees because
production runs `SimplifyArithmeticRule` first. Its multiply/divide branch
rewrites `x / (y / z) < 1` as `(x / y) * z < 1`; for DOUBLE `x=1, y=2, z=0`,
the filter changes from UNKNOWN to TRUE. Its add/subtract branch also regroups
`abs(((x / 1.0) + 1.0) + 1.0) = 10000000000000000` as `abs((x / 1.0) + (1.0 +
1.0)) = 10000000000000000`; for DOUBLE `x=10000000000000000`, the predicate
changes from true to false, and `Abs` prevents this comparison rule from
matching. Please fence both `SimplifyArithmeticRule` branches for subtrees
containing `Divide` unless exact SQL equivalence is proven, and add analyzed
runtime cases with that rule enabled/disabled.
--
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]
