Skip to content

fix sub-second units - #27017

Open
daviszhen wants to merge 7 commits into
matrixorigin:mainfrom
daviszhen:0812-fix-sub-second
Open

fix sub-second units#27017
daviszhen wants to merge 7 commits into
matrixorigin:mainfrom
daviszhen:0812-fix-sub-second

Conversation

@daviszhen

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #26983

What this PR does / why we need it:

此分支主要支持 Time Window 的 MICROSECOND 亚秒窗口。

修改内容:

  • Time Window 执行器支持 MICROSECOND 单位。
  • interval() helper 支持微秒换算。
  • mo_win_divisor() 改为按微秒计算 sliding 对齐粒度。
  • sliding window 的内部 truncate helper 改为使用 MICROSECOND 单位。
  • mo_win_truncate 保留 timestamp(6) / datetime(6) 的 scale,避免分桶时丢失小数秒。
  • 新增 UT 覆盖微秒换算、truncate、sliding 边界和聚合结果。
  • 新增 BVT 覆盖:
    • INTERVAL(..., MICROSECOND)
    • SLIDING(..., MICROSECOND)
    • GAPFILL(PARTITION) + FILL(VALUE, ...)
    • TIMESTAMP(6) / DATETIME(6) 微秒边界。

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@aptend aptend left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 LeftHandCold left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 1f20868. Two production-reachable correctness blockers remain:

  1. _wstart / _wend are still bound as TIMESTAMP scale 0 in base_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.
  2. MICROSECOND is accepted for temporal inputs of any scale, while execution casts generated boundaries back to the source timestamp type. For DATETIME(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:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/M Denotes a PR that changes [100,499] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants