Repository navigation
perf(function): avoid calendar decomposition for fixed-unit DATE_TRUNC - #31004
LeftHandCold wants to merge 2 commits into
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? |
LeftHandCold
left a comment
LeftHandCold
left a comment
There was a problem hiding this comment.
代码复核：建议通过，未发现需要 Request Changes 的确定性问题。审查 HEAD：af00ad672a186786720edc96e403497306e2fdd3。
设计判断：局部无状态算术优化，不改变函数接口、类型或时区职责。
逐项核对：
- 仅有效的非负 DATETIME 范围走秒/分/时/日的微秒取余；该表示从公历午夜起算，因此固定单位边界与原日历分解一致。
- ZeroDatetime、负原始值、越界原始值和月/季/周/年仍走原处理；没有把负数截断或超范围兼容行为偷偷改掉。
- DATE 的子日单位拒绝仍在调用层；TIMESTAMP 的时区、夏令时折叠和回转逻辑仍由原调用层负责，不是直接对 UTC 值按当地日取模。
- 新测试的日历 oracle 独立于新算法，覆盖闰年、世纪、1924 边界、年 1/9999、微秒和 NULL/错误顺序。未引入分配、锁或长期状态。
验证边界：已核对当前 HEAD 的成功 CI 40023230517，并完成 diff --check。本次为代码与既有证据复核，没有重新运行 TSBS；PR 中的收益数字仍限定于作者测量场景。
当前认证账号也是 PR 作者，故以 COMMENT 记录建议通过；这不是 GitHub 正式 Approval。
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: af00ad672a186786720edc96e403497306e2fdd3
Assessment: approve
Reviewed the complete three-file delta and affected temporal types, SQL overloads, and vector callers. The guarded arithmetic preserves calendar truncation for valid DATETIME values while retaining raw-value fallback, DATE restrictions, NULL/error ordering, and TIMESTAMP timezone/DST handling. The optimization belongs in the existing owner and introduces no allocation or retained state. No actionable findings remain.
Model: gpt-6-astra / xhigh
Session: 01a13dcf-1586-7a01-9000-d45e20995cf8 · slot 1 · generation 1
Validation:
-
Reviewed Gregorian-oracle tests, epoch/year-9999 boundaries, leap/century cases, negative/year-65537 fallback controls, vector NULL/zero/error behavior, benchmark quality, and existing DATE/DST and SQL regression coverage.
-
Reused frozen exact-head successful UT, SCA, shared build, coverage, proxy BVT, and pessimistic multi-CN BVT statuses. Repository UT selection includes pkg/sql/plan/function; skipped jobs were not counted as passes.
-
Local static audit passed revision/base verification, source cleanliness, complete patch scope, git diff --check, and gofmt. Receipt: checks/1792809017213820158-4.json; retained log: review-static.log.
-
Performance tracing confirms removal of calendar decomposition for fixed units without new allocations. Calendar/raw-value fallback adds bounded guard/dispatch work; paths not using DATE_TRUNC are unchanged.
-
Saved review-assessment.json and review-evidence.json in the assigned diagnostic directory.
Limitations:
-
No local Go tests or benchmarks were rerun. local_validation is empty; frozen CI supplies job outcomes rather than per-test logs or author execution receipts.
-
Author-reported TSBS medians improved from 302.1 to 250.0 ms (Q1), 340.2 to 278.3 ms (Q2), and 352.6 to 287.1 ms (Q5). Raw measurements were unavailable; these isolated-patch results were not independently reproduced and do not establish combined-patch gains.
-
The linked issue #31003 body was absent; its contract was assessed through the frozen PR description and discussion.
|
Queued — the merge queue status continues in this comment ↓. |
Merge Queue Status
Waiting for any of
All merge conditions
Waiting for
All queue conditions
|
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #31003.
What this PR does / why we need it:
DATE_TRUNCdecomposes and rebuilds the calendar for every row, including fixed second/minute/hour/day boundaries. For valid DATETIME values, clear the remainder of the corresponding microsecond period directly. Keep the original fallback for raw invalid values and calendar units. Vector NULL/error handling, DATE validation and TIMESTAMP timezone/DST handling are unchanged. The production change is 15 lines and adds no allocation or retained state.On the original TSBS CPU-only dataset (8.64 million measurement rows, 1,000 tags), alternating A/B/B/A with ten measured samples per version/query gives:
All five original queries retain complete source-checked outputs. Q3/Q4 controls do not use DATE_TRUNC; their median wall changes are +1.36 ms/+0.59% and CPU changes +0.83%/+0.17%. The whole engine/client/validator shares four CPU cores and a 12 GiB, no-swap cgroup. Whole-block memory peaks are 1.478–1.532 GiB with zero OOM/limit events. Profiles were collected separately from timing. This A/B isolates only this patch on
b78794175e65; Top-1 and other candidate optimizations are not combined.Validation:
af00ad672ais rebased onto verified main1651eec948. All three changed source/test files are byte-identical across the rebase; datetime callers/types/vector/registration are unchanged. Full owning-package tests, gofmt/diff checks, scoped vet, configured incremental golangci-lint and native-provenance checks all pass on this delivery head.This is a draft for review. Measurements establish the stated CPU-only improvement; DevOps/IoT and broader benchmark optimization remain separate work.