Skip to content

branch-4.2: [fix](local shuffle) Require hash input for distinct finalize agg without group keys #66570 - #68190

Closed
924060929 wants to merge 1 commit into
auto-pick-66295-branch-4.2from
auto-pick-66570-branch-4.2
Closed

924060929 wants to merge 1 commit into
auto-pick-66295-branch-4.2from
auto-pick-66570-branch-4.2

Conversation

@924060929

Copy link
Copy Markdown
Contributor

Cherry-picked from #66570

@924060929
924060929 requested a review from yiguolei as a code owner September 18, 2026 09:29
@924060929

Copy link
Copy Markdown
Contributor Author

run buildall

@924060929

Copy link
Copy Markdown
Contributor Author

/review

@924060929

Copy link
Copy Markdown
Contributor Author

Superseded by #68199, which backports the whole local shuffle series as one PR.

@924060929 924060929 closed this Sep 18, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 FunctionName from the analysis package. Branch-4.2 has no org.apache.doris.catalog.FunctionName; FunctionCallExpr#getFnName() returns org.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 scalar multi_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 --check on 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-1 is 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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
import org.apache.doris.catalog.FunctionName;
import org.apache.doris.analysis.FunctionName;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant