Map DateTimeOffset to DateTime64 and fix composite component reads (#53) - #63
Draft
alex-clickhouse wants to merge 1 commit into
Draft
Map DateTimeOffset to DateTime64 and fix composite component reads (#53)#63alex-clickhouse wants to merge 1 commit into
alex-clickhouse wants to merge 1 commit into
Conversation
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>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Adds native DateTimeOffset support and repairs composite component materialization.
Changes:
- Maps
DateTimeOffsetto UTC-pinnedDateTime64(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 on lines
+111
to
+113
| // The driver often already produces the target type, for example Array(Int32) -> int[]. | ||
| if (value is TElement[] alreadyTyped) | ||
| return alreadyTyped; |
Comment on lines
+72
to
+73
| if (value is T alreadyTyped) | ||
| return alreadyTyped; |
Comment on lines
+109
to
+111
| If you must keep the offset, store it yourself in a second column alongside a `DateTime`. To keep | ||
| the whole value as text, ask for the conversion explicitly with `HasConversion<string>()` — note | ||
| that `HasColumnType("String")` on its own is not enough, because it adds no converter. |
| * `HasPrecision(n)` is honoured for a property with no `HasColumnType`, and keeps the UTC pin — for example `HasPrecision(3)` gives `DateTime64(3, 'UTC')`. | ||
| * SQL literals carry the offset (`'2026-01-15 10:00:00.1234567+05:00'`), which lands on the same instant whatever timezone the target column declares. | ||
| * No value converter is used, so the driver receives the `DateTimeOffset` directly on both the query parameter path and the bulk insert path. | ||
| * **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 — it resolves the plain string mapping with no converter, so the CLR type no longer agrees with the property. |
Comment on lines
+77
to
+78
| if (value is Dictionary<TKey, TValue> alreadyTyped) | ||
| return alreadyTyped; |
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). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #53.
Problem
The provider had no mapping for
DateTimeOffset. EF Core therefore fell back toDateTimeOffsetToStringConverterthrough the value-converter selector, and made aStringcolumnwithout a warning. Two failures came from this:
DateTime64column failed withTYPE_MISMATCH, because the providerdeclared the parameter as
String.SaveChangescould not write the value at all.Solution
A
DateTimeOffsetproperty now maps toDateTime64(7, 'UTC')through the newClickHouseDateTimeOffsetTypeMapping. No value converter is used, so the driver receives theDateTimeOffsetdirectly 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 asDateTime64(7), the driver sends a UTC wall clock, and the server then reads it insession_timezone. The instant moves when that setting is not UTC. A UTC-pinned store type removesthis 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
DateTimeOffsetrange, soMinValueandMaxValuework as open-ended range limits.
The offset is not kept. ClickHouse has no type that stores a UTC offset.
DateTime64holds aninstant, 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
DateTimeOffsetinside a composite typedoes not work without them, and because they touch the same resolution path in
ClickHouseTypeMappingSource.Composite columns did not convert their components on read.
Array(T),Map(K, V)andTuple(...)read the whole column throughGetValue, so a component mapping's own read pipelinenever ran. Any component whose CLR type differs from the driver's type threw
InvalidCastException. This was not new withDateTimeOffset—DateOnly[],Dictionary<string, DateOnly>andTuple<DateOnly, …>were already affected, becauseDateOnlyalso arrives from the driver as a
DateTime.The new
ClickHouseComponentConversionhelper rebuilds the composite component by component. Itapplies 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 directcast, and a runtime fast path returns the driver's array untouched when it is already the target
type.
Array(Nullable(T))DDL was double-wrapped for a value-type element, which gaveArray(Nullable(Nullable(T))). ClickHouse rejects this withNested type Nullable(T) cannot be inside Nullable type, soEnsureCreatedand migrations both failed.A component mapping ignored the CLR component type. One store type can serve more than one
CLR type:
DateTime64servesDateTimeandDateTimeOffset, andDate32servesDateTimeandDateOnly. 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) andCompositeElementConversionTests(484 lines), 59 testsin total. All run against a real ClickHouse server through Testcontainers, as the guidance in
AGENTS.mdprefers.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, andDateTimeOffsetandDateOnlyinsideArray,Map,Tupleand nested composites.Known limits
ValueConverterstill does not work, because the bulk insertpath 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
enuminside acomposite is written as its raw ordinal. Reading such a column works.
DateTimeOffsetmembers such as.Yeardo not translate to SQL yet (No LINQ translation for DateTime members and methods #55).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 offsetsare 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
DateTimeOffsetproperty that relied on the oldStringcolumn now resolves toDateTime64(7, 'UTC'). AddHasConversion<string>()to keep the previous shape. Note thatHasColumnType("String")on its own is not enough, because it adds no converter.Note for the reviewer
ClickHouseEngineBuilder.cshas two XML doc comments only, which explain the relationship betweenWithOrderByandWithPrimaryKey. This is unrelated to #53. Say the word and I will take it out.🤖 Generated with Claude Code