Repository navigation
fix(storage): use calendar days for S3 ranges - #690
Conversation
0520f46 to
d55fb9a
Compare
xe-nvdk
left a comment
There was a problem hiding this comment.
Thanks for this, and for the clear AI-assistance disclosure and the honest Windows test-environment notes; both are exactly what CONTRIBUTING asks for. Your mechanical analysis of Truncate(24h)/Add(24h) is correct, the reproduction in the PR body is real, and the diff is tight. The change request is about which time zone the fix should anchor to, and it is a substantive one.
Arc's partition layout is UTC. Ingestion writes {db}/{measurement}/{Y}/{M}/{D}/{H} directories from UTC timestamps, the partition pruner generates UTC paths, and as of this release the query engine's session zone is pinned to UTC as well. The correct output of this function is therefore the set of UTC calendar days covering the range, regardless of what zone the callers' time.Time values carry.
Anchoring to startTime.Location() produces silently wrong sets for non-UTC inputs. Concrete case: a range ending 2026-11-02 20:00 America/New_York is 2026-11-03 01:00 UTC, so matching rows live in the 2026/11/03 partition; the local-day walk stops at 11/02 and that partition is never listed. West-of-UTC ranges lose their trailing UTC day (silent data exclusion when a caller uses this for pruning) and gain a spurious leading one. The pre-fix code was broken differently (it rendered UTC-truncated instants through the input's zone, which is your Oct 30 artifact), so this PR replaces one wrong zone contract with another rather than fixing the underlying one.
One more thing worth knowing: this function currently has no callers in the codebase, so your tests will BE the contract the next caller inherits. That raises the stakes on getting the zone right rather than lowering them.
Requested change: normalize both boundaries to UTC and walk UTC calendar days. Your structure is otherwise right, roughly:
start := startTime.UTC()
end := endTime.UTC()
y, m, d := start.Date()
current := time.Date(y, m, d, 0, 0, 0, 0, time.UTC)
y, m, d = end.Date()
last := time.Date(y, m, d, 0, 0, 0, 0, time.UTC).AddDate(0, 0, 1)
for current.Before(last) { ...; current = current.AddDate(0, 0, 1) }(AddDate in UTC is equivalent to Add(24h), which is fine; keeping AddDate documents the intent.) Please also flip the tests to assert UTC-day output for non-UTC inputs, e.g. the NY 20:00 case above must include the 11/03 path, and update the release-notes wording ("in the range's starting time zone" becomes UTC). The mixed-location test stays meaningful: both inputs normalize to the same UTC day set.
Verified on the branch meanwhile: builds clean, tests pass as written including under other host zones. Happy to merge quickly once the anchor is UTC.
|
Merging as-is: your DST analysis and test scaffolding are solid, and since this function currently has no callers there is no runtime risk. The zone-anchoring change from my review (UTC calendar days rather than the start time's location, see the worked example there) lands as an immediate maintainer follow-up so you can see the exact delta; the short version is that Arc's partition directories are UTC dates, so path generation must normalize to UTC regardless of the inputs' zones. Thanks for the contribution and the clean disclosure. |
Follow-up to #690: Arc's partition directories are UTC dates, so GetQueryPathRange must normalize both boundaries to UTC before walking calendar days. Anchoring to the start time's location skipped the trailing UTC partition for west-of-UTC ranges (a New York range ending Nov 2 20:00 is Nov 3 01:00 UTC, and the 11/03 partition was never listed) and added a spurious leading one. Tests now pin UTC-day output for non-UTC and mixed-zone inputs, including that boundary case, and keep the DST-weekend coverage. Refs #321, #690
Summary
Problem
time.Time.Truncate(24*time.Hour)truncates elapsed time from the zero instant rather than finding local midnight, andAdd(24*time.Hour)can cross a daylight-saving transition without advancing to the next local date. On currentmain, an Oct 31–Nov 2 New York range produced Oct 30, Oct 31, and Nov 1 paths.Approach
Construct midnight boundaries in
startTime.Location(), convertendTimeinto that same location, and advance withAddDate(0, 0, 1). This preserves the existing inclusive-day contract while making the calendar semantics explicit.Verification
gofmtclean for the changed Go filesgo vet ./internal/storagego test ./internal/storage -count=1TZ=America/New_York go test ./internal/storage -count=1The repository-wide
go test ./...was attempted but is not runnable in this Windows environment:CGO_ENABLED=0, no C compiler is installed, DuckDB's Windows binding files are excluded, and SQLite tests report thatgo-sqlite3requires cgo. The focused storage package is cgo-independent and passes; Linux CI remains authoritative perCONTRIBUTING.md.Risk and exclusions
Low risk: one date-iteration function and focused tests. No storage I/O, API shape, dependencies, or partition format changed. Out of scope: validation policy for an end instant earlier than the start instant.
AI assistance disclosure: this change was produced through an autonomous coding agent, independently reviewed, and verified with the commands above.
Closes #321