fix(window): prevent overflow in value window offsets#25918
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
XuPeng-SH
left a comment
There was a problem hiding this comment.
The LAG/LEAD distance checks are sound, but the NTH_VALUE branch still trusts overflowed ROWS frame bounds before applying its new bounded-index check. A concrete reproducer is attached inline.
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
NTH_VALUE still has a silent wrong-result overflow path in ROWS frames.
P1 - Bound ROWS frame offsets before trusting the NTH_VALUE frame length (pkg/sql/colexec/window/window.go:548)
buildInterval can return overflowed ROWS bounds before this check: ROWS BETWEEN 9223372036854775807 FOLLOWING AND UNBOUNDED FOLLOWING on three rows computes rowIdx + int(following), which overflows for rows 1 and 2. Clamping then changes the negative bound to partition start 0, so this check sees a three-row frame and returns row 0 (10), although the frame must be empty and the result NULL. Saturate/validate ROWS-bound arithmetic before clamping and add this regression case.
matrix-meow
added
size/M
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
The prior blocking ROWS-frame overflow is fixed on the current head, and no functional regression remains. Current CI is green; local tests were blocked by missing prebuilt CGo artifacts. One non-blocking regression-test gap remains.
P2 - Make the NTH_VALUE test reach the overflow path (pkg/sql/colexec/window/value_window_test.go:566)
With three rows and ROWS BETWEEN 1 PRECEDING AND CURRENT ROW, left is at most 1, so the old left + MaxInt64 - 1 calculation never overflows and this test passes before the fix. Using four rows makes left == 2 on the last row: the old target wraps to MinInt64 and UnionOne can incorrectly read row 0, while the new length check returns NULL. The implementation is correct, so this is non-blocking, but the regression should exercise the bug it is intended to prevent.
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
The earlier review on this head contains only non-blocking findings. Approving.
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
No findings. The prior ROWS-frame overflow review thread is resolved and independently validated against the current head; bounds are checked before conversion/arithmetic. Regression coverage reaches the original LEAD, NTH_VALUE, and frame-bound cases. Local Go tests could not run because the environment has no Go toolchain; current PR CI is green.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
I checked the overflow fix from correctness / robustness / unhappy-path angles and I did not find a blocker.
The core arithmetic changes look right:
leadnow checks the partition-local distance before doingj + offset, so the overflowed wraparound path is gone.nth_valuenow checksn > right-leftbefore computingleft + n - 1, which removes the target-row overflow.buildRowsIntervalnow saturates hugeROWS ... PRECEDING/FOLLOWINGbounds to the partition edges instead of overflowing signedintarithmetic.
The new unit + SQL regression coverage is enough for the bug being fixed.
Optional follow-up only: it would still be nice to add a couple more regressions for non-const huge offsets / huge nth_value, but I do not see that as a merge blocker here.
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
Reviewed the current head independently. The LAG/LEAD/NTH_VALUE index checks compare offsets before converting them to int, and ROWS frame bounds saturate against partition limits before arithmetic. Boundary tests cover MaxInt64 offsets and overflowing FOLLOWING frames. I found no merge-blocking correctness, lifecycle, or compatibility issue.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review on exact head 7ea630b: no blocking findings.
I rechecked the full arithmetic closure rather than relying on the prior approval. LAG/LEAD compare partition-local distance before int conversion/arithmetic; NTH_VALUE checks frame length before computing the target row; all four finite ROWS-bound directions saturate at partition edges before signed arithmetic.
Fresh validation:
- full pkg/sql/colexec/window tests: pass
- focused overflow tests under race, count=10: pass
- full window package under race: pass
- go build and go vet: pass
- exact-head SQL with constant and non-constant offsets, defaults, non-zero partition starts, MaxInt64 and MaxUint64, plus huge PRECEDING/FOLLOWING frames: correct
I also ran the same SQL shapes on the exact PR base. The base returned earlier rows for huge LEAD, FIRST_VALUE empty-frame, and four-row NTH_VALUE cases, while the PR head returned NULL/default as required. This confirms the regressions exercise real pre-fix failures. Q1-Q3 review found no new resource, wait, or growth risks.
Merge Queue Status
This pull request spent 1 hour 38 minutes 22 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonPull request #25918 has been dequeued Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.
Failing checks:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Tick the box to put this pull request back in the merge queue (same as
|
6 participants
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #25906
What this PR does / why we need it:
LEADpreviously added a BIGINT offset to the current row before checking the partition boundary.INT64_MAXtherefore overflowed on later rows and silently read earlier values.This PR:
LEAD(..., 9223372036854775807).Validation:
go test ./pkg/sql/colexec/window -count=1go test -race ./pkg/sql/colexec/window -run '^(TestProcessValueFunc_LeadWithMaxInt64Offset|TestProcessValueFunc_NthValueWithMaxInt64Position)$' -count=1go vet ./pkg/sql/colexec/windowgo build ./pkg/sql/colexec/windowgit diff --check