Uh oh!
There was an error while loading. Please reload this page.
[fix](partition_prune) Move the pruning of predicates that are always true after partition pruning into the PlanPostProcessor - #63111
Conversation
hello-stephen
commented
May 9, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
feiniaofeiafei
commented
May 9, 2026
run buildall |
1 similar comment
feiniaofeiafei
commented
May 9, 2026
run buildall |
feiniaofeiafei
commented
May 9, 2026
run buildall |
feiniaofeiafei
commented
May 11, 2026
run buildall |
hello-stephen
commented
May 11, 2026
TPC-H: Total hot run time: 29511 ms |
hello-stephen
commented
May 11, 2026
TPC-DS: Total hot run time: 170534 ms |
feiniaofeiafei
commented
May 11, 2026
run buildall |
hello-stephen
commented
May 11, 2026
TPC-H: Total hot run time: 29722 ms |
hello-stephen
commented
May 11, 2026
TPC-DS: Total hot run time: 170733 ms |
hello-stephen
commented
May 11, 2026
FE Regression Coverage ReportIncrement line coverage |
feiniaofeiafei
commented
May 12, 2026
run p0 |
foxtail463
commented
May 12, 2026
I agree with the core idea: partition-prunable predicates should not be removed before MV rewrite, otherwise MV matching may see an incomplete query predicate set and produce an unsafe rewrite. One concern is where the deferred-removal state is stored. The current implementation records
Putting that proof in A cleaner structure may be to attach this information to the scan itself, e.g. classPartitionPruneInfo {
Set<Long> provedPartitionIds;
List<Slot> partitionSlots;
Set<Expression> prunableConjuncts;
}Then the post processor can simply inspect scan.getPartitionPruneInfo() and remove a conjunct only when: partitionPruneInfo.getProvedPartitionIds().containsAll(scan.getSelectedPartitionIds())This keeps the invariant local to the scan and avoids using StatementContext as a side table for plan-node-specific proof. |
…nstead of StatementContext ### What problem does this PR solve? Problem Summary: Move PartitionPrunablePredicate info from StatementContext (keyed by TableIdentifier) onto LogicalOlapScan / PhysicalOlapScan so it follows the scan through with*/deepCopy and is included in equals. Also propagate the info onto the MV scan rewritten by SyncMaterializationContext#getScanPlan so the post-processor can still strip the deferred conjuncts after sync MV rewrite. ### Release note None ### Check List (For Author) - Test: Regression test (prune_predicates_mv_test.groovy) - Behavior changed: No - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
hello-stephen
commented
May 12, 2026
FE Regression Coverage ReportIncrement line coverage |
feiniaofeiafei
commented
May 12, 2026
run buildall |
One olap scan can carry at most one PartitionPrunablePredicate, so use Optional instead of Set. Simplifies merge in PruneOlapScanPartition and iteration in PrunePartitionPredicate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
d817a84 to
4c371b5Comparefeiniaofeiafei
commented
May 12, 2026
run buildall |
hello-stephen
commented
May 12, 2026
TPC-H: Total hot run time: 29486 ms |
hello-stephen
commented
May 12, 2026
TPC-DS: Total hot run time: 169360 ms |
hello-stephen
commented
May 12, 2026
FE UT Coverage ReportIncrement line coverage |
feiniaofeiafei
commented
May 12, 2026
run feut |
hello-stephen
commented
May 12, 2026
FE Regression Coverage ReportIncrement line coverage |
hello-stephen
commented
May 12, 2026
FE UT Coverage ReportIncrement line coverage |
morrySnow
commented
May 13, 2026
/review |
feiniaofeiafei
commented
May 14, 2026
run buildall |
hello-stephen
commented
May 14, 2026
TPC-H: Total hot run time: 29585 ms |
hello-stephen
commented
May 14, 2026
TPC-DS: Total hot run time: 171194 ms |
PR approved by at least one committer and no changes requested. |
PR approved by anyone and no changes requested. |
hello-stephen
commented
May 14, 2026
FE UT Coverage ReportIncrement line coverage |
feiniaofeiafei
commented
May 14, 2026
/review |
hello-stephen
commented
May 14, 2026
FE Regression Coverage ReportIncrement line coverage |
924060929
commented
May 14, 2026
/review |
There was a problem hiding this comment.
Code review completed. I did not find a critical/blocking issue to request changes.
Critical checkpoint conclusions:
- Goal and tests: The PR defers removal of partition-prunable predicates until after MV rewrite and carries the necessary metadata through logical/physical OLAP scans. The added regression suite covers async MV, partitioned MV, and sync MV alias cases; unit coverage for
PartitionPruner.pruneWithResultremains focused on predicate-result metadata. - Scope and focus: The implementation is reasonably focused on partition predicate pruning/MV rewrite interaction. The existing package-location review thread was already addressed by moving
PartitionPrunablePredicatetotrees.plans, so I did not repeat it. - Concurrency/lifecycle: The changes are optimizer plan metadata and post-processing only; I did not identify new shared mutable state, lock ordering, or lifecycle hazards.
- Configuration/compatibility: No new configs, storage formats, Thrift/protocol fields, or rolling-upgrade compatibility concerns were introduced.
- Parallel paths: OLAP scan logical-to-physical propagation and sync MV scan propagation are covered. External file scan partition pruning is not part of this behavior and remains separate.
- Conditional checks:
skipPrunePredicateand delete-statement exclusions are preserved both when recording and when removing deferred predicates. The post-processor also verifies that recorded selected partitions still cover the final scan partitions before removing predicates. - Test coverage/results: Regression output is present and deterministic via
order_qt; no additional user-provided focus points were supplied. I did not run the tests in this review runner. - Observability: No new observability requirement found; this is an optimizer rewrite/post-process change.
- Transaction/persistence/data writes: Not applicable.
- FE/BE variable passing: Not applicable.
- Performance: The added post-processor work is limited to physical filters directly over OLAP scans with recorded metadata; no obvious hot-path anti-pattern was found.
User focus: .opencode-review.yiiWoK/review_focus.txt says no additional focus was provided; no extra focus-specific issue was found.
Uh oh!
There was an error while loading. Please reload this page.
… true after partition pruning into the PlanPostProcessor (apache#63111) ### What problem does this PR solve? Related PR: apache#57169 Problem Summary: After partition pruning, predicates that are evaluated to constant true are removed as an optimization (introduced by apache#57169). However, this introduced a bug in materialized view rewriting: when such predicates are removed, they are not compensated above the rewritten materialized view. Since the materialized view itself is not partitioned, this leads to incorrect query results. This PR fixes the issue by moving the removal of constant-true predicates to the planPostProcessors phase. This ensures that the predicate removal does not interfere with materialized view rewriting, preserving correctness while still retaining the optimization benefit. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #57169
Problem Summary:
After partition pruning, predicates that are evaluated to constant true are removed as an optimization (introduced by #57169). However, this introduced a bug in materialized view rewriting: when such predicates are removed, they are not compensated above the rewritten materialized view. Since the materialized view itself is not partitioned, this leads to incorrect query results.
This PR fixes the issue by moving the removal of constant-true predicates to the planPostProcessors phase. This ensures that the predicate removal does not interfere with materialized view rewriting, preserving correctness while still retaining the optimization benefit.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)