fix(window): honor session timezone in timestamp range frames - #27384
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
There was a problem hiding this comment.
Reviewed exact head a0e8f51. TIMESTAMP RANGE interval arithmetic now reads the execution process session zone, preserves the prior time.Local fallback, and keeps timezone state execution-local with no operator lifecycle changes. DST gap clamping is limited to non-round-tripping civil boundaries; ordinary and fixed-offset paths retain the fast binary-search behavior. Unit and SQL regressions cover spring gaps, fall folds, UTC/non-DST controls, and process reuse. No blocking findings.
aptend
left a comment
There was a problem hiding this comment.
I deep-reviewed the complete diff and review history at exact head a0e8f51d0e5f45c548b6a462cb93bf4554722eb9. Session-zone propagation, spring-gap clamping, process reuse, and the existing UTC/fixed-zone paths look sound. The full window package, focused race test, go vet, and git diff --check pass. However, an independent MySQL 8.4.11 differential found a blocking fall-fold case not covered by the current matrices: civil timestamp order is non-monotonic relative to stored TIMESTAMP instant order during the repeated hour, so converting one civil boundary to one instant and binary-searching the instant-sorted vector admits rows MySQL excludes. A temporary exact-head regression reproduced the wrong contiguous frame. Please handle fold-crossing membership rather than assuming a single monotonic instant interval, and add ASC/DESC plus PRECEDING/FOLLOWING cases containing both repeated-hour occurrences and an intervening lower civil time.
XuPeng-SH
left a comment
There was a problem hiding this comment.
P1: preserve non-contiguous civil-time membership across a fall-back fold. At this exact head, use America/New_York with UTC instants 05:30, 06:00, 06:30, 07:00 on 2024-11-03 (displayed as 01:30 EDT, 01:00 EST, 01:30 EST, 02:00 EST) and values 1, 10, 100, 1000. MySQL 8.4.10 returns 1101 for the first row under ORDER BY ts RANGE BETWEEN CURRENT ROW AND INTERVAL 30 MINUTE FOLLOWING: it includes both 01:30 occurrences and 02:00, but excludes the intervening 01:00. The current searchLeft/RightWithLocation path converts one civil boundary back to one instant and then binary-searches the raw instant-sorted vector, so it can only produce a contiguous [left,right) interval; an exact-head production-helper regression returns [0,4] and 1111 instead. This is a wrong-query-result bug during the repeated hour, not just a missing edge-case test. Please make fold-crossing membership correct without assuming one monotonic instant interval, and cover ASC/DESC plus PRECEDING/FOLLOWING witnesses containing both repeated-hour occurrences and an intervening lower civil time.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed exact head f0b08a410470b4d651e77effbc8b60ce74a1eb88. The new evalOrderVector invalidation and its same-vector/two-batch regression close my prior stale-fold-cache blocker, and the fold-membership correctness matrix passes. One new P1 performance/liveness blocker remains.
P1 — Avoid an all-partition scan for every row of an unbounded fold frame
Once timestampCivilOrderHasFold is true, timestampRangeSelection handles a one-sided unbounded frame by iterating every row in [start,end) (window.go:1252-1262). Unlike finite bounds, this path has no DatetimeRangeToTimestampRange fast-path proof, so it repeats that full scan for every output row—even when the current row is months away from the repeated hour. first_value, last_value, and nth_value previously obtained each RANGE boundary with binary search and then copied one value; on any partition containing a fall-back transition, RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW (and its following/descending variants) now becomes quadratic.
I reproduced the production helper path with an instant-sorted New York partition spanning the 2024 fold and evaluated the unbounded frame for every row. A clean-worktree, one-iteration benchmark on this head measured:
1,000 rows: 8.7-9.5 ms
2,000 rows: 34.8-36.5 ms
4,000 rows: 139.4-142.6 ms
Doubling rows consistently costs about 4x. Extrapolation is not needed to establish the regression: the code executes n membership checks for each of n rows. The new inner scan also has no cancellation poll, so cancellation arriving during a large scan is delayed until it returns.
Please preserve non-contiguous civil-time correctness with a bounded representation/search strategy (for example, transition-aware monotonic spans or a reusable civil-order index) rather than rescanning/materializing the whole partition per row. Add a size-scaling regression/benchmark for unbounded fold frames and poll cancellation in any remaining long scan.
Validation on the committed exact head: full ./pkg/sql/colexec/window tests pass; the focused session-zone/fold suite passes under -race -count=5; go vet and git diff --check pass; CI is green. The temporary review benchmark was removed and the isolated worktree is clean.
aptend
left a comment
There was a problem hiding this comment.
Deep re-review of exact head f0b08a410470b4d651e77effbc8b60ce74a1eb88. I read every historical review, inline comment, author reply, and resolved-thread state; compared the post-a0e8f51d0e5f45c548b6a462cb93bf4554722eb9 changes; and audited the full five-file diff. The original fall-fold membership defect and the later same-vector cache-generation defect are now closed, and the aggregate/value-function correctness matrices are coherent. A blocking hot-path/liveness regression remains inline: every row of a one-sided unbounded frame rescans the complete partition once any fold is present. My independent exact-head benchmark confirms quadratic scaling. The full window suite, focused fold/session-zone race repetitions, vet, and diff checks pass; the temporary benchmark was removed and the worktree restored.
|
|
||
| col := vector.MustFixedColNoTypeCheck[types.Timestamp](vec) | ||
| rows := ctr.timestampRangeRows[:0] | ||
| for i := start; i < end; i++ { |
There was a problem hiding this comment.
[P1] This full-partition scan runs once per output row for every one-sided unbounded frame whenever the partition contains any fall-back fold. first_value, last_value, and nth_value previously found each RANGE boundary by binary search and copied one value; now UNBOUNDED PRECEDING ... CURRENT ROW (and the symmetric/descending forms) execute n civil conversions and membership tests for each of n rows, even when almost every row is far from the transition. An exact-head -benchtime=1x -count=3 production-helper benchmark over New York instant-sorted partitions measured 14.5–14.8 ms at 1,000 rows, 56.9–61.5 ms at 2,000, and 223.7–226.1 ms at 4,000: each doubling costs about 4x. The loop also has no cancellation poll, so a canceled large query remains stuck until the current O(n) scan returns. Build a reusable transition/civil-order index or monotonic-span representation so each frame lookup is bounded, and poll cancellation in any remaining long scan; add a size-scaling regression for the unbounded value-function path.
There was a problem hiding this comment.
Addressed in current head 003cceb38a54be7d72bb1e4999194684c0a5e2eb (fix(window): index timestamp range fold spans). The fold path now builds and caches session-civil monotonic spans once per materialized order-vector generation, then binary-searches each span for a frame; it no longer scans the whole partition per output row. The cache is invalidated before the materialized order vector is reused for the next input batch. Span-index construction polls cancellation at the standard interval. Added the cancellation regression and BenchmarkWindowTimestampRangeFoldUnboundedValue (1k/2k/4k); current controlled-CGo -benchtime=1x results were 0.92/2.42/3.50 ms. Fresh focused fold tests, the full ./pkg/sql/colexec/window package, CGo-configured build, and vet all pass.
XuPeng-SH
left a comment
There was a problem hiding this comment.
[P1] Detect timezone transitions even when sampled civil values never reverse
timestampCivilOrderIndex only sets hasFold when adjacent sampled wall-clock values strictly reverse. A sparse, instant-sorted partition can cross the fall-back offset transition while its sampled civil values still increase (or remain equal), so this returns hasFold=false and timestampRangeSelection falls back to the single UTC interval. Exact-head production-path repro with America/New_York: order keys 2024-11-03 05:00 UTC (01:00 EDT) and 2024-11-03 06:30 UTC (01:30 EST), then evaluate row 0 with RANGE BETWEEN CURRENT ROW AND INTERVAL 30 MINUTE FOLLOWING. Civil membership is [01:00, 01:30], so both rows qualify; buildIntervalRows returns only row 0. The same blind spot exists for two repeated civil values such as the two 01:30 occurrences. Please derive fold/transition presence from the timezone offset transition over the instant range (not an observed ordering reversal), and add sparse-crossing plus equal-civil regression cases.
iamlinjunhong
left a comment
There was a problem hiding this comment.
已在当前精确 head 9169baf4e21959fdf620f8f259549d7784afdebb 验证该稀疏/相等 civil-time 情形。
timestampCivilOrderIndex 现在从排序 TIMESTAMP 的完整 instant 范围通过 ZoneBounds 识别 UTC offset 减小的 transition,并在跨越 transition 时切分 civil-order span;因此不再依赖相邻采样的 wall-clock 值是否反转。新增回归覆盖 01:00 EDT → 01:30 EST 的稀疏升序、对应降序,以及两次 01:30 的相等 civil time;三种情形均返回两个成员。分布式 SQL 回归也包含这两项控制。
本地受控 CGo focused fold suite、完整 ./pkg/sql/colexec/window、该包 go build/go vet 均通过;unbounded value benchmark 为 1000/2000/4000 行 2.29/4.34/8.45 ms(-benchtime=1x)。当前 head 未复现该 blind spot。
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed exact head b77c950d99f0f05b5eba4a6f2ec2ab639d4be4be. The ZoneBounds transition discovery closes the prior sparse/equal-civil fold blocker, the cached span index closes the quadratic unbounded-frame blocker, cache generation remains correctly invalidated, and the focused committed fold/session-zone suite passes. One new P1 wrong-result blocker remains: the fold selection is applied to legal multi-expression ORDER BY frames and discards the complete tuple peer/order semantics. I reproduced this through processAggregateFuncRange in UTC with sorted (k, ts, v) rows (1,A,1),(1,B,2),(2,A,4),(2,B,8) and RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW. The correct sums are [1,3,7,15]; exact head returns [5,15,5,15]. The last timestamp key restarts from B to A at the next k group, civilReversed mistakes that lexicographic reset for a timezone fold, and the resulting timestamp-only spans select future rows from other k groups. Please preserve ctr.os/all ORDER BY keys for multi-key CURRENT/UNBOUNDED RANGE frames (or constrain this civil-fold path to the single-key cases where it is semantically valid) and add an execution regression for ORDER BY k, ts. The temporary review test was removed and the isolated worktree is clean apart from review-only ignored/symlinked build dependencies.
|
|
||
| vec := ctr.orderVecs[len(ctr.orderVecs)-1].Vec[0] | ||
| loc := windowSessionLocation(proc) | ||
| selection, err = ctr.timestampRangeSelection(proc, loc, rowIdx, start, end, vec, frame) |
There was a problem hiding this comment.
[P1] Do not replace multi-key RANGE peer semantics with a timestamp-only fold selection. Multi-expression ORDER BY is legal for unbounded/current RANGE frames. With UTC rows (k,ts,v)=(1,A,1),(1,B,2),(2,A,4),(2,B,8), sum(v) over (order by k,ts range between unbounded preceding and current row) must yield [1,3,7,15]; exact head yields [5,15,5,15]. The last key normally resets from B to A at a new k group, civilReversed marks that as a fold, and this call overrides the correct ctr.os interval with timestamp-only spans that include rows from different leading-key groups. Gate the civil-fold path to its valid single-key shape or retain all-key peer/order boundaries, and cover this query.
There was a problem hiding this comment.
Fixed in 7cdd454dbce0cedc2e0ed8ce91d96320ed644829. timestampRangeSelection now runs only for a single TIMESTAMP ORDER BY key, so multi-expression RANGE frames retain the complete tuple peer interval in ctr.os. Added a production-path aggregate regression for ORDER BY k, ts RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW; it returns [1, 3, 7, 15]. The focused regression, full controlled-CGo window package, and controlled-CGo build/vet pass.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed exact head 7cdd454. The latest single-key guard closes the multi-key RANGE wrong-result blocker: civil-fold span selection is now used only for one TIMESTAMP ORDER BY key, while tuple ordering retains ctr.os peer boundaries. The prior fold-membership, sparse-transition, batch-generation cache, cancellation, and unbounded-frame complexity fixes remain intact. Focused fold/session-zone tests pass under race count=5, the full window package passes, and the 1k/2k/4k unbounded benchmark scales approximately linearly (0.42/0.65/1.32 ms locally). Required CI is green.
aunjgr
left a comment
There was a problem hiding this comment.
Reviewed exact head e9f43d4. The fold path is restricted to a single TIMESTAMP ORDER BY key, caches bounded per-materialized-order-vector civil spans and clears them on reuse/reset/free, and polls cancellation while building the index. Multi-key RANGE retains tuple-peer semantics. No blocking issue found.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of exact head e9f43d4. The previously fixed fold-membership, sparse-transition, cache-generation, multi-key, cancellation, and one-fold scaling cases remain covered, and the committed focused suite passes. Two independent P1 blockers remain: a public-path constant TIMESTAMP ORDER BY panics because logical const rows are indexed as physical rows, and fold selections drop NULL peers that belong to an unbounded side. Both were reproduced through processAggregateFuncRange; the constant-key control passes at PR base 6ff9be9. Please close both adjacent shapes and add execution regressions.
| if err := checkCanceled(proc, end-1-i); err != nil { | ||
| return nil, err | ||
| } | ||
| if vec.GetNulls().Contains(uint64(i)) || col[i] == types.ZeroTimestamp { |
There was a problem hiding this comment.
[P1] Handle constant TIMESTAMP order vectors before indexing logical row positions. MustFixedColNoTypeCheck returns one physical element for a const vector, while vec.Length() and [start,end) are logical row counts. This reverse scan therefore reads col[end-1] and panics for a legal constant TIMESTAMP ORDER BY. A production aggregate-path test with vector.NewConstFixed(..., 4) and RANGE BETWEEN CURRENT ROW AND CURRENT ROW panics here with index out of range [3] with length 1; the identical test at PR base 6ff9be9 returns [15,15,15,15]. The public SQL shape is accepted (there is already a constant TIMESTAMP RANGE binder case in pkg/sql/plan/window_binder_test.go). Please use a const-aware accessor or bypass fold indexing for a constant peer group, and add an execution regression.
There was a problem hiding this comment.
Fixed in af19357517e1b75b633ad1614f889609c1038497. RANGE boundary searches now evaluate a constant vector at its one physical row and map the result back to the logical interval; the fold selector also bypasses constant vectors. The aggregate regression covers CURRENT ROW peers and a finite FOLLOWING frame with no qualifying rows. Focused and full controlled-CGo window tests, build, and vet pass.
| if err := checkCanceled(proc, i-start); err != nil { | ||
| return nil, err | ||
| } | ||
| if vec.GetNulls().Contains(uint64(i)) { |
There was a problem hiding this comment.
[P1] Preserve NULL peers that lie on an unbounded frame side. This branch removes NULL rows from every civil span, and once hasFold is true timestampRangeSelection constructs membership only from those spans. For ASC/default NULLS FIRST with RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW, each non-NULL frame must retain the leading NULL peer. A production aggregate-path witness with values 100(NULL), 1(01:00 EDT), 2(01:30 EDT), 4(01:00 EST), 8(01:30 EST) returns [100,5,15,5,15]; the correct result is [100,105,115,105,115]. The symmetric unbounded-following/NULLS-LAST shapes are exposed too. Please represent the applicable NULL prefix/suffix (including ASC/DESC and explicit NULL ordering) and add fold regressions.
There was a problem hiding this comment.
Fixed in af19357517e1b75b633ad1614f889609c1038497. The cached fold index now retains leading and trailing NULL peer spans; selection includes them only when the corresponding frame side is unbounded and reaches the NULL end of the sorted order. Aggregate regressions cover ASC NULLS FIRST/UNBOUNDED PRECEDING, ASC NULLS LAST/UNBOUNDED FOLLOWING, and DESC NULLS FIRST. Focused and full controlled-CGo window tests, build, and vet pass.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of exact head af19357. The const TIMESTAMP path now bypasses fold indexing and preserves finite empty-frame boundaries; the fold index records sorted NULL prefix/suffix bounds and restores them only for the matching unbounded side, covering ASC/DESC and explicit NULL ordering without changing finite-frame membership. The prior fold, sparse-transition, cache-generation, cancellation, multi-key, and linear one-fold paths remain intact. Fresh focused regressions, complete window package tests, go build, go vet, and diff checks pass. No blocking findings.
Which issue(s) this PR fixes:
Fixes #27351
What this PR does / why we need it:
Use the query session time zone for TIMESTAMP RANGE boundaries and preserve session-civil frame membership across DST gaps and fall-back folds.