Skip to content

Map DateTimeOffset to DateTime64 and fix composite component reads (#53) - #63

Draft
alex-clickhouse wants to merge 4 commits into
mainfrom
fix/issue-53-datetimeoffset
Draft

Map DateTimeOffset to DateTime64 and fix composite component reads (#53)#63
alex-clickhouse wants to merge 4 commits into
mainfrom
fix/issue-53-datetimeoffset

Conversation

@alex-clickhouse

Copy link
Copy Markdown
Collaborator

Fixes #53.

Problem

The provider had no mapping for DateTimeOffset. EF Core therefore fell back to
DateTimeOffsetToStringConverter through the value-converter selector, and made a String column
without a warning. Two failures came from this:

  • A query against a real DateTime64 column failed with TYPE_MISMATCH, because the provider
    declared the parameter as String.
  • SaveChanges could not write the value at all.

Solution

A DateTimeOffset property now maps to DateTime64(7, 'UTC') through the new
ClickHouseDateTimeOffsetTypeMapping. No value converter is used, so the driver receives the
DateTimeOffset directly on the query parameter path and on the bulk insert path.

Three design decisions are important:

The store type pins the timezone to 'UTC'. For a timezone-less parameter type such as
DateTime64(7), the driver sends a UTC wall clock, and the server then reads it in
session_timezone. The instant moves when that setting is not UTC. A UTC-pinned store type removes
this dependency on server configuration.

Precision 7 is one .NET tick (100 ns). The round trip is therefore exact, and no value comes
back truncated. Precision 7 also covers the full DateTimeOffset range, so MinValue and MaxValue
work as open-ended range limits.

The offset is not kept. ClickHouse has no type that stores a UTC offset. DateTime64 holds an
instant, and a declared timezone only decides how that instant is rendered. The instant is preserved
and the offset is not, so a value read back carries the offset of the column's declared timezone.
The README records this, together with the alternative for a caller who must keep the offset.

Also in this PR: three composite-mapping defects

Work on #53 exposed these. They are in the same PR because DateTimeOffset inside a composite type
does not work without them, and because they touch the same resolution path in
ClickHouseTypeMappingSource.

  1. Composite columns did not convert their components on read. Array(T), Map(K, V) and
    Tuple(...) read the whole column through GetValue, so a component mapping's own read pipeline
    never ran. Any component whose CLR type differs from the driver's type threw
    InvalidCastException. This was not new with DateTimeOffsetDateOnly[],
    Dictionary<string, DateOnly> and Tuple<DateOnly, …> were already affected, because DateOnly
    also arrives from the driver as a DateTime.

    The new ClickHouseComponentConversion helper rebuilds the composite component by component. It
    applies the same two steps EF Core applies to a scalar column: the mapping's data-reader
    conversion, then its ValueConverter. A component that needs no conversion keeps the direct
    cast, and a runtime fast path returns the driver's array untouched when it is already the target
    type.

  2. Array(Nullable(T)) DDL was double-wrapped for a value-type element, which gave
    Array(Nullable(Nullable(T))). ClickHouse rejects this with Nested type Nullable(T) cannot be inside Nullable type, so EnsureCreated and migrations both failed.

  3. A component mapping ignored the CLR component type. One store type can serve more than one
    CLR type: DateTime64 serves DateTime and DateTimeOffset, and Date32 serves DateTime and
    DateOnly. Resolving a component from an explicit store type always picked the default CLR type,
    which gave the composite the wrong element type and broke change tracking.

Tests

DateTimeOffsetMappingTests (680 lines) and CompositeElementConversionTests (484 lines), 59 tests
in total. All run against a real ClickHouse server through Testcontainers, as the guidance in
AGENTS.md prefers.

They cover the round trip at each precision, a non-UTC column timezone, the ambiguous
daylight-saving hour, ordering and comparison, MinValue/MaxValue, SQL literal generation,
HasPrecision, the bulk insert path, and DateTimeOffset and DateOnly inside Array, Map,
Tuple and nested composites.

Known limits

  • Writing a component that needs a ValueConverter still does not work, because the bulk insert
    path passes model values to the driver without applying converters (SaveChanges fails for value-converted properties: bulk insert path does not apply the converter #54). An enum inside a
    composite is written as its raw ordinal. Reading such a column works.
  • DateTimeOffset members such as .Year do not translate to SQL yet (No LINQ translation for DateTime members and methods #55).
  • In a column timezone with daylight saving, the repeated hour when clocks go back is ambiguous,
    because the driver gives a wall clock and drops the offset. The provider recovers the instant
    where the zone's standard offset is zero, such as Europe/London. Where both candidate offsets
    are non-zero, such as Europe/Paris, the value can read back one hour early. The default 'UTC'
    store type is not affected.

Behaviour change

A DateTimeOffset property that relied on the old String column now resolves to
DateTime64(7, 'UTC'). Add HasConversion<string>() to keep the previous shape. Note that
HasColumnType("String") on its own is not enough, because it adds no converter.

Note for the reviewer

ClickHouseEngineBuilder.cs has two XML doc comments only, which explain the relationship between
WithOrderBy and WithPrimaryKey. This is unrelated to #53. Say the word and I will take it out.

🤖 Generated with Claude Code

Map DateTimeOffset to DateTime64(7, 'UTC'). The provider had no mapping
for the type, so EF Core fell back to DateTimeOffsetToStringConverter and
silently made a String column. That broke queries against real DateTime64
columns with TYPE_MISMATCH, and SaveChanges could not write the value.

The store type pins the timezone to 'UTC'. For a timezone-less parameter
type the driver sends a UTC wall clock, which the server then reads in
session_timezone. Precision 7 is one .NET tick, so the round trip is
exact. No value converter is used, so the driver gets the DateTimeOffset
directly on the query parameter path and the bulk insert path.

ClickHouse stores no UTC offset, so the instant is kept and the offset is
not. A value read back carries the offset of the column's declared
timezone.

Also fix three defects in the composite mappings, which #53 exposed:

* Array(T), Map(K, V) and Tuple(...) read the whole column through
  GetValue, so a component mapping's read pipeline never ran. Any
  component whose CLR type differs from the driver's type therefore threw
  InvalidCastException. DateOnly components were already affected before
  DateTimeOffset existed as a mapped type. Composites are now rebuilt
  component by component, applying the data-reader conversion and then
  the ValueConverter.
* Array(Nullable(T)) DDL was double-wrapped for a value-type element,
  which ClickHouse rejects.
* A component resolved from an explicit store type always picked the
  default CLR type, because one store type can serve more than one CLR
  type. Array, Map and Tuple now pass the component CLR type.

Known limit: writing a component that needs a ValueConverter still does
not work, because the bulk insert path skips converters (#54).

Co-Authored-By: Claude <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds native DateTimeOffset support and repairs composite component materialization.

Changes:

  • Maps DateTimeOffset to UTC-pinned DateTime64(7).
  • Converts Array, Map, and Tuple components during reads.
  • Adds integration coverage and user documentation.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
ClickHouseDateTimeOffsetTypeMapping.cs Implements mapping, literals, and timezone-aware reads.
ClickHouseComponentConversion.cs Centralizes component conversion.
ClickHouseArrayTypeMapping.cs Rebuilds arrays requiring conversion.
ClickHouseMapTypeMapping.cs Rebuilds converted map entries.
ClickHouseTupleTypeMapping.cs Converts tuple components.
ClickHouseNullableElementMapping.cs Handles nullable component reads and DDL.
ClickHouseTypeMappingSource.cs Registers and resolves new mappings.
ClickHouseEngineBuilder.cs Documents sorting and primary keys.
DateTimeOffsetMappingTests.cs Tests mapping and round trips.
CompositeElementConversionTests.cs Tests composite materialization.
README.md Documents DateTimeOffset behavior.
CHANGELOG.md Records feature and fixes.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/EFCore.ClickHouse/Storage/Internal/Mapping/ClickHouseTupleTypeMapping.cs Outdated
Comment thread README.md Outdated
Comment thread CHANGELOG.md Outdated
Comment thread src/EFCore.ClickHouse/Storage/Internal/Mapping/ClickHouseMapTypeMapping.cs Outdated
@alex-clickhouse

Copy link
Copy Markdown
Collaborator Author

Let's make sure we have tests with Fixed/UTC±HH:MM:SS timezone (which clickhouse supports and .NET doesn't natively...there is support for them in the client lib).

Two review findings on #63.

A ClickHouse column can declare a fixed UTC offset instead of a named
zone, spelled `Fixed/UTC±HH:MM:SS`. No .NET timezone has such a name, so
`TimeZoneInfo.FindSystemTimeZoneById` cannot resolve it and every such
column failed to read, reporting missing host timezone data instead of
the real cause. The offset is now parsed out of the name, which also
needs no daylight-saving logic because a fixed offset never changes.

ClickHouse does not hold the minutes and seconds fields to 59. It
carries the excess, so `Fixed/UTC+05:60:00` is a legal name for the
offset +06:00, and the server accepts any name up to 24 hours in total.
The driver does not read those names, and gives a UTC wall clock rather
than one in the column's timezone, so attaching the parsed offset would
move the instant by the whole offset and report nothing. Such a name is
therefore recognised but refused, with the plain spelling to use in its
place. Widening the parser alone would have been worse than the bug.

ClickHouse also accepts offsets that DateTimeOffset cannot hold: it caps
the magnitude at 14 hours and requires whole minutes. Such a column now
reports itself and the limit it breaks, rather than failing inside the
constructor with a message that names only the rule.

Separately, the composite mappings return a driver value that already
has the target CLR type without rebuilding it. That is sound only where
matching types prove there is no work left, which holds for every
component mapping the provider resolves on its own: each one either
changes the CLR type or coerces a numeric type, which is a no-op once
the type matches. A value converter can break it, because it may change
the value and keep the type. Components that carry one are now always
rebuilt. `ElementType(el => el.HasConversion(...))` reaches this from
the public API.

Also record two limits that the docs claimed away:

- `DateTimeOffset.MinValue` and `MaxValue` only round trip through a
  column whose timezone offset is zero. Both sit at the edge of the
  `DateTime` range, and the driver builds a wall clock in the column's
  timezone to return a value, so a non-zero offset pushes one end
  outside `DateTime`. A named zone is worse than a fixed offset, because
  zones carry a Local Mean Time offset for year 1, which shifts a value
  near `MinValue` quietly instead of reporting it.
- Keeping the old String shape with HasConversion leaves the property
  read-only until #54 is fixed.
@alex-clickhouse

Copy link
Copy Markdown
Collaborator Author

Good catch — this was a real bug, and chasing it turned up a second one I would not have found otherwise.

The bug you spotted

ClickHouseDateTimeOffsetTypeMapping resolved the declared timezone with TimeZoneInfo.FindSystemTimeZoneById, which cannot resolve Fixed/UTC+05:30:00 however complete the host tzdata is. So every fixed-offset column failed to read, and the error told the user to install tzdata that would never help. Fixed by parsing the offset out of the name. No daylight-saving logic is needed, because a fixed offset does not change.

Measured against the server, the spelling is exact — which is why the parser is strict rather than lenient:

spelling server
Fixed/UTC+05:30:00 accepted
Fixed/UTC+5:30:00, Fixed/UTC+05:30, fixed/utc+05:30:00 rejected
UTC+05:30, +05:30, Etc/GMT-3-style aliases aside rejected

The trap underneath it

The driver comment says minutes and seconds are held to 00-59 so that a malformed name cannot be misread. I took that at face value and copied the bound. It is wrong: ClickHouse carries the excess. Measured with timeZoneOffset:

name offset
Fixed/UTC+05:60:00 21600 (+06:00)
Fixed/UTC+05:00:60 18060 (+05:01)
Fixed/UTC+09:99:99 38439 (+10:40:39)
Fixed/UTC+24:00:00 86400
Fixed/UTC+25:00:00 rejected

So Fixed/UTC+05:60:00 is a legal column that my first fix still mishandled. But widening the parser would have been worse than the bug: the driver has the same [0-5]\d bound, and when it fails it returns a UTC wall clock rather than one in the column timezone. Measured on one row holding 2026-03-21 14:25:36Z:

Fixed/UTC+05:60:00  ->  2026-03-21 14:25:36  Unspecified   (UTC wall clock)
Fixed/UTC+06:00:00  ->  2026-03-21 20:25:36  Unspecified   (correct wall clock)

Attaching a parsed +06:00 to the first gives 08:25:36Z — six hours wrong, silently. So the provider recognises the shape and refuses it, naming the plain spelling to use instead. It is the one case where the honest answer is an error.

Worth deciding separately whether the driver should widen its own regex; happy to raise that upstream. Note its comment is also inverted — ClickHouse itself makes 60 minutes one hour.

Two more limits, both now reported rather than hit

  • DateTimeOffset holds an offset only within ±14 h and only in whole minutes. ClickHouse accepts both a larger magnitude and seconds granularity, so Fixed/UTC+15:00:00 and Fixed/UTC+00:00:42 now report the timezone and the limit instead of failing inside the constructor.
  • Unrelated to fixed offsets, and pre-existing: DateTimeOffset.MinValue/MaxValue only round trip through a zero-offset column. Both sit at the edge of the DateTime range and the driver must build a wall clock to return a value, so any non-zero offset pushes one end outside DateTime — reading MaxValue from an Asia/Tokyo column throws. A named zone is worse than a fixed offset: zones carry a Local Mean Time offset for year 1 (+09:18:59 for Tokyo), so a value near MinValue reads back quietly shifted. That matters because the README promotes those two as open-ended range sentinels, which is the pattern from DateTimeOffset not supported #53. Now documented and tested.

Coverage

~30 new tests. Round trips for +05:30, -07:00, +05:45, +00:00 and +00:01; both representability limits; the carried-field case end to end through a real column; and a test asserting TimeZoneInfo genuinely cannot resolve the name, so nobody simplifies the parsing away later.

756 unit / 323 functional, all passing. #64 is rebased on this.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (6)

CHANGELOG.md:7

  • This compatibility recommendation still omits issue #54's write limitation: HasConversion<string>() keeps the old schema, but SaveChanges cannot insert the property because the bulk path skips converters. State that this leaves the property read-only until #54 is fixed, rather than presenting it as a complete migration path.
* **`DateTimeOffset` is now a supported property type**, and maps to `DateTime64(7, 'UTC')`. The provider previously had no mapping for it, so EF Core fell back to `DateTimeOffsetToStringConverter` and made a `String` column with no warning — which broke queries against a real `DateTime64` column with `TYPE_MISMATCH`, and stopped `SaveChanges` from writing the value at all. The store type pins the timezone to `'UTC'` so the instant does not depend on the server's `session_timezone`, and precision 7 is one .NET tick (100 ns), so the round trip is exact. `HasPrecision(n)` is honoured for a property with no `HasColumnType`, and keeps the UTC pin. Note that ClickHouse has no type that stores a UTC offset: `DateTime64` holds an instant, and a declared timezone only decides how that instant is rendered. The instant is kept and the offset is not, so a value read back carries the offset of the column's declared timezone. A non-UTC column such as `DateTime64(6, 'Asia/Tokyo')` reads correctly, as does a fixed-offset column such as `DateTime64(7, 'Fixed/UTC+05:30:00')`, and `DateTimeOffset` composes into `Array`, `Map` and `Tuple` columns. Where an instant cannot be reproduced exactly the read throws rather than returning a value that is quietly wrong: the repeated hour when clocks go back in a zone whose candidate offsets are both non-zero (`Europe/Paris`), and a value before 1900 in a named zone, where the offset is Local Mean Time to the second. Writing a value the store type cannot hold throws for the same reason — ClickHouse wraps it rather than reporting it, which matters for precision 8 and 9, whose ranges are narrower than `DateTimeOffset`. **Behaviour change:** a `DateTimeOffset` property that relied on the old `String` column now resolves to `DateTime64(7, 'UTC')` — add `HasConversion<string>()` to keep the previous shape. See the [DateTimeOffset section of the README](https://github.com/ClickHouse/ClickHouse.EntityFrameworkCore#datetimeoffset) for the daylight-saving and `MinValue`/`MaxValue` limits. ([#53](https://github.com/ClickHouse/ClickHouse.EntityFrameworkCore/issues/53))

src/EFCore.ClickHouse/Storage/Internal/Mapping/ClickHouseDateTimeOffsetTypeMapping.cs:343

  • This range check uses only whole Unix seconds, so it is wrong at the partial seconds at the Int64 boundaries. For precision 9, a value at Unix second 9223372036 with 0.9 seconds of ticks passes this check, although its DateTime64 count exceeds long.MaxValue (the actual maximum fraction is about 0.854775807) and therefore still wraps. The lower boundary is also unnecessarily rejected for part of its second. Compare the exact scaled epoch-tick count, including fractional ticks, against long.MinValue/long.MaxValue.
        var seconds = dateTimeOffset.ToUnixTimeSeconds();
        var (min, max) = RepresentableSecondsRange(Precision);
        if (seconds >= min && seconds <= max)
            return;

src/EFCore.ClickHouse/Update/Internal/ClickHouseModificationCommandBatch.cs:113

  • Validation only runs when the top-level property mapping implements this interface. An Array(DateTime64(9)), Map, or Tuple containing a DateTimeOffset has a composite top-level mapping, so an out-of-range component bypasses this check and can still be written as a wrapped date. Recursively validate composite components (or make composite mappings propagate component validators) before sending the row.
                    if (value is not null
                        && modification.TypeMapping is IClickHouseWriteValidatingTypeMapping validating)
                    {
                        validating.ValidateWriteValue(value, modification.ColumnName);

src/EFCore.ClickHouse/Storage/Internal/Mapping/ClickHouseComponentConversion.cs:124

  • This delegate constant makes every composite that needs component conversion unusable with EF Core precompiled-query generation, as the method's own remarks acknowledge. That turns the new DateTimeOffset/DateOnly composite support into a runtime-only feature. Represent the reader as a quotable/liftable expression (or another precompilation-safe helper) rather than embedding a compiled delegate.
    public static Expression CreateConverter(RelationalTypeMapping mapping, Type componentType)
        => Expression.Constant(
            GetConverter(mapping, componentType),
            typeof(Func<,>).MakeGenericType(typeof(object), componentType));

README.md:165

  • The far-end DateTime range limit still applies to a non-zero fixed offset: the driver must render the instant as a wall clock in that offset, so MaxValue at +05:30 (or MinValue at a negative offset) exceeds DateTime, just as described above for named zones. Limit this statement to the timezone-data, DST, and pre-1900 Local Mean Time restrictions.
None of the limits above applies here: the host needs no timezone data, a fixed offset is never
ambiguous, and it does not change before 1900. Two points of its own do:

RELEASENOTES.md:7

  • This compatibility recommendation still omits issue #54's write limitation: HasConversion<string>() keeps the old schema, but SaveChanges cannot insert the property because the bulk path skips converters. State that this leaves the property read-only until #54 is fixed, rather than presenting it as a complete migration path.
* **`DateTimeOffset` is now a supported property type**, and maps to `DateTime64(7, 'UTC')`. The provider previously had no mapping for it, so EF Core fell back to `DateTimeOffsetToStringConverter` and made a `String` column with no warning — which broke queries against a real `DateTime64` column with `TYPE_MISMATCH`, and stopped `SaveChanges` from writing the value at all. The store type pins the timezone to `'UTC'` so the instant does not depend on the server's `session_timezone`, and precision 7 is one .NET tick (100 ns), so the round trip is exact. `HasPrecision(n)` is honoured for a property with no `HasColumnType`, and keeps the UTC pin. Note that ClickHouse has no type that stores a UTC offset: `DateTime64` holds an instant, and a declared timezone only decides how that instant is rendered. The instant is kept and the offset is not, so a value read back carries the offset of the column's declared timezone. A non-UTC column such as `DateTime64(6, 'Asia/Tokyo')` reads correctly, as does a fixed-offset column such as `DateTime64(7, 'Fixed/UTC+05:30:00')`, and `DateTimeOffset` composes into `Array`, `Map` and `Tuple` columns. Where an instant cannot be reproduced exactly the read throws rather than returning a value that is quietly wrong: the repeated hour when clocks go back in a zone whose candidate offsets are both non-zero (`Europe/Paris`), and a value before 1900 in a named zone, where the offset is Local Mean Time to the second. Writing a value the store type cannot hold throws for the same reason — ClickHouse wraps it rather than reporting it, which matters for precision 8 and 9, whose ranges are narrower than `DateTimeOffset`. **Behaviour change:** a `DateTimeOffset` property that relied on the old `String` column now resolves to `DateTime64(7, 'UTC')` — add `HasConversion<string>()` to keep the previous shape. See the [DateTimeOffset section of the README](https://github.com/ClickHouse/ClickHouse.EntityFrameworkCore#datetimeoffset) for the daylight-saving and `MinValue`/`MaxValue` limits. ([#53](https://github.com/ClickHouse/ClickHouse.EntityFrameworkCore/issues/53))

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DateTimeOffset not supported

2 participants