Skip to content

[fix](expr opt) Preserve division denominator boundaries #67892

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
morrySnow wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[fix](expr opt) Preserve division denominator boundaries #67892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions ...src/main/java/org/apache/doris/nereids/rules/expression/rules/SimplifyArithmeticRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,14 @@ private static List<Operand> 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<Operand> 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.

@github-actions github-actions Bot Sep 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Preserve operand order around the atomic denominator

This guard keeps the denominator's descendants together, but process() can still move a surrounding constant across the intact divisor. For the analyzed DOUBLE tree (D / 2.0) / (B / C), doFlatten now yields [+D, -2.0, -(B / C)]; the variable/constant partition then rebuilds (D / (B / C)) / 2.0 (the changed expectations at lines 104-109 encode the same relocation). With D = Double.MAX_VALUE, B = 1.0, and C = 2.0, the original returns Double.MAX_VALUE, while the rewritten tree overflows at D / 0.5 and returns infinity. Please make an atomic negative operand an ordering barrier, or otherwise preserve factors on their original side, and cover this surrounding-factor case.

result.add(Operand.of(false, expr));
return;
}
BinaryArithmetic arithmetic = null;
Predicate<Expression> isPositiveArithmetic = isAddOrSub
? TypeUtils::isAdd : TypeUtils::isMultiply;
Expand Down
43 changes: 38 additions & 5 deletions ...e/src/test/java/org/apache/doris/nereids/rules/expression/SimplifyArithmeticRuleTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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
Expand All @@ -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)))");
Expand Down
13 changes: 13 additions & 0 deletions ...on-test/data/nereids_rules_p0/expression/simplify_arithmetic/test_simplify_arithmetic.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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

32 changes: 32 additions & 0 deletions ...st/suites/nereids_rules_p0/expression/simplify_arithmetic/test_simplify_arithmetic.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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=''"
}
Loading