[fix](simplify agg) SimplifyAggGroupBy should verify injectivity#64335
Open
yujun777 wants to merge 2 commits into
Open
[fix](simplify agg) SimplifyAggGroupBy should verify injectivity#64335yujun777 wants to merge 2 commits into
yujun777 wants to merge 2 commits into
Conversation
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>
yujun777
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
1 similar comment
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29419 ms |
Contributor
TPC-DS: Total hot run time: 169732 ms |
Contributor
TPC-H: Total hot run time: 28739 ms |
Contributor
TPC-DS: Total hot run time: 169104 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
1 similar comment
Contributor
FE UT Coverage ReportIncrement line coverage |
morrySnow
reviewed
Jun 10, 2026
| } | ||
|
|
||
| @VisibleForTesting | ||
| protected static boolean isLosslessWidening(DataType src, DataType tgt) { |
Contributor
There was a problem hiding this comment.
in #64080, DataType add a new interface isInjectiveCastTo, i think we could reuse it here
Contributor
FE Regression Coverage ReportIncrement line coverage |
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>
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29113 ms |
Contributor
TPC-DS: Total hot run time: 169787 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
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.
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.
New
isLosslessWidening(src, tgt): type-family-aware widening checkusing
width()for integral/float, andDecimalV3Type.forType()fordecimal range/scale comparison.
Changes
SimplifyAggGroupBy.java: +80 lines, rewritten core logicExpressionUtils.java: -35 lines, removed unusedisSlotOrCastOnSlot/extractSlotOrCastOnSlotSimplifyAggGroupByTest.java: +216 lines, 25 tests covering all new paths