Fix GetDateTimeOffset: return the stored instant for DST-ambiguous timestamps

[polyglotAI-bot]

Description

Fixes #515.

ClickHouseDataReader.GetDateTimeOffset(int) did not read the offset from the stored
instant. It decoded the column to a wall-clock DateTime (Kind=Unspecified for a
timezone-aware column) and then re-interpreted that wall clock in the column timezone via
AbstractDateTimeType.CoerceToDateTimeOffset(DateTime) → NodaTime AtLeniently. Inside a
DST fall-back hour a local wall clock is ambiguous — it occurs twice, at two different
offsets — and AtLeniently always resolves the ambiguity to the earlier offset. That is
right for the first occurrence and wrong for the second, and the information needed to
tell them apart had already been discarded by the round trip through the wall clock. The
result was a DateTimeOffset one hour before the instant the server stored (e.g.
2025-11-02T01:30:00-04:00 instead of -05:00 for America/New_York), i.e. a genuinely
different point in time, not an equivalent representation of the same one.

The fix keeps the instant instead of trying to recover it. ClickHouseType gains an
internal ReadWithInstant that reads a value exactly like Read while also reporting the
absolute Instant it decoded (DateTimeType/DateTime64Type report it, NullableType
delegates after the null byte, everything else reports null). ClickHouseDataReader
decodes date/time columns through it and GetDateTimeOffset converts the captured instant
with the same Instant → DateTimeOffset conversion the type already uses, so the accessor
now agrees with the stored instant for both occurrences of an ambiguous wall clock.

Reproduces on DateTime('<zone>'), DateTime64(p, '<zone>'), and their Nullable(...)
forms; every other input keeps the value and offset it had before (the first occurrence of
the ambiguous hour, non-ambiguous instants, zero-offset-at-that-instant zones, columns with
no timezone, and Date/Date32/Time columns, which carry no instant and still take the
old coercion path).

Changes

• ClickHouse.Driver/Types/ClickHouseType.cs — new internal virtual object ReadWithInstant(ExtendedBinaryReader, out Instant?); the default reports no instant and delegates to Read.
• ClickHouse.Driver/Types/DateTimeType.cs, DateTime64Type.cs — override it to report the decoded instant (DateTime64Type.FromClickHouseTicks now goes through a new ToInstant, so the tick→instant conversion has a single definition). No extra timezone resolution or DateTimeOffset construction happens on the row-decode path: only the Instant is kept, and the DateTimeOffset is built in the accessor exactly as before.
• ClickHouse.Driver/Types/NullableType.cs — override it to pass through to the underlying type after the null byte, so Nullable(DateTime(...)) is covered too (a NULL reports no instant).
• ClickHouse.Driver/Types/AbstractDateTimeType.cs — ToDateTimeOffset(Instant) is now internal so the reader can use the type's own conversion.
• ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs — precompute the date/time ordinals once per result set (BuildDateTimeColumns; null when the result set has none, in which case the row loop is byte-for-byte the old one) and capture their instants per row; GetDateTimeOffset prefers the captured instant. With an IReadValueConverter configured the converter may replace the value, so the captured instant is used only while the value the caller sees still matches the one it was captured for; otherwise the old coercion is used.
• CHANGELOG.md / RELEASENOTES.md — Bug Fixes entry.

Test

New fixture ReadDateTimeOffsetAmbiguousDstTests in
ClickHouse.Driver.Tests/Types/TimezoneHandlingTests.cs (integration, TestCaseSource
following the existing ReadDateTimeFixedUtcOffsetTests style). Expected instants/offsets
are hard-coded from the IANA rules for the 2025 America/New_York fall-back day
(1762061400 = 05:30Z, -04:00; 1762065000 = 06:30Z, -05:00; both render as local
01:30:00, verified against the server), not read back from the driver.

• The reported bug for DateTime, DateTime64(3) and Nullable(DateTime): the instant and offset must be the stored ones.
• Contrast cases that pin pre-existing behaviour: the first occurrence of the same wall clock (-04:00), a non-ambiguous summer instant, and Europe/London's fall-back hour where the offset is zero at that instant (so the value keeps Kind=Utc and a zero offset).
• A mixed-column row (String, DateTime('tz'), Nullable(DateTime('tz')) NULL, Date, DateTime64(3,'tz'), Int32, String) asserting every column still decodes correctly — the new decode path must stay byte-aligned — and that a Date column still reports a zero offset.
• A multi-row test asserting the offset always belongs to the current row.

5 of the 9 cases fail on main and all 9 pass with the fix. Full suite green:
dotnet test ClickHouse.Driver.Tests --framework net10.0 → 9779 passed, 0 failed
(142 skipped), no existing test edited or weakened.

Pre-PR validation gate

• Deterministic repro confirmed (5 new cases fail on main, pass with the fix)
• Root cause documented above
• Fix targets the root cause (the instant is kept rather than re-derived from an ambiguous wall clock)
• Test fails without fix, passes with fix
• No existing tests broken, none edited
• Convention compliance verified per AGENTS.md (read + write paths considered — the write path's lenient resolution of an Unspecified wall clock is unchanged and still correct there; Release build produces no new analyzer warnings; CHANGELOG + RELEASENOTES updated)
• No public API change (GetDateTimeOffset's signature is unchanged; the new member is internal), so PublicAPI/*.txt is untouched

[codecov]

Codecov Report

❌ Patch coverage is 83.01887% with 9 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...ckHouse.Driver/ADO/Readers/ClickHouseDataReader.cs	66.66%	1 Missing and 3 partials ⚠️
ClickHouse.Driver/Types/NullableType.cs	62.50%	2 Missing and 1 partial ⚠️
ClickHouse.Driver/Types/DateTime64Type.cs	85.71%	1 Missing ⚠️
ClickHouse.Driver/Types/RowInstantReader.cs	95.23%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[Copilot]

Pull request overview

This PR fixes ClickHouseDataReader.GetDateTimeOffset(int) for DST fall-back ambiguous timestamps by preserving and reusing the decoded absolute instant (rather than re-interpreting an ambiguous wall-clock DateTime), ensuring the returned DateTimeOffset matches the stored instant for timezone-aware DateTime/DateTime64 columns (issue #515).

Changes:

• Added an internal ReadWithInstant(..., out Instant?) decode hook to capture decoded instants for types that encode them.
• Updated the ADO reader to capture per-row instants for date/time columns and prefer them in GetDateTimeOffset.
• Added integration regression tests for DST-ambiguous instants and updated CHANGELOG/RELEASENOTES.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
RELEASENOTES.md	Documents the DST-ambiguous GetDateTimeOffset bug fix (issue #515).
CHANGELOG.md	Adds the corresponding unreleased bug-fix entry.
ClickHouse.Driver/Types/ClickHouseType.cs	Introduces ReadWithInstant to optionally report a decoded Instant.
ClickHouse.Driver/Types/DateTimeType.cs	Overrides ReadWithInstant to report the decoded Instant.
ClickHouse.Driver/Types/DateTime64Type.cs	Centralizes tick→instant conversion and overrides ReadWithInstant to report the decoded Instant.
ClickHouse.Driver/Types/NullableType.cs	Ensures nullable date/time types can propagate instants through the null-byte wrapper.
ClickHouse.Driver/Types/AbstractDateTimeType.cs	Exposes ToDateTimeOffset(Instant) internally for reuse by the reader.
ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs	Captures per-row instants during decoding and uses them in GetDateTimeOffset.
ClickHouse.Driver.Tests/Types/TimezoneHandlingTests.cs	Adds regression coverage for ambiguous DST instants and mixed-column alignment/row isolation.

[alex-clickhouse]

I don't like the design with the instantColumns and rowInstants in the Reader...what are the options here in terms of the structure, is it possible to encapsulate this fix in the DateTime types?

[polyglotAI-bot]

Fair objection — I've restructured it in d585059 so the reader no longer knows anything about instants. Options I considered, and why this one:

1. Fully inside the date/time types, no reader involvement at all — not possible.
The offset has to survive from decode to GetDateTimeOffset, and the decoded value can't carry it: for a tz-aware column ToDateTime returns a DateTimeKind.Unspecified wall clock, and inside a fall-back hour that wall clock maps to two offsets. So the instant has to be remembered somewhere per row.

It can't be remembered on the type instance: the same instance decodes every row and every element of a composite column (Array(DateTime('tz'))), and type instances are shared — TypeConverter hands out the same object for ReverseMapping/template lookups, so per-row mutable state on a type wouldn't be per-result-set and wouldn't be thread-safe. The only object with the right lifetime is the reader's row buffer.

2. Store a DateTimeOffset in the row buffer instead of a DateTime (so no side state at all). This changes what GetValue, GetValues, GetFieldValue<T>, POCO binding and IReadValueConverter see for every timezone-aware column — a user-visible break for the common path to fix an edge case. Rejected.

3. What's pushed now: the reader delegates, the types own the logic.

• New internal IInstantReader (Types/IInstantReader.cs) — implemented explicitly by DateTimeType (so DateTime32), DateTime64Type, and by NullableType which just delegates to its underlying type. Nothing about instants is on ClickHouseType anymore; that file is back to its original content.
• New Types/RowInstantReader.cs owns the per-column capability map, the per-row instants and the conversion to a DateTimeOffset. RowInstantReader.Create(types) returns null when no column decodes an instant, which is also how the Date/Date32 fast path from the earlier review comment is kept.
• ClickHouseDataReader keeps one field, rowInstantReader, forwards the row loop to it and asks it for the offset. instantColumns, rowInstants and the NodaTime import are gone from the reader.

Also shortened the changelog entry per AGENTS.md ("keep entries short, no root-cause analysis").

Behavior is unchanged from the previous head — full suite still green on net10.0 (9786 passed, 142 skipped, 0 failed). The regression test that pinned the type-capability flag now pins the same thing through RowInstantReader.Create, which is where the decision is actually made.

Two related gaps I deliberately did not fold in here: GetDateTimeOffset still throws for LowCardinality/SimpleAggregateFunction-wrapped date/time columns, and GetFieldValue<DateTimeOffset> doesn't go through this accessor at all. Both pre-date this PR; happy to file/fix them separately if you'd like.

[polyglotAI-bot]

Rebased/merged main in to clear the conflict (main advanced with #501, which replaced the shared Unreleased section with changelog.d/ fragments).

Resolution: CHANGELOG.md and RELEASENOTES.md are now byte-identical to main (this branch no longer touches either — RELEASENOTES.md is generated and the Unreleased section is off-limits per changelog.d/README.md), and the entry moved to a fragment: changelog.d/517-getdatetimeoffset-dst-instant.fixes.md. dotnet run scripts/changelog.cs -- --check passes (41 pending fragments) and --render places the entry under Bug Fixes:.

No code changes in this push beyond the merge itself. Full suite green on the merged tree (net10.0: 10174 passed, 142 skipped, 0 failed).

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 31e8508. Configure here.}

[polyglotAI-bot]

Rebuilt on current main (merge commit 2de97f8) — the PR is mergeable again.

The conflict was semantic, not textual: #449 (box-free POCO read fast path) reworked the date/time read API, so this branch was re-expressed on top of it.

• DateTimeType / DateTime64Type now implement main's ReadDateTime / ReadDateTimeOffset overrides (the old Read override is gone) and keep the explicit IInstantReader implementation next to them. Both share the one ReadInstant(reader) helper, so DateTime64Type no longer had two tick-to-instant conversions.
• AbstractDateTimeType.ToDateTimeOffset(Instant) stays internal (rather than main's protected) because RowInstantReader reports a captured instant through it. Subclasses still reach it, being in the same assembly.
• ClickHouseDataReader keeps main's unconditional columnTypeNames = rawTypeNames, plus the RowInstantReader.Create(types) line.

No behavior change from the merge. Full suite green on net10.0: 10505 passed, 142 skipped, 0 failed. dotnet run scripts/changelog.cs -- --check OK; the changelog.d/517-...fixes.md fragment is unchanged.

Still waiting on a review of the IInstantReader / RowInstantReader design.

[polyglotAI-bot]

Rebased onto main (merge commit 367570e). The conflict was semantic: main (#499) replaced the reader's shared object[] row buffer with per-column ColumnSlots, and this branch decoded rows through its own loop. The fix is re-expressed on the slot architecture, which also answers the earlier structural objection — the reader now holds no instant state at all:

• IInstantReader is reduced to Instant ReadInstant(reader), implemented explicitly by DateTimeType and DateTime64Type. Date/Date32 do not implement it; NullableType no longer needs to, because NullableSlot consumes the null marker and ColumnSlotFactory unwraps.
• New InstantCapturingReader : ITypedReader<DateTime> — one instance per column per reader, so the instant is per-result-set state, never on the shared type instance. It decodes exactly what the type's own DateTime read produces and keeps the instant it came from.
• ColumnSlotFactory binds a DateTime/DateTime64 column's slot to that reader; every other column keeps its own typed reader, so nothing else changes. RowInstantReader is deleted.
• GetDateTimeOffset reads the instant off the slot and falls back to the previous coercion when there is none (Date/Date32, a NULL cell, or a converter that changed the value).

Verification: full suite green on net10.0 — 10800 passed, 142 skipped, 0 failed. The regression fixture fails 8 of 19 cases with the capture disabled and passes with it. Added cases from a self-review pass: DateTime64 scale 0 and 9, and a NULL-after-value row proving a captured instant is never reported for a NULL cell.

Note for CI: main currently does not compile (TryGetEnumOrdinal still reads the removed CurrentRow); #566 fixes it. This branch inherits that failure and does not include the fix, which belongs to #566. Local verification applied #566 on top, then reverted it before committing.

No approval was dismissed — the PR had none (reviewDecision: REVIEW_REQUIRED).