[fix](expr opt) Preserve division comparison semantics - #67895
Open
morrySnow wants to merge 1 commit into
Open
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
morrySnow
changed the title
morrySnow
marked this pull request as draft
September 12, 2026 03:34
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
Problem: Comparisons whose left side divides a value by a constant could return incorrect booleans or lose NULL results after expression simplification.
Root cause: The arithmetic comparison rule treated division as algebraically invertible and rewrote `x / c op r` as `x op r * c`. That equivalence does not hold under SQL numeric coercion, floating-point and decimal rounding, overflow, or division-by-zero semantics.
Reproduction: Division by zero should yield NULL, but the rewrite changed `x / 0 > 1` into `x > 0`. Finite non-zero counterexamples also differ at rounding boundaries, including DOUBLE `0.30000000000000004 / 3.0 > 0.1`, integer-to-DOUBLE `-100 / 11 > -9.090909090909092`, and DECIMAL `1 / 3 > 0.333333`.
Fix: Remove division from the inverse-rearrangement allowlist so the complete Divide expression remains on its original side of the comparison. Keep the existing additive and supported date/time rearrangements unchanged.
Tests: Add expression-rule unit coverage for zero representations, folded zero, NULL, non-finite values, finite rounding boundaries, negative divisors, and nested arithmetic. Add end-to-end plan and result regression coverage with the rule enabled and disabled.
### Release note
Fix incorrect comparison results and lost NULL values for division expressions with constant divisors.
### Check List (For Author)
- Test:
- Unit Test
- Regression test
- Full FE build and checkstyle
- Behavior changed: Yes. Division comparisons are kept intact unless a future rewrite can prove SQL-semantic equivalence.
- Does this need documentation: No
morrySnow
force-pushed
the
fix/reject-zero-divisor-rearrangement
branch
from
September 12, 2026 08:35
b58789b to
f8d6651
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 17053 ms |
Contributor
TPC-DS: Total hot run time: 83281 ms |
Contributor
ClickBench: Total hot run time: 14.89 s |
morrySnow
marked this pull request as ready for review
September 12, 2026 11:19
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review result: REQUEST_CHANGES.
The direct-root edit is locally correct: it stops the unsafe x / c op r inverse for every comparison kind, preserves analyzed Divide coercion/nullability, and the new unit/regression oracles are deterministic and pre-patch-sensitive for that narrow path. However, production normalization still changes query truth values through the three mechanisms described inline.
Critical checkpoint conclusions:
- Goal/proof: not achieved end to end; direct Divide roots are fixed and tested, but MF-1/MF-2/MF-3 remain production-reachable.
- Scope/clarity: the five-file patch is small and clear locally, but the functionally parallel predecessor rule and retained containing Add/Subtract path are not fenced.
- Concurrency/thread safety: not involved; no threads, shared mutable state, locks, atomics, or blocking work are added.
- Lifecycle/static initialization: not involved; no ownership, cleanup, circular-reference, release, or static-initialization behavior changes.
- Configuration: no product configuration is added; the test-only existing disabled-rule session setting targets the intended rule and is restored in
finally. - Compatibility: no symbols, storage formats, serialized metadata, RPCs, or rolling-upgrade surfaces change.
- Parallel paths: analyzer/coercion, production rule order, both
SimplifyArithmeticRulebranches, bottom-up re-entry, folding, and FE-to-BE evaluation were traced; the three inline mechanisms are the remaining gaps. - Conditions: direct-root exact-class gating and all comparison orientations are sound, but the new local condition does not protect division-containing parent/subtree shapes.
- Tests: direct zero/-0/NULL/folded/NaN/infinity/negative/finite-double/integer/decimal cases are broad; analyzed runtime negative cases for the three remaining wrapper/predecessor paths are missing.
- Test results: the changed
.outordering, column mapping, and enabled/disabled results are internally correct. No builds or tests were run in this review because the automation contract permits static review only. - Observability: no operational/distributed path is introduced; analyzed plan shapes and deterministic runtime result comparisons are the appropriate diagnostics.
- Transactions/persistence/data writes: not involved; no EditLog, versioning, MoW, transaction, write, or crash-atomicity behavior changes.
- FE/BE variables: no variable or protocol propagation is added.
- Error handling/memory safety/BE nullable handling: no new Status/exception, allocation/ownership, or BE column-nullability code is introduced; existing Divide nullability and zero-divisor NULL behavior were traced.
- Performance: preserving more division-containing trees may reduce simplification/pushdown, but that is the intended correctness tradeoff; no separate CPU, memory, or complexity issue was found.
- Other: all five changed files and all comparison/operator, wrapper, cast, decimal, special-value, and rule-reentry paths were swept; no fourth distinct issue remains.
Review convergence is complete after three rounds: every final-round reviewer returned NO_NEW_VALUABLE_FINDINGS, the finding set was stable, and no candidate remains unresolved. No additional user focus was provided.
| 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 |
Contributor
There was a problem hiding this 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.
| 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))"); |
Contributor
There was a problem hiding this 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
Problem: Comparisons whose left side divides a value by a constant could return incorrect booleans or lose NULL results after expression simplification.
Root cause: The arithmetic comparison rule treated division as algebraically invertible and rewrote
x / c op rasx op r * c. That equivalence does not hold under SQL numeric coercion, floating-point and decimal rounding, overflow, or division-by-zero semantics.Reproduction: Division by zero should yield NULL, but the rewrite changed
x / 0 > 1intox > 0. Finite non-zero counterexamples also differ at rounding boundaries, including DOUBLE0.30000000000000004 / 3.0 > 0.1, integer-to-DOUBLE-100 / 11 > -9.090909090909092, and DECIMAL1 / 3 > 0.333333.Fix: Remove division from the inverse-rearrangement allowlist so the complete Divide expression remains on its original side of the comparison. Keep the existing additive and supported date/time rearrangements unchanged.
Tests: Add expression-rule unit coverage for zero representations, folded zero, NULL, non-finite values, finite rounding boundaries, negative divisors, and nested arithmetic. Update the two existing positive- and negative-divisor assertions in
SimplifyArithmeticRuleTestto expect the division expression to remain in place. Add end-to-end plan and result regression coverage with the rule enabled and disabled. Local runs passed all 5SimplifyArithmeticRuleTesttests and all 3SimplifyArithmeticComparisonRuleTesttests.CI triage: The FE unit-test failure was the pair of stale division-rearrangement expectations corrected here. The other reported failures are infrastructure failures outside this code path: the cloud job could not install or invoke Maven, the performance job used FE image metadata newer than the tested binary supports, and the vault job could not pull the MinIO image.
Release note
Fix incorrect comparison results and lost NULL values for division expressions with constant divisors.
Check List (For Author)