fix(hashjoin): broadcast const vectors in spill paths - #26938
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Conclusion: REQUEST_CHANGES. The PR author and authenticated reviewer are both XuPeng-SH, so GitHub cannot record a formal self-request-changes review; this is submitted as COMMENTED.
One merge-blocking correctness gap remains. The single bounded scatter, canonical hashing, UnionInt32 materialization, and recovery projection are otherwise consistent. Q1/Q2/Q3 found no leak, wait-cycle, or unbounded-growth defect, and the ordinary hot-path cost is one vector-level IsConst branch.
Validation on exact head 684797aff5f9 vs base/merge-base 0989f741286c: all changed tests and the three full owning packages passed; HashJoin, DedupJoin, and RightDedupJoin typed-key suites passed across resident/initial-spill/re-spill. A deterministic review-only pressure regression (8,192 rows, 80 KiB account, length-one const key) fails on this head with invalid input: invalid vector window after input reduction, proving the inline finding. Build, vet, and git diff --check passed. Race stress is not applicable because the diff changes no shared/concurrent state.
LeftHandCold
left a comment
There was a problem hiding this comment.
Reviewed at 684797a. No blocking findings.
Checked the constant-vector contract end to end: non-empty constants broadcast across the batch logical row count in initial spill and re-spill, while nil vectors, empty constants for non-empty input, and ordinary short vectors remain fail-closed. The varlena recovery projection now uses logical ranges for constants without weakening non-const bounds or overflow checks. No new ownership, cleanup, retry, concurrency, or per-row hot-path work is introduced.
Local low-concurrency validation passed:
- go build and go vet for keycodec, hashbuild, and spillutil
- full tests for all three owning packages
- focused prepared-parameter/broadcast/recovery tests
- HashJoin finite-budget initial-spill + re-spill integration test
Current GitHub UT, BVT, SCA, CodeQL, and coverage checks are green.
|
Addressed the latest request-changes review in c47df29. Reflection: the previous regression stopped after proving that an offset window remained The production fix keeps the existing empty-const and ordinary-short-vector guards, then returns zero physical/selected area for Fresh validation passed: controlled focused regressions, full @aptend please re-review. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Conclusion: APPROVE. GitHub cannot record a formal self-approval because the PR author and authenticated reviewer are both XuPeng-SH, so this verdict is submitted as COMMENTED.
I independently re-reviewed exact head c47df29 against base and merge-base 00cb7ef. No blocking findings remain.
Correctness closure: scalar constants broadcast only when non-empty and free of row-specific grouping or heterogeneous provenance; empty constants, ordinary short vectors, nil vectors, and short constants with row metadata remain fail-closed. The same logical-row contract now reaches resident hashmap validation, Batch and allocation-accounted pressure windows, initial spill, re-spill, and retained-recovery projection. Const-NULL varlena contributes zero payload before descriptor inspection, including offset logical windows and SetConstNull vectors with reusable stale area.
Q1/Q2/Q3 closure: window data and area remain borrowed while owned bitmap/provenance allocations are released exactly once; partial construction and every retry return path clean their temporary windows. The pressure loop has cancellation, progress checks, bounded retries, and monotonically shrinking chunks. No new wait edge, goroutine, retained unbounded collection, or generation-crossing state is introduced.
Performance and compatibility: ordinary paths add only vector-level O(1) const/range checks. There is no new per-row work, allocation, copy, lock, logging, or persisted representation change. Standalone Vector.Window remains physically bounded; logical broadcast is exposed only through explicit logical-row and Batch APIs.
Fresh local validation on this exact head passed: focused broadcast/offset/const-NULL/stale-area/rejection/row-conservation regressions; full pkg/container/..., pkg/common/hashmap/..., and pkg/sql/colexec/... tests; controlled build and vet for all changed owners plus HashJoin, DedupJoin, and RightDedupJoin dependents; git diff --check. Race stress is not applicable because the production diff changes no concurrent/shared state.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Conclusion: APPROVE. GitHub cannot record a formal self-approval because the PR author and authenticated reviewer are both XuPeng-SH, so this verdict is submitted as COMMENTED.
I re-reviewed exact head c47df29 against merge-base 00cb7ef and read every current PR comment/review. No blocking findings remain.
Both historical blockers are closed at the consumer boundary, not only at validation. Pressure reduction now uses logical-row windows for the input batch and separately evaluated key vectors, including nonzero offsets, while preserving all rows exactly once. Const-NULL varlena recovery now returns zero payload before descriptor inspection, so both offset logical windows and SetConstNull vectors with reusable stale area are valid. Empty constants for nonempty input, nil vectors, ordinary short vectors, grouping constants without the requested row domain, and heterogeneous prepared-parameter sidecars remain fail-closed.
Q1/Q2/Q3 audit: data and area in windows stay borrowed; owned bitmap and provenance allocations are released exactly once on success, partial construction failure, and every retry exit. Retry chunks shrink monotonically, cancellation and progress guards bound failure, and no wait edge, goroutine, generation crossing, or unbounded retained collection was added. Initial spill, re-spill, selected-row materialization, canonical hashing, and retained recovery now share the same logical-row contract.
Compatibility and performance: standalone Vector.Window keeps strict physical bounds; broadcast behavior is limited to explicit logical-row APIs and Batch.Window, whose RowCount owns the logical domain. Ordinary inputs add only O(vectors) constant/range checks at batch boundaries. There is no new per-row work, allocation, copy, lock, logging, wire-format change, or persisted-format change.
Fresh validation on this exact head passed: go list of the complete CGo closure; build and vet for all changed owning packages plus HashJoin, DedupJoin, RightDedupJoin, and LoopJoin consumers; full tests for those ten packages; 20 consecutive runs of the pressure row-conservation, prepared-parameter spill, offset const-NULL, stale-area, and full HashBuild/JoinMap regressions; and git diff --check. All current GitHub CI checks are terminal and green.
Nonblocking repository-state note: the branch is behind current main by 310fa3f, a frontend/parser-only change with no overlapping files or merge-tree conflict. The existing CHANGES_REQUESTED state is from aptend at b95ab95 before the c47df29 fix; aptend or a maintainer still needs to re-review or dismiss that stale review.
What changed
Why
The typed-key work in #26738 made resident and spill hashing share one codec. After #26836, resident hash maps also accept a physical-length-one constant vector over an arbitrary logical row range, while spill validation still required
vec.Length() == rows. A prepared parameter therefore worked in resident mode but failed initial spill or re-spill withErrHashBuildBudgetInvalid.This change aligns the existing spill boundary with the vector broadcast contract. It does not change typed-key encoding or add a new abstraction.
Validation
The new focused tests failed on the unfixed
mainwithinvalid hash build budgetand pass after the change.go list,go build, andgo vetforkeycodec,hashbuild, andspillutilPerformance
The ordinary spill path adds one inlinable
IsConstbranch per vector per batch. There are no new per-row operations, allocations, copies, logs, locks, or retained state.Closes #26432