[fix](expr opt) Preserve division denominator boundaries - #67892
Open
morrySnow wants to merge 1 commit into
Open
[fix](expr opt) Preserve division denominator boundaries#67892morrySnow wants to merge 1 commit into
morrySnow wants to merge 1 commit into
Conversation
### 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
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
8 tasks
Contributor
FE UT Coverage ReportIncrement line coverage |
morrySnow
changed the title
Contributor
TPC-H: Total hot run time: 16768 ms |
Contributor
TPC-DS: Total hot run time: 82062 ms |
Contributor
ClickBench: Total hot run time: 14.64 s |
hello-stephen
pushed a commit
that referenced
this pull request
Sep 12, 2026
…67897) Since today every `Doris_DorisCloudRegression_VaultP0` run dies in the `run` step before executing a single test, e.g. #67883 (TeamCity build 39010) and #67881 / #67882 / #67885 / #67886 / #67892 / #67893: ``` doris-external--minio Pulling doris-external--minio Error Error response from daemon: pull access denied for minio/minio, repository does not exist or may require 'docker login': denied: requested access to the resource is denied ERROR: start minio docker twice failed ``` MinIO stopped publishing container images in October 2025 (the project is a source-only distribution now, see minio/minio#21647) and the `minio/minio` and `minio/mc` repositories have since been removed from Docker Hub altogether (`https://hub.docker.com/v2/repositories/minio/minio/` answers 404, same for `minio/mc`). The few VaultP0 runs that still pass do so only on agents that have the image cached locally (their logs have no `Pulling` line). The iceberg, hudi and polaris third-party fixtures, `test_file_cache_warmup_read_metrics_docker` (which runs a `docker run minio/minio` itself), the all-in-one `cloud.yml` and the datalake samples reference the same images and are one cache eviction away from the same failure. `quay.io/minio/minio` and `quay.io/minio/mc` still serve every tag we use -- `RELEASE.2024-11-07T00-52-20Z`, `RELEASE.2025-01-20T14-49-07Z`, mc `RELEASE.2025-01-17T23-25-50Z`, the two 2022 tags of the samples and `latest` -- and MinIO keeps pushing hotfix tags there (latest one dated 2026-04). `docker manifest inspect` resolves all of them (amd64 / arm64 / ppc64le). So every reference gets the `quay.io/` prefix and the tags stay exactly as they were: same builds, different registry. The CI agents already pull from quay.io for the OceanBase fixture. A longer-term option is to mirror these three tags into the project's own `doristhirdpartydocker` namespace, which already hosts hive / zookeeper / kafka / trinodb; that needs someone with push access to that Docker Hub organization and can follow separately.
starocean999
approved these changes
Sep 12, 2026
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.
Review summary
Request changes: the patch fixes the direct denominator-descendant escape demonstrated by 1 / (1 / number), but one deterministic DOUBLE wrong-result path remains when the existing constant/variable bucketing moves a surrounding factor across the new atomic denominator. The blocking case is described inline. Two review rounds converged with no other distinct finding.
Critical checkpoints
- Goal and correctness: the new guard correctly keeps descendants of
x / (y / z)andx / (y * z)out of the enclosing flatten, and the regression test proves the primary zero/NULL reproduction. The goal is not fully achieved because the intact denominator can still move relative to a surrounding constant, changing overflow, underflow, and rounding behavior. - Scope and focus: the four-file change is otherwise small and focused. No additional user-provided focus was supplied.
- Concurrency: not involved; the rule operates on per-query immutable expression trees and adds no shared state or locking.
- Lifecycle/static initialization: not involved; no resources, callbacks, globals, or cross-TU initialization are added.
- Configuration: no production configuration is added or changed. The regression suite temporarily disables the existing rule and restores the session value.
- Compatibility: no function symbol, storage format, journal, RPC, or FE/BE protocol changes are present; rolling-upgrade compatibility is unaffected.
- Parallel paths and stage placement: the existing
SIMPLIFY_ARITHMETICregistration and normalization stage are unchanged. Integral coercion casts, homogeneous DOUBLE trees, multiply/divide denominators, left-associated division, decimals, zero, NULL, and fixed-point revisits were traced. No rewrite loop was found. - Conditional logic: the new denominator-polarity check is clearly commented, but treating only the subtree contents as atomic is insufficient unless its encounter position is also preserved.
- Test coverage: the JUnit cases and analyzer-backed regression correctly fail on the buggy parent for the reported nested-division case and cover deterministic projection/filter outputs, a disabled-rule baseline, DOUBLE shapes, decimal no-op behavior, and a left-associated control. They do not cover the submitted surrounding-factor case.
- Test results: the checked-in golden rows are correct and ordered. Per the review-runner contract I did not build or run tests; author commands were not independently rerun. At exact head
0290c8ca516c41068278fe94b4af07c0bdb0e98c, FE UT, compile, CheckStyle, P0/nonconcurrent regressions, and performance are reported passing.check_coverage_feandvault_p0report failure with authenticated logs unavailable; the coverage bot separately reports 100% incremental line coverage. - Observability: no new runtime service or operational path is introduced, so logs or metrics are not needed.
- Transactions, persistence, and data writes: not involved; no EditLog, visibility, transaction, storage, or crash-recovery behavior changes.
- FE/BE propagation: no new variable or metadata is transmitted. FE type/nullability remains consistent with the existing BE DOUBLE operators.
- Performance and memory: the early return reduces traversal work and adds no material allocation or memory-accounting obligation. No additional performance issue was found.
- Other invariants: no error-handling, security, authorization, or data-lifecycle concern applies beyond the submitted expression-correctness issue.
Review status: complete static review at the exact supplied head; all candidate points were verified, deduplicated, and resolved into the single inline request below.
| // 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. |
Contributor
There was a problem hiding this comment.
[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.
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
4 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.
Problem
Arithmetic simplification could change query results by moving factors across a nested division denominator. In particular, zero and nullable inputs could produce a non-NULL value after rewriting.
Root cause
The multiply/divide flattening traversal propagated the outer denominator polarity into the denominator subtree. It then inverted or moved the subtree multiplication and division operands into the enclosing expression, even though a denominator is an evaluation boundary.
Reproduction
1 / (1 / number) was rewritten to number * 1. For number = 0, the original expression evaluates to NULL while the rewritten expression evaluates to 0. The same unsafe flattening affected shapes such as x / (y / z) and x / (y * z).
Fix
Keep every complete subtree reached in denominator position as an atomic operand during multiply/divide flattening. The normal recursive rewrite still simplifies within that subtree, but no factor can be inverted or moved across its enclosing division boundary. Add focused unit coverage for integer, nullable, double, decimal, and zero cases, plus end-to-end plan and result checks.
Tests