fix sub-second units - #27017
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? |
aptend
left a comment
There was a problem hiding this comment.
Deep review against 1f20868. The focused CGo-enabled tests pass, and the microsecond conversion / mixed-unit GCD path is otherwise internally consistent.
[P1] _wstart / _wend still have a scale-0 logical type, so the advertised TIMESTAMP(6)/DATETIME(6) boundary support is incomplete. This patch preserves the scale inside mo_win_truncate, but baseBindColRef still constructs both boundary columns as a hard-coded TIMESTAMP with Scale: 0 (pkg/sql/plan/base_binder.go:400-413). The new BVT output itself exposes the mismatch: its headers are _wstart[93,64,0] / _wend[93,64,0] while their values contain microseconds. The TIME_WINDOW runtime emits boundaries using node.Timestamp.Typ, so the runtime vector and planner-visible type now disagree. Client metadata and schema-producing consumers such as CTAS can consequently declare/drop/round these sub-second values as scale 0. Please propagate the time-window timestamp/group scale to the boundary expressions and add a plan/BVT assertion that checks the boundary type (ideally including CTAS/materialization), not only the printed values.
[P2] Please remove the unrelated 126-line refresh of test/distributed/cases/ddl/comprimary_key.result. It changes formatting and unordered row order for a DDL case that this PR does not touch, making the functional diff harder to audit and potentially blessing nondeterministic output.
XuPeng-SH
left a comment
There was a problem hiding this comment.
[P1] Do not round sub-second window boundaries back to the source scale
The new validator accepts MICROSECOND for every temporal input, but mo_win_truncate preserves args[0].Typ.Scale, while timewin.newTsExpr casts both _wstart and _wend back to TsType (the source column type). A window finer than that scale therefore loses its boundary precision.
On exact head 1f20868cab514eb6b11f7e67fcae7ab6d8de6bd4, I exercised the operator with a DATETIME(0) value at 2026-08-12 10:00:00, INTERVAL=SLIDING=1 MICROSECOND, and both boundaries enabled. Internally the bounds differ by 1 microsecond, but the emitted vectors had end-start == 0: both values were rounded to the same second. The SQL form ... INTERVAL(ts,1,MICROSECOND) is now admitted, so this reaches the public path and returns an invalid zero-width visible window; adjacent sliding boundaries can collapse for the same reason.
Please either reject a resolution the timestamp/datetime result scale cannot represent, or promote the helper and boundary result type to a sufficient FSP (normally 6 when MICROSECOND participates). Add public/operator regressions for scales 0, 3, and 6, asserting _wend-_wstart and adjacent _wstart values rather than only internal grouping.
LeftHandCold
left a comment
There was a problem hiding this comment.
Reviewed exact head 1f20868. Two production-reachable correctness blockers remain:
_wstart/_wendare still bound asTIMESTAMPscale 0 inbase_binder.go, so the public expression metadata does not describe the newly emitted microsecond values. This can lose precision when the result is materialized or consumed through metadata-sensitive paths.MICROSECONDis accepted for temporal inputs of any scale, while execution casts generated boundaries back to the source timestamp type. ForDATETIME(0)/ lower-scale inputs, distinct microsecond boundaries can collapse to the same visible value, including zero-width windows.
Please propagate a sufficient boundary scale end-to-end, or reject intervals finer than the representable input/result precision, and add coverage for input scales 0/3/6 plus materialized result types. The arithmetic/GCD changes otherwise look consistent and current CI is green. The unrelated comprimary_key.result churn should also be removed from this PR.
| } | ||
| switch typ { | ||
| case types.Second, types.Minute, types.Hour, types.Day: | ||
| case types.MicroSecond, types.Second, types.Minute, types.Hour, types.Day: |
There was a problem hiding this comment.
P1: This newly admits microsecond windows for temporal inputs of every scale, but execution still casts _wstart / _wend through node.Timestamp.Typ, and the binder still exposes those columns as scale-0 TIMESTAMP. With DATETIME(0) and similarly lower-scale inputs, distinct 1µs boundaries collapse to the same visible value. Please either promote the boundary/result type to sufficient precision end-to-end or reject intervals finer than that type can represent; tests should cover scales 0/3/6 and assert both values and output metadata.
What type of PR is this?
Which issue(s) this PR fixes:
issue #26983
What this PR does / why we need it:
此分支主要支持 Time Window 的 MICROSECOND 亚秒窗口。
修改内容: