Uh oh!
There was an error while loading. Please reload this page.
[opt](aggregate) eliminate FD-redundant group-by keys via ANY_VALUE wrapping - #64849
Merged
Conversation
hello-stephen
commented
Jun 25, 2026
Contributor
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
englefly
commented
Jun 25, 2026
ContributorAuthor
run buildall |
englefly
commented
Jun 25, 2026
ContributorAuthor
/review |
Contributor
There was a problem hiding this comment.
Review result: request changes.
Key checkpoints:
- Goal and tests: the PR targets FD-redundant GROUP BY key removal with ANY_VALUE for output-needed keys, but the implementation is incomplete for the existing PK/FK aggregate-pushdown caller, and the new unit test can fail before proving the rewrite.
- Scope/focus: the change is small, but it changes a public optimizer helper contract and a shared aggregate output shape.
- Parallel paths: PushDownAggThroughJoinOnPkFk still consumes findCanBeRemovedExpressions() using the old contract and is not updated for wrap-only keys.
- ExprId/lifecycle: the direct rewrite creates any_value(slot#x) AS slot#x, which adjacent Nereids code documents as an invalid ExprId shape that requires a fresh ExprId plus upper-reference rewriting.
- Concurrency/config/storage/FE-BE protocol: not applicable.
- Test coverage: existing added coverage is not reliable as written; local FE test execution was not run because thirdparty/installed/bin/protoc and thrift are missing, which fe/AGENTS.md treats as a stop condition.
Subagent conclusions: optimizer-rewrite OPT-1 was merged into MAIN-1; tests-session-config TST-1 became MAIN-2; TST-2 was dismissed because LineLength is suppressed under /test/. Convergence round 1 ended with both live subagents reporting NO_NEW_VALUABLE_FINDINGS for the same MAIN-1/MAIN-2/MAIN-3 final comment set.
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.
hello-stephen
commented
Jun 25, 2026
Contributor
TPC-H: Total hot run time: 28399 ms |
hello-stephen
commented
Jun 25, 2026
Contributor
TPC-DS: Total hot run time: 172035 ms |
hello-stephen
commented
Jun 25, 2026
Contributor
ClickBench: Total hot run time: 25.15 s |
hello-stephen
commented
Jun 25, 2026
Contributor
FE Regression Coverage ReportIncrement line coverage |
engleflyforce-pushed
the
eliminateGroupByKey-fd
branch
2 times, most recently
from
June 26, 2026 09:19
1234484 to
886c8d0Compareenglefly
commented
Jun 26, 2026
ContributorAuthor
/review |
englefly
commented
Jun 26, 2026
ContributorAuthor
run buildall |
englefly
marked this pull request as ready for review
June 26, 2026 09:21
englefly
requested review from
924060929, morrySnow and starocean999
as code ownersJune 26, 2026 09:21
Contributor
There was a problem hiding this comment.
Review result: request changes.
Key checkpoints:
- Goal and tests: the PR now fixes the earlier direct ANY_VALUE ExprId/helper issues on current head, but the conversion to a single custom rewrite regresses the old filter-specific disable rule contract. The added unit test covers the happy-path custom visitor shape, but not production rule-id/session disabling.
- Scope/focus: the code change is small and focused on Nereids aggregate key elimination. The remaining issue is in the registration/session-control surface, not unrelated code.
- Parallel paths: PushDownAggThroughJoinOnPkFk now keeps the public helper behavior separate from wrap-only keys on current head; no new issue found there.
- ExprId/lifecycle: wrapped outputs now allocate a fresh alias ExprId and rewrite upper references, matching the EliminateGroupByKeyByUniform precedent.
- Concurrency/config/storage/FE-BE protocol: no concurrency, storage, persistence, or FE-BE protocol changes found. The applicable configuration/session checkpoint is not satisfied because ELIMINATE_FILTER_GROUP_BY_KEY remains a valid disable-rule name but no longer gates its old filter branch.
- Test coverage: local FE tests were not run because thirdparty/installed, thirdparty/installed/bin/protoc, and thirdparty/installed/bin/thrift are missing in this runner. Focused git diff --check over the changed PR files passed.
User focus: .code-review.Gwylvs/review_focus.txt contains no additional focus points.
Subagent conclusions: tests-session-config TSC-1 was accepted as MAIN-1 and became the inline comment. optimizer-rewrite OR-1 was merged as a duplicate of TSC-1. Final convergence round 1 ended with both live subagents reporting NO_NEW_VALUABLE_FINDINGS for the same current ledger/comment set containing MAIN-1 only.
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.
Uh oh!
There was an error while loading. Please reload this page.
englefly
commented
Jun 30, 2026
ContributorAuthor
run buildall |
hello-stephen
commented
Jun 30, 2026
Contributor
TPC-H: Total hot run time: 29680 ms |
hello-stephen
commented
Jun 30, 2026
Contributor
TPC-DS: Total hot run time: 173991 ms |
hello-stephen
commented
Jun 30, 2026
Contributor
ClickBench: Total hot run time: 25.39 s |
hello-stephen
commented
Jun 30, 2026
Contributor
FE Regression Coverage ReportIncrement line coverage |
englefly
commented
Jul 5, 2026
ContributorAuthor
run buildall |
…hildren to prevent ExprId mismatch when beforePushDownJobs rules wrap producer output with new slots
…inOnPkFk The ANY_VALUE rewrite (any_value(pk) -> any_value(fk)) used the primaryToForeignDeps map built from findBijectionSlots(), which only guarantees a bijective functional dependency, not value equality. For a bijection like unique_code <-> fk derived through a unique-key FD plus the pk = fk equality, any_value(unique_code) was rewritten to any_value(fk), silently changing results (e.g. 100/200 -> 1/2). Revert to the master behavior: aggs whose output contains any_value(primary column) are rejected by the final guard and the pk/fk pushdown is skipped (conservative and correct). The bijection map stays for GROUP BY replacement only; a follow-up can restore the any_value pushdown using DataTrait.calEqualSet for value-level swaps.
engleflyforce-pushed
the
eliminateGroupByKey-fd
branch
from
August 7, 2026 06:06
793758b to
2fa6818Compareenglefly
commented
Aug 7, 2026
ContributorAuthor
run buildall |
hello-stephen
commented
Aug 7, 2026
Contributor
TPC-H: Total hot run time: 29145 ms |
hello-stephen
commented
Aug 7, 2026
Contributor
TPC-DS: Total hot run time: 159259 ms |
hello-stephen
commented
Aug 7, 2026
Contributor
ClickBench: Total hot run time: 24.15 s |
englefly
commented
Aug 7, 2026
ContributorAuthor
run feut |
hello-stephen
commented
Aug 7, 2026
Contributor
FE UT Coverage ReportIncrement line coverage |
starocean999
approved these changes
Aug 15, 2026
Contributor
PR approved by at least one committer and no changes requested. |
englefly
commented
Aug 15, 2026
ContributorAuthor
/review |
Contributor
There was a problem hiding this comment.
Requesting changes for three blocking planner-correctness defects, reported in two inline comments: two independent scan paths can advertise invalid FDs that the new ANY_VALUE branch turns into wrong results, and lateral ON predicates can retain a removed aggregate-output ExprId and fail plan validation.
Review status: converged after three complete rounds of full-scope and risk-focused review. Every candidate was independently checked and deduplicated against the live PR discussions.
User focus: review_focus.txt supplied no additional focus, so I reviewed the complete PR and its upstream/downstream planner paths.
Critical checkpoint conclusions:
- Goal and proof: The intended optimization—remove FD-redundant group keys while preserving required outputs with
ANY_VALUE—works on the covered Project/Aggregate shapes, but the current code does not accomplish it safely because invalid scan traits can change results and incomplete ExprId propagation can invalidate a plan. Existing tests do not cover the three reported paths. - Scope and clarity: The core rule and expression-rewrite changes are reasonably localized, but the accompanying MV/MTMV disables and broad regression edits expand the validation surface. Existing live threads already cover the weak/disabled-test concerns, so I did not duplicate them.
- Concurrency: The planner rewrite and replacement map are statement/job-local; no new shared mutable state, threads, locks, or lock ordering were introduced. The MTMV changes only improve exception logging.
- Lifecycle/static initialization: The custom rewriter is constructed per job, and the inspected MTMV session-variable disable/restore paths restore state in
finally. No new static initialization or ownership lifecycle issue was found. - Configuration: No configuration item is added. Existing
read_mor_as_dup_tables,skip_delete_bitmap, and selected-index modes expose one reported bug because their uniqueness guards run after superclass constraints have already been imported. - Compatibility: This is FE-internal planner behavior; no function symbol, storage format, persisted serialization, or FE-BE wire contract changes were introduced, so no rolling-upgrade compatibility shim is needed.
- Parallel paths: Parallel/special paths were traced across CTEs, set operations, Repeat/distinct rewrites, MV pre-rewrite, selected indexes, raw-version reads, and lateral Generate nodes. The selected/raw trait leaks and the missed Generate conjunct carrier are the blocking gaps reported inline.
- Special conditions: The non-base-index and raw-read early returns are documented, but they do not suppress constraints already added by
super.computeUnique(). The condition ordering therefore does not enforce its stated invariant. - Test coverage: The PR adds unit and regression coverage for the main alias/CTE rewrite, but lacks executed result tests for partial selected-index constraints, raw MOR versions with an explicit constraint, and a grouped derived table feeding
JOIN LATERAL ... ON. Those tests are required for the reported fixes. - Modified test results: The changed query54 shape files were inspected and retain the same operator counts and scalar-result assertions; no separate result-file defect was substantiated. Weak assertions, disabled suites, and cleanup/isolation issues already have live review threads and were not repeated.
- Observability: The MTMV logging changes preserve the throwable and improve diagnosis. No new metric is needed for this statement-local rewrite; deterministic planner/result regressions are the appropriate guard for these failures.
- Transactions and persistence: No transaction, EditLog, replay, or persisted-metadata format is changed. MTMV create/cache/refresh rule fences and their state restoration were checked without finding a new mismatch.
- Data writes and crash safety: No production data-write, commit, or crash-recovery path is modified. The DML regression-suite edits do not alter runtime write semantics.
- FE-BE variables: No new variable or execution option is transmitted between FE and BE, so there are no sibling thrift/constant-folding/point-query propagation paths to update.
- Performance: Exact-head performance CI passed, and the changed query54 shapes retain the same high-level operator counts. I found no substantiated CPU, memory, or complexity regression beyond the blocking correctness failures.
- Other issues: After three rounds and a final changed-file sweep, no additional nonduplicate issue survived. The already-raised nullable outer-join FD concern is fenced by the merged prerequisite fix and was not duplicated here.
Exact-head CI reviewed: CheckStyle, compile, FE unit tests, P0, cloud, external, nonconcurrent, and performance passed. check_coverage_fe failed before tests because a Maven dependency download was truncated, so it provides no coverage signal. Per the review-runner contract, I did not run local builds or tests.
| removeExpression.add(entry.getKey()); | ||
| } else { | ||
| // Still needed in output: remove from group-by, wrap with ANY_VALUE in output | ||
| wrapWithAnyValue.add(entry.getKey()); |
Contributor
There was a problem hiding this comment.
[P1] Suppress invalid scan constraints before this branch
This added ANY_VALUE path turns two existing LogicalOlapScan.computeUnique() ordering gaps into wrong results because super.computeUnique() imports table constraints before the scan-specific guards run:
- A direct non-base index containing
a,cfor a table-levelUNIQUE(a,b)resolves only{a}infindSlotsByColumn(). The selected-index return then leaves that singleton advertised as unique, soGROUP BY a,cbecomesGROUP BY aplusANY_VALUE(c)and merges distinct(1,'x')/(1,'y')groups. - A MOR unique-key table with an explicit constraint on
kandread_mor_as_dup_tables='*'deliberately exposes versions(1,10),(1,20), and(1,30), but the raw-read return also leaves the superclassk -> vtrait intact. This branch collapses those three(k,v)groups to one.
Please suppress superclass constraints for raw-version reads and require every constrained column to be present before registering a constraint on a selected index, then add executed result regressions for both modes.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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
6 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?
When a group-by key is functionally dependent on another key (e.g. s_suppkey -> s_name via PK) but required in output, remove it from GROUP BY and wrap with ANY_VALUE().
Previously EliminateGroupByKey kept such keys in GROUP BY to preserve SQL semantics. Now they are replaced with ANY_VALUE wrappers in the output, allowing the group-by set to be minimized while keeping the column in SELECT.
Public findCanBeRemovedExpressions() API preserved for backward compatibility. Internal logic split into FindResult with separate removeExpression and wrapWithAnyValue sets.
Test: testEliminateByPkWithOutputNeeded verifies ANY_VALUE wrapping when SELECT contains an FD-redundant group-by key.
Issue Number: close #xxx
Related PR: #65982#66801#66803
上面 3 个 pr 是原有 master 的bug fix. pick 这个 pr 前, 确保上面 3 个 pr 已经 pick
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)