Conversation
|
run buildall |
|
/review |
|
Superseded by #68199, which backports the whole local shuffle series as one PR. |
There was a problem hiding this comment.
Review outcome
Changes requested. The production distribution fix appears correct after tracing Nereids physical-property selection, FE local-exchange propagation, and the BE aggregation contract, but one branch-compatibility error prevents the changed FE test source from compiling. I am leaving one P1 inline finding.
Finding
- [P1] Import
FunctionNamefrom the analysis package. Branch-4.2 has noorg.apache.doris.catalog.FunctionName;FunctionCallExpr#getFnName()returnsorg.apache.doris.analysis.FunctionName. The exact one-line correction is in the inline comment.
Critical checkpoints
- Goal and correctness: The production change requires key-aligned input for keyed scalar DISTINCT/finalize aggregation while retaining the no-requirement path for genuinely keyless
COUNT(*)and direct scalarmulti_distinct_*. Nereids constrains the physical child to the aggregate partition/grouping ExprId equivalence set before legacy translation, so an unrelated hash key cannot satisfy the changed edge. - Scope and clarity: Production logic is localized to
AggregationNode; the other four changed files provide unit, plan-shape, and result coverage. The effective-key helper follows the existing FE/BE distribution model. - Concurrency and lifecycle: Planning remains per-query and introduces no shared mutable state, thread, lock, static initialization, ownership, or resource-release change.
- Configuration and compatibility: No configuration item, persisted representation, Thrift field, or FE/BE wire contract changes. Existing session knobs are reset or pinned in the relevant tests. The invalid test import is the only branch-4.2 symbol incompatibility found and is blocking until corrected.
- Parallel and conditional paths: LOCAL/FIRST, FIRST_MERGE, finalize, grouped/scalar, rewritten/direct multi-distinct, compatible child HASH, joins, set operations, inherited correctness propagation, serial sources, and disabled pre-agg local exchange were traced. No additional production defect was substantiated.
- Tests and validation: Apart from the compile-blocking import, the assertions are non-vacuous, pin the direct keyed aggregate edge, cover no-redundant/keyless paths, isolate shared session state, and use a fixed mathematical result for both planner modes. This review was static-only as required: no build or test was run.
git diff --checkon the exact PR range was clean. - Observability, persistence, transactions, and writes: No production persistence, replay, transaction, visibility, data-write, logging, metric, or tracing path changes.
- Performance: Compatible existing hash placement avoids a redundant local exchange, and the non-empty effective-key gate avoids a zero-key task funnel. Added planning scans are bounded by small aggregate/key lists.
- Other correctness: Every candidate was independently verified, deduplicated, accepted, or dismissed with code evidence.
MAIN-1is the sole accepted issue.
Review focus and completion
No additional user-provided focus was supplied, so the complete five-file change and relevant upstream/downstream paths were reviewed. Two review rounds and final convergence found no issue beyond the single inline compile blocker; no unresolved suspicious point remains.
| import org.apache.doris.analysis.SortInfo; | ||
| import org.apache.doris.analysis.TupleDescriptor; | ||
| import org.apache.doris.analysis.TupleId; | ||
| import org.apache.doris.catalog.FunctionName; |
There was a problem hiding this comment.
[P1] Import FunctionName from the analysis package
FunctionCallExpr#getFnName() returns org.apache.doris.analysis.FunctionName, and this branch contains no org.apache.doris.catalog.FunctionName. As written, this changed test cannot compile, so none of the new aggregation coverage can run. Please import the existing analysis class instead.
| import org.apache.doris.catalog.FunctionName; | |
| import org.apache.doris.analysis.FunctionName; |
Cherry-picked from #66570