Uh oh!
There was an error while loading. Please reload this page.
[fix](simplify agg) SimplifyAggGroupBy should verify injectivity - #64335
Merged
Conversation
yujun777
requested review from
924060929, englefly, morrySnow and starocean999
as code ownersJune 10, 2026 01:09
hello-stephen
commented
Jun 10, 2026
Contributor
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
yujun777
commented
Jun 10, 2026
ContributorAuthor
run buildall |
1 similar comment
yujun777
commented
Jun 10, 2026
ContributorAuthor
run buildall |
hello-stephen
commented
Jun 10, 2026
Contributor
TPC-H: Total hot run time: 29419 ms |
hello-stephen
commented
Jun 10, 2026
Contributor
TPC-DS: Total hot run time: 169732 ms |
hello-stephen
commented
Jun 10, 2026
Contributor
TPC-H: Total hot run time: 28739 ms |
hello-stephen
commented
Jun 10, 2026
Contributor
TPC-DS: Total hot run time: 169104 ms |
hello-stephen
commented
Jun 10, 2026
Contributor
FE UT Coverage ReportIncrement line coverage |
1 similar comment
hello-stephen
commented
Jun 10, 2026
Contributor
FE UT Coverage ReportIncrement line coverage |
| } | ||
| @VisibleForTesting | ||
| protected static boolean isLosslessWidening(DataType src, DataType tgt) { |
hello-stephen
commented
Jun 10, 2026
Contributor
FE Regression Coverage ReportIncrement line coverage |
yujun777
commented
Jun 10, 2026
ContributorAuthor
run buildall |
hello-stephen
commented
Jun 10, 2026
Contributor
TPC-H: Total hot run time: 29113 ms |
hello-stephen
commented
Jun 10, 2026
Contributor
TPC-DS: Total hot run time: 169787 ms |
hello-stephen
commented
Jun 10, 2026
Contributor
FE Regression Coverage ReportIncrement line coverage |
The rule simplified GROUP BY f(x) to GROUP BY x without verifying that f(x) is injective (one-to-one). This caused wrong results when: - literal is NULL (any op: a+NULL → always NULL) - Multiply/Divide by zero (a*0 → always 0) - Multiply/Divide with float/double operands (precision loss) Also adds proper handling of implicit lossless widening casts (integral→integral, float→double, integral→decimal, decimal→decimal) and removes the now-unused ExpressionUtils.extractSlotOrCastOnSlot. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Float/double precision loss affects Add/Subtract as well as Multiply/Divide (e.g., 1e16 + 1.0 = 1e16 in DOUBLE). Reject all float/double arithmetic uniformly. Decimal ops are allowed as precision overflow is too extreme to worry about in practice. Remove the now-unused FractionalType widening case. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…njectiveCastTo Replace the hand-rolled isLosslessWidening helper in SimplifyAggGroupBy with the existing DataType.isInjectiveCastTo, which provides the same injectivity check and is already used across the codebase. Key changes: - Replace isLosslessWidening call in canExtractSlot with isInjectiveCastTo - Remove isLosslessWidening method and its DataType/DecimalV3Type/IntegralType imports - Update tests to call isInjectiveCastTo directly on DataType Unit Test: - SimplifyAggGroupByTest: 29 tests passed
yujun777force-pushed
the
fix-simplify-agg-group-by
branch
from
June 30, 2026 08:17
f5f80c1 to
1ab195cCompareyujun777
commented
Jun 30, 2026
ContributorAuthor
run buildall |
hello-stephen
commented
Jun 30, 2026
Contributor
TPC-H: Total hot run time: 29848 ms |
hello-stephen
commented
Jun 30, 2026
Contributor
TPC-DS: Total hot run time: 173943 ms |
hello-stephen
commented
Jun 30, 2026
Contributor
ClickBench: Total hot run time: 25.2 s |
morrySnow
left a comment
Contributor
There was a problem hiding this comment.
Thanks for this fix! The changes to reject NULL literal, Multiply/Divide by zero, float/double types, and lossy CASTs are all correct and important. However, I found one significant gap: integer division with a non-zero literal is also not injective, but the checkLiteral only rejects Divide when the literal is zero. Please see the inline comments for details.
Summary
- Integer division not fully addressed:
a / C(C ≠ 1, -1) andC / afor any C are not injective for integer types due to truncation.GROUP BY a/2, a/3would be incorrectly simplified toGROUP BY a. - Missing integration test for the "divide without bare slot" scenario.
- Minor:
checkLiteralparameter type should beBinaryArithmeticinstead ofExpressionfor clarity. - Removal of
isSlotOrCastOnSlot/extractSlotOrCastOnSlot: Verified these methods are only called fromSimplifyAggGroupBy.java, so removal is safe.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…cate Javadoc Key changes: - checkLiteral: Expression expr -> BinaryArithmetic expr, with explicit cast at call site - ExpressionUtils: remove duplicate /** left by deleted methods Unit Test: - SimplifyAggGroupByTest: 29 tests passed
yujun777
commented
Jul 1, 2026
ContributorAuthor
run buildall |
morrySnow
approved these changes
Jul 1, 2026
Contributor
PR approved by at least one committer and no changes requested. |
hello-stephen
commented
Jul 1, 2026
Contributor
TPC-H: Total hot run time: 29521 ms |
hello-stephen
commented
Jul 1, 2026
Contributor
TPC-DS: Total hot run time: 174524 ms |
hello-stephen
commented
Jul 1, 2026
Contributor
ClickBench: Total hot run time: 25.44 s |
yujun777
commented
Jul 1, 2026
ContributorAuthor
run external |
hello-stephen
commented
Jul 1, 2026
Contributor
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
hello-stephen
commented
Jul 1, 2026
Contributor
FE Regression Coverage ReportIncrement line coverage |
morrySnow
commented
Jul 1, 2026
Contributor
/review |
Contributor
There was a problem hiding this comment.
Automated review summary for PR 64335:
I reviewed the current head acb363442f9eb15d249cf66d52bd55a64712ca01 against the three changed files and the existing inline review threads. I did not find a new distinct issue worth an additional inline comment in this pass.
Critical checkpoint conclusions:
- Goal and tests: The PR tightens
SimplifyAggGroupByso it only removes group keys for accepted arithmetic/cast shapes that expose one underlying slot, and it adds helper plus PlanChecker coverage. Existing threads already cover the remaining notable test gap around analyzed cast-path integration. - Scope: The change is focused on the rewrite rule, removal of now-unused
ExpressionUtilshelpers, and targeted FE tests. - Concurrency, lifecycle, config, compatibility, persistence, and write paths: Not involved by this PR.
- Parallel paths: The rule runs in the analyzer before
NormalizeAggregate; I did not find a distinct join or aggregate implementation-path issue beyond the existing threads. - Arithmetic semantics: I rechecked the current rule against divide/type-coercion behavior and BE divide handling. The remaining arithmetic overflow, finite-scale decimal divide, integer divide, and skipped-evaluation concerns are already represented in existing review threads, so I did not duplicate them.
- Test and style validation: I did not run FE tests or checkstyle because this checkout is missing
thirdparty/installedandthirdparty/installed/bin/protoc, andfe/AGENTS.mdsays to stop FE build work when that prerequisite is missing.
User focus: no additional review focus was provided.
Subagent conclusions: optimizer-rewrite and tests-session-config both completed focused passes with no candidate findings. Final convergence round 1 reviewed the current ledger and empty proposed inline-comment set, and both subagents replied NO_NEW_VALUABLE_FINDINGS.
Uh oh!
There was an error while loading. Please reload this page.
github-actionsBot
pushed a commit
that referenced
this pull request
Jul 1, 2026
) ## Problem `SimplifyAggGroupBy` simplified `GROUP BY f(x)` to `GROUP BY x` without verifying that `f(x)` is injective (one-to-one). This caused wrong results: | Expression | Why wrong | |---|---| | `a * 0` / `0 * a` | always evaluates to 0 — all rows fall into one group | | `0 / a` | always evaluates to 0 | | `a / 0` | division by zero | | `a + NULL` / `a * NULL` / ... | always evaluates to NULL | | `a * 0.1` with float/double | precision loss may map different inputs to same result | ## Fix 1. **`isBinaryArithmeticSlot`**: restructured to separate slot-expr from literal, then validate each independently. Float/double check runs early, before slot extraction. 2. **New `checkLiteral(expr, literal)`**: rejects NULL literal and Multiply/Divide by zero. 3. **New `canExtractSlot(expr)`**: replaces the old unconditional `extractSlotOrCastOnSlot` — only accepts bare `Slot` or implicit lossless widening casts (integral→integral, float→double, integral→decimal, decimal→decimal). Range and scale are compared directly for correctness. ## Changes - `SimplifyAggGroupBy.java`: +80 lines, rewritten core logic - `ExpressionUtils.java`: -35 lines, removed unused `isSlotOrCastOnSlot` / `extractSlotOrCastOnSlot` - `SimplifyAggGroupByTest.java`: +216 lines, 25 tests covering all new paths --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
github-actionsBot
pushed a commit
that referenced
this pull request
Jul 1, 2026
) ## Problem `SimplifyAggGroupBy` simplified `GROUP BY f(x)` to `GROUP BY x` without verifying that `f(x)` is injective (one-to-one). This caused wrong results: | Expression | Why wrong | |---|---| | `a * 0` / `0 * a` | always evaluates to 0 — all rows fall into one group | | `0 / a` | always evaluates to 0 | | `a / 0` | division by zero | | `a + NULL` / `a * NULL` / ... | always evaluates to NULL | | `a * 0.1` with float/double | precision loss may map different inputs to same result | ## Fix 1. **`isBinaryArithmeticSlot`**: restructured to separate slot-expr from literal, then validate each independently. Float/double check runs early, before slot extraction. 2. **New `checkLiteral(expr, literal)`**: rejects NULL literal and Multiply/Divide by zero. 3. **New `canExtractSlot(expr)`**: replaces the old unconditional `extractSlotOrCastOnSlot` — only accepts bare `Slot` or implicit lossless widening casts (integral→integral, float→double, integral→decimal, decimal→decimal). Range and scale are compared directly for correctness. ## Changes - `SimplifyAggGroupBy.java`: +80 lines, rewritten core logic - `ExpressionUtils.java`: -35 lines, removed unused `isSlotOrCastOnSlot` / `extractSlotOrCastOnSlot` - `SimplifyAggGroupByTest.java`: +216 lines, 25 tests covering all new paths --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Closed
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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
SimplifyAggGroupBysimplifiedGROUP BY f(x)toGROUP BY xwithoutverifying that
f(x)is injective (one-to-one). This caused wrong results:a * 0/0 * a0 / aa / 0a + NULL/a * NULL/ ...a * 0.1with float/doubleFix
isBinaryArithmeticSlot: restructured to separate slot-expr from literal,then validate each independently. Float/double check runs early, before
slot extraction.
New
checkLiteral(expr, literal): rejects NULL literal andMultiply/Divide by zero.
New
canExtractSlot(expr): replaces the old unconditionalextractSlotOrCastOnSlot— only accepts bareSlotor implicit losslesswidening casts (integral→integral, float→double, integral→decimal,
decimal→decimal). Range and scale are compared directly for correctness.
Changes
SimplifyAggGroupBy.java: +80 lines, rewritten core logicExpressionUtils.java: -35 lines, removed unusedisSlotOrCastOnSlot/extractSlotOrCastOnSlotSimplifyAggGroupByTest.java: +216 lines, 25 tests covering all new paths