From 0290c8ca516c41068278fe94b4af07c0bdb0e98c Mon Sep 17 00:00:00 2001 From: morrySnow Date: Sat, 12 Sep 2026 04:20:08 +0800 Subject: [PATCH] [fix](nereids) Preserve division denominator boundaries ### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: SimplifyArithmeticRule flattened multiplication and division across a nested denominator. For example, it rewrote 1 / (1 / number) to number * 1, changing the result for zero from NULL to 0. This reassociation can also change null, overflow, and floating-point behavior. Treat a subtree reached in denominator position during multiply/divide flattening as an atomic operand. The regular recursive rewrite can still simplify inside that subtree, but its factors cannot cross the enclosing division boundary. Unit and regression tests cover nested division and multiplication denominators, zero and nullable values, and integer, double, and decimal expressions. ### Release note Fix incorrect query results when arithmetic simplification reassociates expressions across nested division denominators. ### Check List (For Author) - Test: Unit Test and Regression test - ./run-fe-ut.sh --run org.apache.doris.nereids.rules.expression.SimplifyArithmeticRuleTest - ./run-regression-test.sh --conf /tmp/env2-range-regression-conf.groovy --run -f regression-test/suites/nereids_rules_p0/expression/simplify_arithmetic/test_simplify_arithmetic.groovy - DISABLE_BUILD_UI=ON ./build.sh --fe - Behavior changed: Yes. Nested denominator expressions remain evaluation boundaries, preserving zero, null, overflow, and floating-point semantics. - Does this need documentation: No --- .../rules/SimplifyArithmeticRule.java | 8 ++++ .../SimplifyArithmeticRuleTest.java | 43 ++++++++++++++++--- .../test_simplify_arithmetic.out | 13 ++++++ .../test_simplify_arithmetic.groovy | 32 ++++++++++++++ 4 files changed, 91 insertions(+), 5 deletions(-) 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 3076e36ec52e11..312721ba16bf14 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 @@ -143,6 +143,14 @@ private static List flatten(Expression expr, boolean isAddOrSub) { // 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) { + if (!isAddOrSub && !flag) { + // A complete denominator is an evaluation boundary. Flattening its multiply/divide + // children into the enclosing expression can invert them or move them to the numerator, + // changing division-by-zero, null, overflow, and floating-point behavior. Keep the + // subtree atomic here; process() will still simplify it recursively within its boundary. + result.add(Operand.of(false, expr)); + return; + } BinaryArithmetic arithmetic = null; Predicate isPositiveArithmetic = isAddOrSub ? TypeUtils::isAdd : TypeUtils::isMultiply; 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 cb09906214bbc9..0d4136069db6c0 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 @@ -56,6 +56,34 @@ void testSimplifyArithmetic() { "(((((IA / ((IB + (IC * (-56 / 50))) + 36)) * ID) / (IE + 50)) * (((40 / 20) * 5) * 6)) + (1 + 200))"); } + @Test + void testPreserveDivisionDenominatorBoundaries() { + executor = new ExpressionRuleExecutor(ImmutableList.of( + bottomUp(SimplifyArithmeticRule.INSTANCE, FoldConstantRule.INSTANCE) + )); + + Assertions.assertAll( + // A nested denominator is an evaluation boundary: its operands cannot be + // inverted or moved into the numerator of the enclosing division. + () -> assertRewriteAfterSimplify("IA / (IB / IC)", "IA / (IB / IC)"), + () -> assertRewriteAfterSimplify("IA / (IB * IC)", "IA / (IB * IC)"), + + // Left-associated division does not introduce a nested denominator boundary. + () -> assertRewriteAfterSimplify("(IA / IB) / IC", "(IA / IB) / IC"), + + // Preserve zero and null behavior for nullable integer operands. + () -> assertRewriteAfterSimplify("1 / (1 / IA)", "1 / (1 / IA)"), + () -> assertRewriteAfterSimplify("IA / (IB / 0)", "IA / (IB / 0)"), + () -> assertRewriteAfterSimplify("IA / (IB * 0)", "IA / (IB * 0)"), + + // Floating-point arithmetic has the same evaluation-order boundary. + () -> assertRewriteAfterSimplify("DA / (DB / DC)", "DA / (DB / DC)"), + () -> assertRewriteAfterSimplify("DA / (DB * DC)", "DA / (DB * DC)"), + + // Decimal arithmetic is intentionally unsupported by this rule and remains unchanged. + () -> assertRewriteAfterSimplify("MA / (MB / MC)", "MA / (MB / MC)")); + } + @Test void testSimplifyArithmeticRuleOnly() { executor = new ExpressionRuleExecutor(ImmutableList.of( @@ -71,10 +99,14 @@ void testSimplifyArithmeticRuleOnly() { assertRewriteAfterSimplify("IA - 2 - ((-IB - 1) - (3 + (IC + 4)))", "(((IA + IB) + IC) - ((((2 + 0) - 1) - 3) - 4))"); // multiply and divide - assertRewriteAfterSimplify("2 / IA / ((1 / IB) / (3 * IC))", "(((((2 / 1) * 3) / IA) * IB) * IC)"); - assertRewriteAfterSimplify("IA / 2 / ((IB * 1) / (3 / (IC / 4)))", "(((IA / IB) / IC) / (((2 * 1) / 3) / 4))"); - assertRewriteAfterSimplify("IA / 2 / ((IB / 1) / (3 / (IC * 4)))", "(((IA / IB) / IC) / (((2 / 1) / 3) * 4))"); - assertRewriteAfterSimplify("IA / 2 / ((IB / 1) / (3 * (IC * 4)))", "(((IA / IB) * IC) / (((2 / 1) / 3) / 4))"); + assertRewriteAfterSimplify("2 / IA / ((1 / IB) / (3 * IC))", + "((2 / IA) / ((1 / IB) / (IC * 3)))"); + assertRewriteAfterSimplify("IA / 2 / ((IB * 1) / (3 / (IC / 4)))", + "((IA / ((IB / (3 / (IC / 4))) * 1)) / 2)"); + assertRewriteAfterSimplify("IA / 2 / ((IB / 1) / (3 / (IC * 4)))", + "((IA / ((IB / (3 / (IC * 4))) / 1)) / 2)"); + assertRewriteAfterSimplify("IA / 2 / ((IB / 1) / (3 * (IC * 4)))", + "((IA / ((IB / (IC * (3 * 4))) / 1)) / 2)"); // hybrid // root is subtract @@ -88,7 +120,8 @@ void testSimplifyArithmeticRuleOnly() { assertRewriteAfterSimplify("-IA / 2 * ((-IB - 1) * (3 / (IC + 4)))", "((((0 - IA) * ((0 - 1) - IB)) / (IC + 4)) / (2 / 3))"); // root is divide assertRewriteAfterSimplify("(-IA / 2) / ((-IB - 1) - (3 + (IC + 4)))", "(((0 - IA) / (((((0 - 1) - 3) - 4) - IB) - IC)) / 2)"); - assertRewriteAfterSimplify("(-IA / 2) / ((-IB - 1) / (3 + (IC * 4)))", "((((0 - IA) / ((0 - 1) - IB)) * ((IC * 4) + 3)) / 2)"); + assertRewriteAfterSimplify("(-IA / 2) / ((-IB - 1) / (3 + (IC * 4)))", + "(((0 - IA) / (((0 - 1) - IB) / ((IC * 4) + 3))) / 2)"); // unsupported decimal assertRewriteAfterSimplify("-2 - MA - ((1 - IB) - (3 + IC))", "((-2 - MA) - ((1 - IB) - (3 + IC)))"); diff --git a/regression-test/data/nereids_rules_p0/expression/simplify_arithmetic/test_simplify_arithmetic.out b/regression-test/data/nereids_rules_p0/expression/simplify_arithmetic/test_simplify_arithmetic.out index e7c6a5a1dd5cd0..fa813f476a962b 100644 --- a/regression-test/data/nereids_rules_p0/expression/simplify_arithmetic/test_simplify_arithmetic.out +++ b/regression-test/data/nereids_rules_p0/expression/simplify_arithmetic/test_simplify_arithmetic.out @@ -1,3 +1,16 @@ -- This file is automatically generated. You should know what you did if you want to edit this -- !return_type_after_projection_should_be_bigint -- +-- !preserve_division_denominator_in_projection -- +0 \N +1 1 +2 2 + +-- !preserve_division_denominator_in_filter -- +0 + +-- !preserve_division_denominator_rule_disabled -- +0 \N +1 1 +2 2 + diff --git a/regression-test/suites/nereids_rules_p0/expression/simplify_arithmetic/test_simplify_arithmetic.groovy b/regression-test/suites/nereids_rules_p0/expression/simplify_arithmetic/test_simplify_arithmetic.groovy index bc1fc20ee8fe9b..a4f0e3cf234d39 100644 --- a/regression-test/suites/nereids_rules_p0/expression/simplify_arithmetic/test_simplify_arithmetic.groovy +++ b/regression-test/suites/nereids_rules_p0/expression/simplify_arithmetic/test_simplify_arithmetic.groovy @@ -36,4 +36,36 @@ suite("test_simplify_arithmetic") { qt_return_type_after_projection_should_be_bigint """ select -3 - (7 + id) as c1 from test_simplify_arithmetic group by c1 """ + + // A nested denominator is an evaluation boundary. In particular, rewriting this to + // number * 1 would change the number = 0 result from NULL to 0. + explain { + sql """ + select number, 1 / (1 / number) as result + from numbers("number" = "3") + """ + verbose true + contains """(1 / (1 / CAST(""" + } + + qt_preserve_division_denominator_in_projection """ + select number, 1 / (1 / number) as result + from numbers("number" = "3") + order by number + """ + + qt_preserve_division_denominator_in_filter """ + select number + from numbers("number" = "3") + where 1 / (1 / number) is null + order by number + """ + + sql "set disable_nereids_expression_rules='SIMPLIFY_ARITHMETIC'" + qt_preserve_division_denominator_rule_disabled """ + select number, 1 / (1 / number) as result + from numbers("number" = "3") + order by number + """ + sql "set disable_nereids_expression_rules=''" }