Repository navigation
[fix](eager-agg) Disable bilateral eager aggregation for decimal SUM - #69093
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
TPC-H: Total hot run time: 29406 ms |
TPC-DS: Total hot run time: 153196 ms |
ClickBench: Total hot run time: 24.52 s |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed PR 69093 at exact head 2066895 against base 0027350. The changed eager-aggregation path introduces DecimalMultiplyForSum to preserve a decimal SUM's type while restoring join multiplicity. I traced the FE coercion path, DecimalV3 width/scale behavior, ArithmeticExpr translation, BE decimal multiply dispatch and overflow handling, expression visitor/rewrite consumers, and eager-aggregation count propagation across inner/cross/outer/semi/anti/nested/union paths. I found no concrete correctness defect and therefore have no new inline findings. The Nereids expression legality checkpoint is satisfied because this change adds no checkLegalityBeforeTypeCoercion overload; the FE locking, RPC exception, and visible-version checkpoints are not applicable to these files. There were no user-provided focus points. Live review and inline-comment refresh found no existing P0/P1 inline findings to carry forward. This was a static review only; no builds or tests were run by this invocation. The review is delivered with converged=false because all assigned delegated passes terminated with the same temporary model-capacity error before returning NO_NEW_VALUABLE_FINDINGS; the main-agent changed-file and risk sweep is complete, but delegated unanimity could not be established.
Review did not converge after 3 rounds. This automated review cannot pass the review check.
Three required delegated review passes were launched and recovery attempts were made, but every child process terminated with a model-capacity error before returning a result. The main-agent review and final changed-file/risk sweep are complete with no verified findings, but the contract's all-subagents NO_NEW_VALUABLE_FINDINGS convergence condition is unproven.
924060929
left a comment
924060929
left a comment
There was a problem hiding this comment.
Eager aggregation restores join multiplicity by multiplying partial SUM values by opposite-side counts. This PR introduces an internal multiplication expression to preserve the decimal SUM type and avoid scale reduction.
I independently reviewed all six changed files, FE coercion and rewrite consumers, statistics, translation, and the necessary BE multiply/SUM boundary at head 2066895. One blocking P2 remains: enforcing the final SUM range on each compensation product rejects valid queries whose positive and negative contributions cancel in the final aggregate. The added tests do not cover cancellation across distinct join keys. Please preserve the required intermediate range or safely decline that rewrite.
The supplied snapshot records successful P0 Regression, FE UT and compilation, and an approval by englefly on this head. The earlier AI review reported no findings but failed its convergence requirement. These records do not resolve the confirmed functional issue. Review scope is FE plus the relevant FE/BE contract; no builds or tests were run.
Automated FE-focused static review (gpt-6.1-sol, xhigh). No build or tests were run.
| @Override | ||
| public DecimalV3Type getDataTypeForDecimalV3(DecimalV3Type sumType, DecimalV3Type countType) { | ||
| Preconditions.checkArgument(countType.getScale() == 0); | ||
| return sumType; |
There was a problem hiding this comment.
[P2] Preserve intermediate range until the final SUM
With eager_aggregation_mode=1, enable_decimal256=false, check_overflow_for_decimal=true and decimal_overflow_scale=6, consider l(g,k,v DECIMAL(38,18)) containing (G,a,+60000000000000000000) and (G,b,-60000000000000000000), and r(k,w) containing two rows with w=1 for each key. SELECT l.g,SUM(l.v),SUM(r.w) FROM l JOIN [shuffle] r ON l.k=r.k GROUP BY l.g should sum to zero. Returning sumType makes each compensation product ±120000000000000000000 undergo the DECIMAL(38,18) range check in BE multiply.cpp:487, throwing before cancellation in the final aggregate. The products still fit Int128; the merge-base multiplication rescales to DECIMAL(38,6) and this case can complete. Preserve sufficient intermediate range through aggregation, or decline the rewrite when that cannot be guaranteed, and add a cancellation regression. This is a static finding; no tests were run.
|
/review |
### What problem does this PR solve? Related PR: apache#69093 Problem Summary: Bilateral eager aggregation restores join multiplicity with partial SUM times the opposite-side count. Ordinary decimal multiplication can reduce scale, while forcing the SUM type can reject intermediate products that would cancel in the final aggregate. Reject bilateral candidates containing a DecimalV3 SUM before changing child plans. Keep single-side decimal pushdown and integer bilateral pushdown, and remove the special decimal multiply expression. ### Release note Avoid decimal SUM precision loss and premature compensation overflow by skipping bilateral eager aggregation for DecimalV3 SUM. ### Check List (For Author) - Test: FE Checkstyle (0 violations); remote build.sh --fe; EagerAggRewriterTest (32 tests passed); decimal_sum_multiplicity and bilateral_eager_agg regression suites passed on the updated dev FE. Regression expectations were generated by the test script using the eager-aggregation-disabled baseline. - Behavior changed: Yes; decimal SUM no longer supports bilateral eager aggregation - Does this need documentation: No
7c2bc12
2066895 to
7c2bc12
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed head 7c2bc12dedb2846fc6ca3fc6225d36ac528a18d7. No new actionable findings. The static review converged in round 1: both complete-review agents and the separate decimal-risk agent returned NO_NEW_VALUABLE_FINDINGS; all candidates and risk focuses have evidence-backed dispositions.
The current implementation declines bilateral eager aggregation when a decimal SUM is present. For Aggregate(g; SUM(l.v), SUM(r.w)) -> Inner/CrossJoin(L, R), decideJoinPushSide returns before child contexts, partial aggregates or compensation products are constructed. One-side decimal SUM remains eligible because the untouched opposite branch has no compensation count in the fresh per-aggregate state.
Critical checkpoint conclusions:
- Goal and correctness: the guard avoids scale reduction and premature range checks from decimal compensation. Added plan assertions and functional cases target both failures; inspected, not executed.
- Scope and reuse: the production change is one import and a focused guard using the existing conservative exit and SUM type machinery.
- Concurrency and locks: no new shared state, threads or locks; rewrite state remains local to each aggregate invocation.
- Lifecycle and initialization: no new resource ownership, cleanup obligations or static initialization dependencies.
- Configuration: no new options; tests exercise existing eager, decimal256 and decimal-overflow controls.
- Compatibility: no changed function symbols, storage formats or wire contracts; existing BE aggregate/expression handling is retained.
- Thrift/protobuf: no definitions changed; cross-branch field-ID checks are inapplicable.
- Parallel paths: reviewed one-side/bilateral, nested joins, projects, filters, UNION ALL, IF decomposition, DISTINCT, COUNT/MIN/MAX, constants, hints and the broadcast shortcut. Existing outer/semi/anti/mark/asof protections remain. Legacy eager exploration classes are not registered in the production rule set. No bypass substantiated.
- Conditional checks: the comment explains both scale loss and cancellation overflow. The guard runs before state mutation and cannot be overridden by push hints or statistics.
- Test coverage: unit cases cover INNER/CROSS joins, aggregate order, swapped sides, COUNT, expression grouping and decimal128/256 types, with positive one-side pushdown controls. Regression cases cover aliases, unaliased output, INSERT OVERWRITE, tiny/negative/NULL values, overflow settings and cancellation.
- Expected results: all 28 golden blocks and tags checked against exact decimal arithmetic and SQL multiplicity; no incorrect result found. Ordering, pre-drops, table naming and session cleanup follow the existing harness conventions.
- Observability: the change only declines an optimization; existing explain plans and error reporting suffice.
- Transactions and persistence: no EditLog, replay, failover or transaction implementation changes.
- Data writes and atomicity: engine write paths are unchanged; INSERT/OVERWRITE appears only in the regression test.
- FE-BE variable passing: no new variables or serialization paths; allowed aggregate type/intermediate semantics remain unchanged.
- Performance: adds a linear aggregate-function check during planning. Bilateral decimal SUM optimization is deliberately disabled for correctness; no separate performance defect established.
- Other optimizer invariants: rule registration/stage order, ExprIds, nullability, mark-join semantics and physical property derivation are unchanged; the new exit creates no rewrite loop.
Existing thread: P2 comment 4439965509 is addressed at this head. The earlier DecimalMultiplyForSum approach is absent and the unsafe cancellation rewrite is declined; a cancellation regression is included. No duplicate inline comment is needed, and no existing P0/P1 finding remains to carry forward.
User focus: none was supplied; the entire four-file PR was reviewed. No unresolved code candidates or uncovered assigned scope remain. Limits: static inspection only; no builds, tests or runtime validation were run, and actual golden-file generation provenance was not verified.
Review converged after 1 round(s).
TPC-H: Total hot run time: 28430 ms |
TPC-DS: Total hot run time: 154499 ms |
ClickBench: Total hot run time: 24.48 s |
### What problem does this PR solve? Related PR: apache#69093 Problem Summary: Disabling bilateral eager aggregation for DecimalV3 SUM removes pre-aggregation from the a, sum_min_max and count_sum_mixed shape plans. Refresh the expected plans with the regression runner. All query result tags are unchanged. ### Release note None ### Check List (For Author) - Test: Regression test; run load, generate eager_agg expectations with -forceGenOut, then rerun eager_agg without generation on dev - Behavior changed: No; test expectations reflect the existing decimal SUM restriction - Does this need documentation: No
FE UT Coverage ReportIncrement line coverage |
|
run buildall |
TPC-H: Total hot run time: 28484 ms |
TPC-DS: Total hot run time: 154039 ms |
ClickBench: Total hot run time: 24.34 s |
|
run cloud_p0 |
feiniaofeiafei
changed the title
|
/review |
There was a problem hiding this comment.
Static review of PR #69093 at head 1249469 converged in round 1. The DecimalV3 SUM guard in EagerAggRewriter runs before child-context and canonical-project construction, so bilateral eager aggregation is skipped for every admitted decimal Sum while one-sided pushdown remains. I traced DecimalV2/DecimalV3 resolution, decomposed SUM(IF(...)), project/union propagation, distinct/Sum0 exclusions, and nested BilateralState count slots; no correctness issue was found. Existing P2 inline comment 4439965509 is addressed by the current head's removed DecimalMultiplyForSum path and is not reposted. The changed FE and regression tests and generated plan outputs were inspected and cover the guarded two-sided cases, one-sided preservation, Decimal256 and overflow settings, aliases, nulls, multiplicity, and cancellation. No user-provided focus points were supplied. Nereids review checkpoints found no new rule-registration, property-derivation, locking, exception, or RPC path. No builds or tests were executed because the review contract is static-only; validation is inspected evidence. The final sweep found only a non-actionable trailing blank line in a generated .out file. No new inline findings.
Review converged after 1 round(s).
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #63690
Problem Summary:
Bilateral eager aggregation can change the result type and scale of a decimal SUM because its compensation uses ordinary decimal multiplication. This can cause an output type mismatch or silently lose precision; for example, a valid result of
0.0000000004can become0.000000.This PR disables bilateral eager aggregation when any aggregate is a DecimalV3 SUM. Single-sided decimal SUM pushdown remains supported.
This causes either:
For example, a valid SUM of
0.0000000004could become0.000000.This Pr fix this problem:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)