Uh oh!
There was an error while loading. Please reload this page.
[improvement](fe) Infer set operator distinct by NDV - #64618
Conversation
hello-stephen
commented
Jun 17, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
morrySnow
left a comment
There was a problem hiding this comment.
Overall Assessment
The PR makes a good architectural improvement — removing the cost-based rewrite job in favor of stats-based NDV heuristics reduces optimizer overhead and enables per-child independent decisions. The core logic of using NDV statistics to decide whether to add local distinct aggregates is sound.
However, there are two significant concerns that need attention:
The regression test (
infer_set_operator_distinct.groovy+.out) was deleted without replacement beyond a single-scenario unit test. The original test covered 20+ plan shapes (UNION/EXCEPT/INTERSECT, nested operations, joins, complex subqueries, hint combinations). The new unit test only validates ONE scenario. This is a major test coverage regression.The
use_INFER_SET_OPERATOR_DISTINCT/use_NO_INFER_SET_OPERATOR_DISTINCThints are now silently ignored. Previously, users could force the rule on/off via these hints (the cost-based job checked them). Now, with the rule as a plaintopDown, there is no mechanism to disable it.
See inline comments for detailed suggestions.
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.
| class InferSetOperatorDistinctTest { | ||
| @Test | ||
| void testInferDistinctForEachChildByNdv() { |
There was a problem hiding this comment.
Test coverage concern: This single test only validates the per-child independent decision for an INTERSECT with one column per child. The deleted regression test (infer_set_operator_distinct.groovy) covered 20+ scenarios including:
- Different set operation types: UNION, EXCEPT, INTERSECT
- Nested set operations (e.g., INTERSECT of EXCEPT of UNION)
- Joins with set operations (INNER, LEFT, RIGHT, FULL OUTER, LEFT SEMI)
- Complex subqueries with filters
- Multi-column scenarios
- Hint-based control (
use_INFER_SET_OPERATOR_DISTINCT,use_NO_INFER_SET_OPERATOR_DISTINCT,/*+ SET_VAR(...) */)
Suggestions:
- Add at least a few more scenarios covering UNION and EXCEPT set operations
- Add a test for the case where stats derivation falls back (stats is null on the plan)
- Add a test for a multi-column group key scenario
- Consider retaining a subset of the regression test for end-to-end validation
fbf38c2 to
c06467bComparefeiniaofeiafei
commented
Jun 22, 2026
run buildall |
hello-stephen
commented
Jun 22, 2026
FE Regression Coverage ReportIncrement line coverage |
hello-stephen
commented
Jun 22, 2026
TPC-H: Total hot run time: 29319 ms |
hello-stephen
commented
Jun 22, 2026
TPC-H: Total hot run time: 29318 ms |
hello-stephen
commented
Jun 22, 2026
TPC-DS: Total hot run time: 173178 ms |
hello-stephen
commented
Jun 22, 2026
ClickBench: Total hot run time: 25.15 s |
feiniaofeiafei
commented
Jun 22, 2026
run buildall |
hello-stephen
commented
Jun 22, 2026
TPC-H: Total hot run time: 29338 ms |
hello-stephen
commented
Jun 22, 2026
TPC-DS: Total hot run time: 173154 ms |
hello-stephen
commented
Jun 22, 2026
ClickBench: Total hot run time: 25.15 s |
hello-stephen
commented
Jun 22, 2026
FE Regression Coverage ReportIncrement line coverage |
feiniaofeiafei
commented
Jun 23, 2026
run nonConcurrent |
hello-stephen
commented
Jun 23, 2026
FE Regression Coverage ReportIncrement line coverage |
morrySnow
commented
Jun 23, 2026
/review |
morrySnow
left a comment
There was a problem hiding this comment.
Review Summary
The core direction of this PR — switching from cost-based to stats-based NDV heuristics — is sound and removes unnecessary optimizer overhead. The per-child independent decision logic is correctly implemented. However, I found several issues that should be addressed:
Key findings:
rejectNLJmethod has multiple issues: misleading name, only peels one level of project, and dead code ingroupByCardinality- Missing documentation for NDV heuristic thresholds: The magic constants lack rationale comments
- Dead code: The fallback
return 2;ingroupByCardinalityandrowCount == 0check are unreachable - Hint support silently dropped:
use_INFER_SET_OPERATOR_DISTINCT/use_NO_INFER_SET_OPERATOR_DISTINCThints are now ignored — needs a TODO or deprecation plan - Test coverage gap: The regression test covering 20+ scenarios was deleted; only 1 scenario remains
See inline comments for detailed suggestions.
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.
ceed511 to
5aab413Comparefeiniaofeiafei
commented
Jun 24, 2026
run buildall |
hello-stephen
commented
Jun 24, 2026
TPC-H: Total hot run time: 28918 ms |
hello-stephen
commented
Jun 24, 2026
TPC-DS: Total hot run time: 171068 ms |
hello-stephen
commented
Jun 24, 2026
ClickBench: Total hot run time: 25.23 s |
hello-stephen
commented
Jun 24, 2026
FE UT Coverage ReportIncrement line coverage |
hello-stephen
commented
Jun 24, 2026
FE Regression Coverage ReportIncrement line coverage |
feiniaofeiafei
commented
Jun 25, 2026
run nonConcurrent |
1 similar comment
feiniaofeiafei
commented
Jun 25, 2026
run nonConcurrent |
hello-stephen
commented
Jun 25, 2026
FE Regression Coverage ReportIncrement line coverage |
morrySnow
commented
Jun 25, 2026
/review |
There was a problem hiding this comment.
Automated review summary for PR #64618 (5aab41304991ba454c25867e7a4fcac69863616c).
I reviewed all changed files and the relevant surrounding optimizer, stats, set-operation, and test paths. I did not find a new substantiated issue that is not already covered by existing inline review threads, so I am submitting a summary-only comment with no inline comments.
Critical checkpoint conclusions:
- Goal and proof: The PR moves
InferSetOperatorDistinctfrom a cost-based rewrite wrapper to a normal top-down rewrite and gates local distinct insertion by NDV/cardinality stats. The new JUnit proves one per-child NDV decision case, while broader deleted regression coverage has already been raised in existing review threads. - Scope and clarity: Code changes are focused on rule placement, the NDV heuristic, and expected plan-shape churn.
- Concurrency/lifecycle/config/persistence: No new concurrency, lifecycle, config, persistence, FE-BE protocol, or storage-format path is introduced.
- Parallel paths and compatibility: I checked
UNION,INTERSECT,EXCEPT, aggregate children, set-op child-output mapping, stats derivation, and no-cost/hint paths. The no-cost rewriter and hint behavior concern is already covered by existing threads onRewriter.java:607. - Test coverage/results:
git diff --checkis clean. I did not run FE tests becausethirdparty/installed/bin/protocis missing, whichfe/AGENTS.mdtreats as a stop condition. The regression-suite deletion versus replacement JUnit coverage concern is already covered by existing threads onInferSetOperatorDistinctTest.java:44. - User focus: No additional user-provided focus points were listed.
Subagent conclusions:
optimizer-rewrite: no new candidate; confirmed the generated aggregate preserves set-op child output slots and found no separate join/aggregate semantic regression beyond existing threads.tests-session-config: no new candidate; confirmed coverage and hint/no-cost concerns are existing-thread duplicates, and noted the local FE test stop condition.- Final convergence round 1 ended with both live subagents replying
NO_NEW_VALUABLE_FINDINGSfor the same zero-inline-comment final set.
PR approved by at least one committer and no changes requested. |
Uh oh!
There was an error while loading. Please reload this page.
What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary:
InferSetOperatorDistinctpreviously relied on a cost-based rewrite job to decide whether to add local distinct aggregates under set operations. This introduces extra optimizer cost.This PR changes
InferSetOperatorDistinctto run as a normal top-down rewrite rule and makes the rule decide whether to generate local distinct aggregates by child NDV/cardinality statistics. The NDV heuristic follows the existing logic inEagerAggRewriter#checkStats, but is kept local toInferSetOperatorDistinctbecause set-operation local distinct inference and eager aggregation pushdown have different optimization boundaries.Each child of a DISTINCT set operation is judged independently, so one child can get a local distinct aggregate while another child can remain unchanged when its NDV indicates poor deduplication benefit.
Release note
None
Check List (For Author)
./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.InferSetOperatorDistinctTest