Skip to content

branch-4.2: [fix](local shuffle) Populate distribution keys for select nodes #67941 - #68192

Closed
924060929 wants to merge 1 commit into
auto-pick-67177-branch-4.2from
auto-pick-67941-branch-4.2
Closed

924060929 wants to merge 1 commit into
auto-pick-67177-branch-4.2from
auto-pick-67941-branch-4.2

Conversation

@924060929

Copy link
Copy Markdown
Contributor

Cherry-picked from #67941

@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

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

Approval opinion: no blocking or otherwise actionable issue was found on exact head c1e29eb44a8e986831bb08de0aa115d5d5fde51e.

Review checkpoints:

  • Goal and correctness: centralizing the three translator-created SelectNode paths preserves the prior node IDs, Nereids mapping, predicates, projections, limits, and fragment-root behavior while supplying the missing output and child distribution metadata. SelectNode still forwards the parent's exact requirement; the child expression list now consumed by PlanNode.enforceRequire is resolved after child translation, so inserted exchanges receive bound child slots. A window placement on v cannot satisfy the join requirement on k, while already matching hash placement and exact GLOBAL/LOCAL/BUCKET flavors retain their existing satisfaction behavior.
  • Scope and call-chain coverage: all three changed files and every Select construction branch were reviewed, along with physical property projection/request derivation, analytic/sort/join/set-operation enforcement, local-exchange serialization, null/ANY properties, aliases and consecutive projects, serial-source handling, and FE/BE planner boundaries. No unresolved candidate remains.
  • Tests: the new three-task regression enables FE local-shuffle planning and exercises both mismatched (PARTITION BY v, join on k) and aligned (PARTITION BY k, join on k) cases. Its count and independent sums are a meaningful oracle for the historical row-loss failure. Direct plan-shape/key assertions and stacked-alias/serial variants would be useful follow-up coverage, but no production defect was established from those gaps.
  • Configuration and compatibility: no configuration definition, protocol, persisted format, transaction/data-write path, rolling-upgrade contract, or documentation surface changes. Existing session variables only select the regression path.
  • Concurrency, lifecycle, and observability: no shared-state, locking, ownership, cleanup, cancellation, logging, or error-propagation behavior changes.
  • Performance: metadata extraction is planner-only and bounded by the distribution-key/output sets. Static tracing found no new unconditional repartition or exact-flavor downgrade.
  • Validation status: this was a static review only; the runner contract prohibited builds and tests. Current CI shows the FE style/license and listed unit/coverage contexts successful. ShellCheck is currently failing on repository-wide shfmt findings in shell files outside this PR's three changed paths, so it is not treated as a finding against this patch.
  • Review convergence: two independent complete-review agents and one separate risk-focused agent all returned NO_NEW_VALUABLE_FINDINGS; the main agent independently adjudicated every recorded risk and the final changed-file/unresolved-candidate sweep was clean.
  • User focus: no additional review focus was supplied.

@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
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