diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/SimplifyArithmeticRule.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/SimplifyArithmeticRule.java index 6eea495e5cfc22..3076e36ec52e11 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/SimplifyArithmeticRule.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/SimplifyArithmeticRule.java @@ -22,12 +22,10 @@ import org.apache.doris.nereids.rules.expression.ExpressionRuleType; import org.apache.doris.nereids.trees.expressions.Add; import org.apache.doris.nereids.trees.expressions.BinaryArithmetic; -import org.apache.doris.nereids.trees.expressions.Cast; import org.apache.doris.nereids.trees.expressions.Divide; import org.apache.doris.nereids.trees.expressions.Expression; import org.apache.doris.nereids.trees.expressions.Multiply; import org.apache.doris.nereids.trees.expressions.Subtract; -import org.apache.doris.nereids.types.DataType; import org.apache.doris.nereids.util.TypeCoercionUtils; import org.apache.doris.nereids.util.TypeUtils; import org.apache.doris.nereids.util.Utils; @@ -138,16 +136,13 @@ private static Expression process(BinaryArithmetic arithmetic, boolean isAddOrSu // isAddOrSub: true for extract only "+" or "-" sub expressions, false for extract only "*" or "/" sub expressions private static List flatten(Expression expr, boolean isAddOrSub) { List result = Lists.newArrayList(); - doFlatten(true, expr, isAddOrSub, result, Optional.empty()); + doFlatten(true, expr, isAddOrSub, result); return result; } // flag: true for '+' or '*', false for '-' or '/' // isAddOrSub: true for extract only "+" or "-" sub expressions, false for extract only "*" or "/" sub expressions - private static void doFlatten(boolean flag, Expression expr, boolean isAddOrSub, List result, - Optional castType) { - // cast (a * 10 as double) * (cast 20 as double) - // => cast(a as double) * (cast 10 as double) * (cast 20 as double) + private static void doFlatten(boolean flag, Expression expr, boolean isAddOrSub, List result) { BinaryArithmetic arithmetic = null; Predicate isPositiveArithmetic = isAddOrSub ? TypeUtils::isAdd : TypeUtils::isMultiply; @@ -156,52 +151,20 @@ private static void doFlatten(boolean flag, Expression expr, boolean isAddOrSub, Predicate isPosNegArithmetic = isPositiveArithmetic.or(isNegativeArithmetic); if (isPosNegArithmetic.test(expr)) { arithmetic = (BinaryArithmetic) expr; - } else if (expr instanceof Cast && hasConstantOperand(expr, isAddOrSub)) { - Cast cast = (Cast) expr; - if (isPosNegArithmetic.test(cast.child())) { - arithmetic = (BinaryArithmetic) cast.child(); - castType = Optional.of(cast.getDataType()); - } } if (arithmetic != null) { - doFlatten(flag, arithmetic.left(), isAddOrSub, result, castType); + doFlatten(flag, arithmetic.left(), isAddOrSub, result); if (isNegativeArithmetic.test(arithmetic) && !flag) { - doFlatten(true, arithmetic.right(), isAddOrSub, result, castType); + doFlatten(true, arithmetic.right(), isAddOrSub, result); } else if (isPositiveArithmetic.test(arithmetic) && !flag) { - doFlatten(false, arithmetic.right(), isAddOrSub, result, castType); + doFlatten(false, arithmetic.right(), isAddOrSub, result); } else { - doFlatten(!isNegativeArithmetic.test(arithmetic), arithmetic.right(), isAddOrSub, result, castType); + doFlatten(!isNegativeArithmetic.test(arithmetic), arithmetic.right(), isAddOrSub, result); } } else { - if (castType.isPresent()) { - result.add(Operand.of(flag, TypeCoercionUtils.castIfNotSameType(expr, castType.get()))); - } else { - result.add(Operand.of(flag, expr)); - } - } - } - - private static boolean hasConstantOperand(Expression expr, boolean isAddOrSub) { - if (expr.isConstant()) { - return true; - } - - Predicate checkArithmetic = isAddOrSub - ? TypeUtils::isAddOrSubtract : TypeUtils::isMultiplyOrDivide; - BinaryArithmetic arithmetic = null; - if (checkArithmetic.test(expr)) { - arithmetic = (BinaryArithmetic) expr; - } else if (expr instanceof Cast) { - Cast cast = (Cast) expr; - if (checkArithmetic.test(cast.child())) { - arithmetic = (BinaryArithmetic) cast.child(); - } - } - if (arithmetic != null) { - return hasConstantOperand(arithmetic.left(), isAddOrSub) - || hasConstantOperand(arithmetic.right(), isAddOrSub); - } else { - return false; + // Keep non-arithmetic expressions atomic. In particular, moving a cast to the + // operands can change rounding, overflow, error and null behavior. + result.add(Operand.of(flag, expr)); } } @@ -241,4 +204,3 @@ public String toString() { } } } - diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/expression/SimplifyArithmeticRuleTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/expression/SimplifyArithmeticRuleTest.java index df14840f628e6f..cb09906214bbc9 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/expression/SimplifyArithmeticRuleTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/expression/SimplifyArithmeticRuleTest.java @@ -95,6 +95,19 @@ void testSimplifyArithmeticRuleOnly() { assertRewriteAfterSimplify("-IA / 2.0 * ((-IB - 1) - (3 + (IC + 4)))", "(((0 - IA) / 2.0) * (((0 - IB) - 1) - (3 + (IC + 4))))"); } + @Test + void testPreserveCastAroundArithmetic() { + executor = new ExpressionRuleExecutor(ImmutableList.of( + bottomUp(SimplifyArithmeticRule.INSTANCE) + )); + + String castExpression = "cast(LA - 9223372036854775800 as double) + cast(0 as double)"; + assertRewriteAfterSimplify(castExpression, castExpression); + + String tryCastExpression = "try_cast(LA - 9223372036854775800 as double) + cast(0 as double)"; + assertRewriteAfterSimplify(tryCastExpression, tryCastExpression); + } + @Test void testSimplifyArithmeticComparison() { executor = new ExpressionRuleExecutor(ImmutableList.of( diff --git a/regression-test/suites/external_table_p0/dialect_compatible/sql/presto/scalar/timestamp/TestOperators.sql b/regression-test/suites/external_table_p0/dialect_compatible/sql/presto/scalar/timestamp/TestOperators.sql index 5a3d008a0e4eca..c8510432786244 100644 --- a/regression-test/suites/external_table_p0/dialect_compatible/sql/presto/scalar/timestamp/TestOperators.sql +++ b/regression-test/suites/external_table_p0/dialect_compatible/sql/presto/scalar/timestamp/TestOperators.sql @@ -222,6 +222,7 @@ SELECT TIMESTAMP '2020-05-01 12:34:56.123456789' BETWEEN TIMESTAMP '2020-05-01 1 SELECT TIMESTAMP '2020-05-01 12:34:56.1234567890' BETWEEN TIMESTAMP '2020-05-01 12:34:56.1234567889' and TIMESTAMP '2020-05-01 12:34:56.1234567891'; SELECT TIMESTAMP '2020-05-01 12:34:56.12345678901' BETWEEN TIMESTAMP '2020-05-01 12:34:56.1234567890' and TIMESTAMP '2020-05-01 12:34:56.12345678902'; SELECT TIMESTAMP '2020-05-01 12:34:56.123456789012' BETWEEN TIMESTAMP '2020-05-01 12:34:56.123456789011' and TIMESTAMP '2020-05-01 12:34:56.123456789013'; +set debug_skip_fold_constant=true; -- SELECT TIMESTAMP '2020-05-01 12:34:56' + INTERVAL '1.123' SECOND; # differ: doris : None, presto : 2020-05-01 12:34:57.123 -- SELECT TIMESTAMP '2020-05-01 12:34:56.1' + INTERVAL '1.123' SECOND; # differ: doris : None, presto : 2020-05-01 12:34:57.223 -- SELECT TIMESTAMP '2020-05-01 12:34:56.12' + INTERVAL '1.123' SECOND; # differ: doris : None, presto : 2020-05-01 12:34:57.243 @@ -389,7 +390,6 @@ SELECT TIMESTAMP '2020-05-01 12:34:56.123456789012' BETWEEN TIMESTAMP '2020-05-0 -- SELECT TIMESTAMP '2020-05-01 12:34:55.1111111111' - TIMESTAMP '2020-05-01 12:34:56.9999999999'; # differ: doris : -2, presto : -0 00:00:01.889 -- SELECT TIMESTAMP '2020-05-01 12:34:55.11111111111' - TIMESTAMP '2020-05-01 12:34:56.99999999999'; # differ: doris : -2, presto : -0 00:00:01.889 -- SELECT TIMESTAMP '2020-05-01 12:34:55.111111111111' - TIMESTAMP '2020-05-01 12:34:56.999999999999'; # differ: doris : -2, presto : -0 00:00:01.889 -set debug_skip_fold_constant=true; SELECT TIMESTAMP '2020-05-01 12:34:56' = TIMESTAMP '2020-05-01 12:34:56'; SELECT TIMESTAMP '2020-05-01 12:34:56.1' = TIMESTAMP '2020-05-01 12:34:56.1'; SELECT TIMESTAMP '2020-05-01 12:34:56.12' = TIMESTAMP '2020-05-01 12:34:56.12';