ClickHouse / ClickHouse/clickhouse-cs
GetDateTimeOffset returns the wrong instant for DST-ambiguous timestamps: it coerces from a wall-clock DateTime instead of reading the stored instant
- Dominant language
- C#
- Stars
- 94
- Forks
- 22
- Avg merge
- 11h 26m
- Merged PRs (30d)
- 22
Description
### Describe the bug
`ClickHouseDataReader.GetDateTimeOffset(int)` returns the wrong **instant** — off by one hour — for timestamps that land in the later half of a DST fall-back hour in a timezone-aware `DateTime`/`DateTime64` column.
The accessor does not decode the offset from the stored instant. It decodes the column to a wall-clock `DateTime` with `DateTimeKind.Unspecified` and then *re-interprets* that wall clock in the column's timezone:
https://github.com/ClickHouse/clickhouse-cs/blob/main/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs — `GetDateTimeOffset`:
```csharp
public virtual DateTimeOffset GetDateTimeOffset(int ordinal) => GetEffectiveClickHouseType(ordinal) is AbstractDateTimeType adt ?
adt.CoerceToDateTimeOffset(GetDateTime(ordinal)) : throw new InvalidCastException();
```
`AbstractDateTimeType.CoerceToDateTimeOffset(DateTime)` resolves an `Unspecified` kind through NodaTime's lenient resolver:
```csharp
_ => TimeZoneOrUtc.AtLeniently(LocalDateTime.FromDateTime(value)).ToDateTimeOffset(),
```
During a fall-back transition a local wall-clock time is **ambiguous** — it occurs twice, at two different offsets. `AtLeniently` always resolves the ambiguity to the *earlier* offset. That is correct for the first occurrence and wrong for the second, and the information needed to tell them apart was already discarded by the round trip through `Unspecified`.
The type itself decodes this correctly elsewhere: `DateTimeType.ReadDateTimeOffset` / `DateTime64Type.ReadDateTimeOffset` go straight from the decoded `Instant` (`ToDateTimeOffset(ReadInstant(reader))`) and never re-interpret a wall clock. Only the ADO accessor takes the lossy route.
### Steps to reproduce
1. Create a table with a timezone-aware `DateTime` column in a zone that observes DST.
2. Insert an instant that falls in the **second** occurrence of a fall-back hour.
3. Read it back with `ClickHouseDataReader.GetDateTimeOffset(ordinal)`.
### Expected behaviour
`GetDateTimeOffset` should return the same `DateTimeOffset` the column's own reader produces — i.e. the offset derived from the stored instant, preserving the instant exactly.
### Code example
```sql
CREATE TABLE dst_bug (ts DateTime('America/New_York')) ENGINE = Memory;
-- 2025-11-02 06:30:00 UTC = 01:30:00 -05:00, the *second* occurrence of local 01:30
INSERT INTO dst_bug VALUES (toDateTime('2025-11-02 06:30:00', 'UTC'));
```
```csharp
using var reader = await client.ExecuteReaderAsync("SELECT ts FROM dst_bug");
await reader.ReadAsync();
var dto = ((ClickHouseDataReader)reader).GetDateTimeOffset(0);
// actual: 2025-11-02T01:30:00-04:00 (2025-11-02T05:30:00Z) <-- one hour early
// expected: 2025-11-02T01:30:00-05:00 (2025-11-02T06:30:00Z)
```
Verified at the type level, without a server, by decoding the same wire bytes two ways:
| wire seconds | route | result |
|---|---|---|
| `1762065000` | `CoerceToDateTimeOffset(ReadDateTime(...))` (what `GetDateTimeOffset` does) | `2025-11-02T01:30:00-04:00` ❌ |
| `1762065000` | `((ITypedReader)type).ReadValue(...)` | `2025-11-02T01:30:00-05:00` ✅ |
The two results differ by an hour of real elapsed time — a genuinely different instant, not an equivalent offset representation.
### Scope
* Reproduces on both `DateTime('')` and `DateTime64(p, '')` (confirmed with `DateTime64(3, 'America/New_York')`, wire millis `1762065000000`).
* Limited to **DST-ambiguous instants**, specifically the later of the two occurrences. The first occurrence (`1762061400`) agrees on both routes, because the lenient resolver happens to pick the offset that occurrence already had. A non-ambiguous instant in the same zone (e.g. `2025-07-15 12:30:00` EDT) round-trips identically.
* The spring-forward **gap** is not reachable on the read path: `ReadDateTime` derives its value from a real `Instant`, and every real instant maps to exactly one non-gap wall-clock time in any zone. `AtLeniently`'s gap handling (shifting forward) is only observable on the write/insert path, for a caller-supplied `Unspecified` `DateTime` that itself falls in a gap.
* Columns with no timezone are unaffected (`TimeZoneOrUtc` is UTC, which has no transitions).
* `GetDateTime` is unaffected — the wall clock it returns is correct; only the offset attached to it by the ADO accessor is wrong.
* `ITypedReader` is unaffected, and structurally so: `AbstractDateTimeType.ReadDateOnly` *is* `DateOnly.FromDateTime(ReadDateTime(reader))` and is not overridden by either timezone-aware subtype, so the projection and the typed read are the same code.
### Suggested fix
Have `GetDateTimeOffset` read the offset from the instant rather than reconstructing it from a wall clock — i.e. route it through the type's own `ITypedReader` / `ReadDateTimeOffset` rather than through `GetDateTime` + `CoerceToDateTimeOffset`. `CoerceToDateTimeOffset(DateTime)` is the right thing on the *write* path, where the caller genuinely supplies a wall clock and the ambiguity has to be resolved by policy; it is the wrong thing on the read path, where the unambiguous answer is still available in the bytes.
Worth adding a regression test that decodes the same wire value through both routes and asserts they agree, parameterised over a fall-back window — the divergence is invisible to any test that only uses non-ambiguous timestamps.
### Configuration
Not environment-specific — the divergence is in decode logic and reproduces without a server.
#### Environment
* Client version: `main` (present since at least the current release; unrelated to the in-flight box-free read work in #449/#499, which do not touch either method)
* .NET version: reproduced on net9.0; not framework-specific
Contributor guide
Research direction
Start in ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs and compare GetDateTimeOffset with AbstractDateTimeType.ReadDateTimeOffset, DateTimeType.ReadDateTimeOffset, and DateTime64Type.ReadDateTimeOffset. Add a regression test using the fall-back wire value described in the issue, covering both DateTime and DateTime64, and verify that the ADO accessor preserves the instant and agrees with the typed reader.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 78/100