Conversation
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Approval opinion: no blocking or otherwise actionable issue was found on exact head c1e29eb44a8e986831bb08de0aa115d5d5fde51e.
Review checkpoints:
- Goal and correctness: centralizing the three translator-created
SelectNodepaths preserves the prior node IDs, Nereids mapping, predicates, projections, limits, and fragment-root behavior while supplying the missing output and child distribution metadata.SelectNodestill forwards the parent's exact requirement; the child expression list now consumed byPlanNode.enforceRequireis resolved after child translation, so inserted exchanges receive bound child slots. A window placement onvcannot satisfy the join requirement onk, 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 onk) and aligned (PARTITION BY k, join onk) 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
shfmtfindings 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.
Contributor
Author
|
Superseded by #68199, which backports the whole local shuffle series as one PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cherry-picked from #67941