Repository navigation
[improvement](fe) Prune partition keys from partition topn sort - #63379
Merged
Merged
Conversation
Member
Author
|
run buildall |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
/review |
CalvinKirs
force-pushed
the
range_partition_key_ck
branch
from
May 19, 2026 02:36
60ecee1 to
3fed697
Compare
Contributor
TPC-H: Total hot run time: 31096 ms |
Contributor
TPC-DS: Total hot run time: 169530 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
morrySnow
requested changes
May 25, 2026
| // partition key should on child tuple, sort key should on partition top's tuple | ||
| TupleDescriptor sortTuple = generateTupleDesc(partitionTopN.child().getOutput(), null, context); | ||
| final SortInfo sortInfo = getSortInfo(partitionTopN.getOrderKeys(), context, sortTuple); | ||
| List<OrderKey> orderKeys = prunePartitionKeys( |
Contributor
There was a problem hiding this comment.
should impl this feature in org.apache.doris.nereids.trees.plans.logical.LogicalWindow#pushPartitionLimitThroughWindow
CalvinKirs
force-pushed
the
range_partition_key_ck
branch
from
May 25, 2026 07:01
3fed697 to
fa14cc5
Compare
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31516 ms |
Contributor
TPC-DS: Total hot run time: 173195 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
FE UT Coverage ReportIncrement line coverage |
CalvinKirs
force-pushed
the
range_partition_key_ck
branch
from
May 28, 2026 09:48
fa14cc5 to
c9ea01e
Compare
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: PartitionTopN could keep partition key expressions in its executable order keys when the same key also appeared in the window ORDER BY. These keys are constant inside each partition, so keeping them leaks redundant order keys into BE SortInfo. This change prunes exact partition-key matches when LogicalWindow creates LogicalPartitionTopN, keeps PhysicalPartitionTopN executable order keys separate from its output order property, and leaves PhysicalPlanTranslator as a pure translator of physical plan fields.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- Unit Test: ./run-fe-ut.sh --run org.apache.doris.nereids.rules.implementation.ImplementationTest#physicalPartitionTopNSeparatesSortKeysFromOutputOrderKeys
- Unit Test: ./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.GeneratePartitionTopnFromWindowTest
- Unit Test: ./run-fe-ut.sh --run org.apache.doris.nereids.glue.translator.PhysicalPlanTranslatorTest#testPartitionTopNPrunesPartitionKeyFromSortInfo
- Static check: git diff --check
- Regression Test: Not run in this turn. The explain regression case is covered by planner/unit assertions here; no FE/BE regression cluster was started.
- Behavior changed: Yes. PartitionTopN no longer sends redundant partition key expressions to BE SortInfo when they exactly match partition keys; physical output ordering still advertises partition keys for property derivation.
- Does this need documentation: No
CalvinKirs
force-pushed
the
range_partition_key_ck
branch
from
May 28, 2026 10:37
c9ea01e to
84dafa4
Compare
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 32231 ms |
Contributor
TPC-DS: Total hot run time: 172233 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
|
/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 result: no blocking issues found.
Critical checkpoint conclusions:
- Goal and proof: The PR separates executable PartitionTopN sort keys from the property-facing output order so exact partition-key duplicates are not sent to BE SortInfo. Added FE planner/unit tests and explain regression coverage exercise pruning and the self-join non-pruning case.
- Scope: The change is focused on LogicalWindow creation, logical-to-physical conversion, physical property derivation, and targeted tests.
- Concurrency and lifecycle: No new concurrency, locking, static lifecycle, or persistence behavior is introduced.
- Configuration and compatibility: No new config, protocol field, storage format, or rolling-upgrade compatibility concern identified.
- Parallel paths: The translator now consumes PhysicalPartitionTopN executable order keys directly, while output-order property derivation keeps the partition keys for the existing window property contract. I did not find another PartitionTopN construction path that needs a separate change.
- Error handling and observability: No new error-handling or logging path is introduced.
- Transaction/data correctness: No transaction or storage visibility behavior is touched. For rank/dense_rank and row_number cases reviewed, pruning exact partition-key order expressions is semantically safe because those expressions are constant inside each partition. The self-join case keeps distinct ExprIds and is covered.
- Performance: The change removes redundant BE sort comparisons for exact duplicate partition/order expressions without adding meaningful overhead.
- Tests: Tests cover logical pruning, physical key separation, translator SortInfo pruning, explain output, and the distinct-slot self-join guard. I did not run the test suite during this automated review.
User focus response: No additional user-provided focus points were present.
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Window required order properties kept duplicate partition keys when order keys already contained the same expressions. This could make the two-phase global PartitionTopN output property fail to satisfy the upper Window and introduce an unnecessary sort.
### Release note
None
### Check List (For Author)
- Test: Regression test / Manual test
- ./run-regression-test.sh --run -d query_p0/explain -s test_global_partition_topn_plan -c jdbc:mysql://127.0.0.1:27030/?useLocalSessionState=true\&allowLoadLocalInfile=true\&zeroDateTimeBehavior=round -ha 127.0.0.1:26030 -u root
- Manual EXPLAIN verified VPartitionTopN order by excludes partition key and no PhysicalQuickSort in the two-phase global shape plan
- Behavior changed: Yes. Avoids redundant sort property requirements when Window order keys duplicate partition keys.
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Clarify why Window physical required properties prune order keys that duplicate partition keys, and how this avoids redundant sort insertion above PartitionTopN.
### Release note
None
### Check List (For Author)
- Test: FE style check
- mvn -pl fe-core -am validate -DskipTests
- Behavior changed: No
- Does this need documentation: No
Member
Author
|
run buildall |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29467 ms |
Contributor
TPC-DS: Total hot run time: 171757 ms |
Contributor
ClickBench: Total hot run time: 25.14 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Member
Author
|
run p0 |
…topn rule reorder Removing CreatePartitionTopNFromWindow from the bottom-up PUSH_DOWN_FILTERS rule set changes rewrite timing for query44, which has no partition-by window key. The two symmetric branches (asceding/i1 vs descending/i2) now tie-break to the opposite side in the cost-based join reorder; verified against a live matched FE+BE cluster that this is a pure left/right swap of two structurally identical subtrees, with no change to PartitionTopN itself. query67 (the actual partition-key-pruning target) is unaffected across all shape_check variants.
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29704 ms |
Contributor
TPC-DS: Total hot run time: 174061 ms |
Contributor
ClickBench: Total hot run time: 25.07 s |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review summary for PR 63379 (head 02ebe0f).
Review opinion: I did not find a new, non-duplicate blocking issue in the current head. The changed PartitionTopN split keeps the full output order property via PhysicalPartitionTopN#getOutputOrderKeys()/ChildOutputPropertyDeriver, while PhysicalPlanTranslator sends only the pruned executable order keys to BE PartitionSortNode. I also traced the BE rank/dense-rank path for empty executable sort keys; it treats all rows as one peer group, so I did not find a distinct truncation issue beyond the already-open semantic threads.
Critical checkpoints:
- Goal and proof: the PR aims to prune partition keys from PartitionTopN sort keys without losing the parent window's required order. The code and added coverage address logical pruning, physical output-order declaration, translator SortInfo pruning, and explain shape coverage.
- Scope: the change is focused on Nereids window/PartitionTopN order-key handling plus related tests and generated shape baselines.
- Concurrency/lifecycle/config/compatibility/persistence/writes: I found no new concurrency, lifecycle, config item, protocol/storage-format, persistence, or data-write path introduced by this PR.
- Parallel paths/performance: I checked
RuleSet/Rewriter,LogicalWindow#getPushDownWindowFuncAndLimit, request/output property derivation, logical-to-physical PartitionTopN, FE-to-BE translation, and BEPartitionSorterbehavior. - Tests/results: the changed query44 shape outputs are paired branch/name swaps, and
git diff --check FETCH_HEAD..HEAD --is clean. I did not run FE UTs or regression tests because this checkout is missingthirdparty/installedandthirdparty/installed/bin/protoc, which blocks the FE build prerequisite.
Duplicate-suppressed existing review context:
EliminateOrderByKeyexplicit/non-ranking ROWS and default-ROWS concerns are already covered bydiscussion_r3417828852anddiscussion_r3419555208.- The stale
EliminateOrderByKeyTestexpectation is already covered bydiscussion_r3450891995. - The self-join explain slot-identity false-pass is already covered by
discussion_r3459388086.
Subagent conclusions:
optimizer-rewritefound no new non-duplicate optimizer/rewrite candidate and returnedNO_NEW_VALUABLE_FINDINGSin convergence round 1.tests-session-configfound no new non-duplicate test/session/config candidate and returnedNO_NEW_VALUABLE_FINDINGSin convergence round 1.- Main merged result: no inline comments proposed; final review is summary-only.
User focus: no additional user-provided review focus was present.
Member
Author
|
run external |
Member
Author
|
run p0 |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
Contributor
FE Regression Coverage ReportIncrement line coverage |
morrySnow
approved these changes
Jul 2, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
yiguolei
pushed a commit
that referenced
this pull request
Sep 20, 2026
…n sort #63379 (#68171) #63379 --------- Signed-off-by: Aurélien Pupier <[email protected]> Co-authored-by: Aurélien Pupier <[email protected]>
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
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?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary: PartitionTopN still passed partition key expressions to BE sort info when the same original slot also appeared in window order keys. These keys are constant inside each partition, so keeping them causes redundant comparison work and noisy explain output, especially after slot remapping changes ExprIds.
after
This PR also adds a guard for self-join cases where the partition key and order key come from the same original table column, but from different join inputs.
Example:
Here l.c2 and r.c2 both come from test_global_partition_topn_plan.c2, but they are different slots after the self join. PARTITION BY l.c2 only guarantees that
the left-side c2 is constant inside each partition. It does not guarantee that the right-side r.c2 is constant, so ORDER BY r.c2 must not be pruned.
The expected EXPLAIN should still keep the order key in VPartitionTopN: