fix(window): handle temporal range bound overflow - #27394
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? |
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
[P1] Reviewed exact head 0584f36. The new insertion-point rule is correct for DATE and DATETIME, but the same valid-query failure remains for the other temporal RANGE types supported by this operator. The T_time and T_timestamp branches in both searchLeft and searchRight still return doTimeAdd/Sub or doTimestampAdd/Sub ErrOutOfRange directly. A stored maximum TIMESTAMP such as 9999-12-31 23:59:59.999999 with an ASC FOLLOWING interval therefore still errors instead of treating the upper search key as after the partition; minimum PRECEDING, DESC, and TIME domain boundaries have the analogous failure. JudgeIntervalNumOverflow errors should continue to propagate, but AddInterval domain overflow should use temporalRangeOverflowBoundary for every supported temporal type. Please add TIME and TIMESTAMP above/below-domain coverage in both directions, including an invalid interval-magnitude control and at least one public SQL regression.
|
已修复 review 指出的问题：TIME/TIMESTAMP 在 searchLeft/searchRight 中现在和 DATE/DATETIME 一样，仅将 AddInterval 产生的 ErrOutOfRange 映射为 temporalRangeOverflowBoundary；JudgeIntervalNumOverflow 仍直接返回错误。补充了四种 temporal 类型的上/下域外、ASC/DESC 双方向与非法区间控制单测，并扩展了公共 SQL 回归。验证：CGo 环境下 go build、go vet、pkg/sql/colexec/window 全包测试均通过。 |
|
Follow-up to @aunjgr: the requested TIME/TIMESTAMP paths are addressed in a2d8f97. Both searchLeft and searchRight now map only AddInterval ErrOutOfRange to temporalRangeOverflowBoundary; JudgeIntervalNumOverflow is still returned. TestSearchLeftRightTemporalRangeOverflow covers DATE/DATETIME/TIME/TIMESTAMP above and below domain in ASC/DESC plus invalid interval magnitudes, and window_temporal_range_bound.sql adds public TIME/TIMESTAMP regression coverage. Fresh local validation: GOWORK=off build and vet for ./pkg/sql/colexec/window/... passed with the repository CGo include paths; mo-cgo-test -v -count=1 -timeout=120s ./pkg/sql/colexec/window/... passed (including the exact regression). The current Actions run 32350145471 has no failed completed job; remaining UT/BVT jobs are still running. |
|
@aunjgr Addressed on 4d0b469. TIME and TIMESTAMP now use temporalRangeOverflowBoundary in both searchLeft/searchRight for AddInterval ErrOutOfRange only; JudgeIntervalNumOverflow still propagates. TestSearchLeftRightTemporalRangeOverflow covers DATE/DATETIME/TIME/TIMESTAMP above/below-domain, ASC/DESC, both searches, and invalid magnitudes. Public SQL coverage includes TIME and TIMESTAMP. The previous Pessimistic BVT failure was ResultParser-only: the new result file listed |
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
The checked MICROSECOND branches for TIMESTAMP bypass timestampRangeBoundary: both doTimestampSub and doTimestampAdd return dt.ToTimestamp(loc) directly, while all other interval units use the new DST-gap normalization.
That produces an incorrect and even non-monotonic RANGE key. In America/New_York, 2024-03-10 01:59:59.999999 + INTERVAL 1 MICROSECOND yields the nonexistent civil time 02:00:00. Go maps that direct conversion back to 01:00 EST, whereas this PRs helper deliberately clamps a gap value to the first valid instant, 03:00 EDT. The frame boundary can therefore jump backward and select the wrong rows.
Please route both MICROSECOND TIMESTAMP results through timestampRangeBoundary(dt, loc) and add add/subtract regressions crossing the spring-forward gap in both sort directions.
aptend
left a comment
aptend
left a comment
There was a problem hiding this comment.
Deep re-review completed on exact head c513527ad2fae12e2d6db0eb0138404f4ce4ab47. I read every historical review and issue comment, checked the (empty) inline/thread history and resolved state, compared the increment from my previous reviewed head 8455db803ac38ca2724f0afe26e8dd69e7ffbb8a, and rechecked the full six-file diff. The prior TIME/TIMESTAMP domain handling, signed MICROSECOND wrap, negative-interval direction, and MinInt64 validation findings are closed. Two blocking boundary-calculation holes remain: the MICROSECOND TIMESTAMP path bypasses the existing DST-gap normalization, and accepted non-MICROSECOND magnitudes can overflow during unit conversion before any domain error is produced.
Validation on this exact head, after make thirdparties with the required CGO environment:
go test ./pkg/sql/colexec/window ./pkg/container/types -count=1: passTestSearchLeftRightTemporalRangeOverflowat count 10 and under-raceat count 3: passgo vet ./pkg/sql/colexec/window ./pkg/container/types: passgit diff --check refs/codex/review/pr27394-base...HEAD: pass- exact DST-gap helper counterexample: expected 03:00 EDT, got 01:00 EST
- exact unit-conversion counterexample: expected insertion point 2, got 1
- public SQL
INTERVAL 307445734562 MINUTEregression: expected window sums[11,10], got[1,10]
All temporary counterexample tests were removed and the review worktree is clean. CI is green on this head.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
P1: calendar-sized RANGE intervals can still wrap back into the valid temporal domain and return silently incorrect frames.
The new temporalRangeIntervalConversionOK closes fixed-duration multiplication overflow, but returns true for MONTH, QUARTER, and YEAR. Those units reach Datetime.AddDateTime, which computes the year in int64 and then narrows it with y = int32(year) before ValidDate/ValidDatetime. A magnitude that differs by 2^32 years therefore wraps to the original year and is accepted.
I reproduced this on exact head 04b5262860f1e243cd3375d91a7d789e54e3cbc5: doDateAdd, doDatetimeAdd, and doTimestampAdd for 2024-01-01 + INTERVAL (1<<32) YEAR all returned 2024-01-01 with no error. In a RANGE frame, the conceptually unreachable upper bound is consequently searched as the current in-domain key, so an enormous FOLLOWING frame can collapse to the current peer instead of reaching the partition end. This is wrong-result risk, not just error handling.
Please validate calendar-unit arithmetic in the wide representation before narrowing (including the month/quarter-to-year conversion), return the same directional domain-overflow signal, and add search-level add/subtract coverage for MONTH, QUARTER, and YEAR in both sort directions. The focused DST-gap, fixed-duration conversion, signed-overflow, and interval-limit tests otherwise pass on this head.
Merge Queue Status
This pull request spent 1 minute 11 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonPull request #27394 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
|
5 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #27350
What this PR does / why we need it:
fix(window): handle temporal range bound overflow