TCP N5a: Add the read-side mirror of WritableElementTypes to IColumnCodec - #548
Draft
alex-clickhouse wants to merge 1 commit into
Draft
TCP N5a: Add the read-side mirror of WritableElementTypes to IColumnCodec#548alex-clickhouse wants to merge 1 commit into
alex-clickhouse wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds read-side type projections to TCP codecs, preparing POCO reads on top of #462.
Changes:
- Adds
ReadableElementTypesand expression-basedProjectRead. - Adds shared date/time projections and nullable/low-cardinality lifting.
- Adds unit and server integration coverage.
The documented reference-source-to-nullable-value projection case remains broken and requires correction.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
Types/IColumnCodec.cs |
Defines the projection contract. |
Types/ColumnValueProjections.cs |
Centralizes projection logic. |
Types/DateTimeColumn.cs |
Reuses shared conversion. |
Types/DateTime64Column.cs |
Reuses shared scaled conversion. |
Types/TimeColumn.cs |
Reuses shared duration conversion. |
Types/Time64Column.cs |
Reuses shared scaled conversion. |
Codecs/DateTimeColumnCodec.cs |
Advertises and builds date projections. |
Codecs/DateTime64ColumnCodec.cs |
Adds scaled date projections. |
Codecs/TimeColumnCodec.cs |
Adds TimeSpan projection. |
Codecs/Time64ColumnCodec.cs |
Adds scaled TimeSpan projection. |
Codecs/NullableColumnCodec.cs |
Lifts inner readable types. |
Codecs/LowCardinalityColumnCodec.cs |
Propagates inner projections. |
ColumnReadProjectionTests.cs |
Tests contracts and expressions. |
ColumnReadProjectionIntegrationTests.cs |
Validates projections against ClickHouse. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+178
to
+180
| return innerTarget.IsValueType | ||
| ? ColumnValueProjections.ProjectNullable(value, inner, innerTarget, targetType) | ||
| : inner.ProjectRead(value, innerTarget); |
Comment on lines
+382
to
+384
| return nullable && innerTarget.IsValueType | ||
| ? ColumnValueProjections.ProjectNullable(value, inner, innerTarget, targetType) | ||
| : inner.ProjectRead(value, innerTarget); |
alex-clickhouse
force-pushed
the
tcp/epic-n5-poco
branch
from
August 14, 2026 16:44
bb4900b to
2602deb
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
alex-clickhouse
force-pushed
the
tcp/epic-n5-poco
branch
from
August 15, 2026 11:48
2602deb to
4435f57
Compare
alex-clickhouse
force-pushed
the
tcp/epic-n5-poco
branch
from
August 16, 2026 10:31
4435f57 to
9e22d63
Compare
alex-clickhouse
force-pushed
the
tcp/epic-n5-poco
branch
from
August 16, 2026 17:50
9e22d63 to
735a6a8
Compare
alex-clickhouse
force-pushed
the
tcp/epic-n5-poco
branch
from
August 17, 2026 09:49
735a6a8 to
48f9a17
Compare
A codec already declares what it accepts on the write path: WritableElementTypes in preference order, CanWrite as the membership test, and the type switch inside WriteColumn. The read path had no counterpart, because a codec decodes to exactly one canonical type -- and for the date/time family that type is the raw wire count, so `uint` for DateTime and `long` for DateTime64(s). A plain `DateTime` property therefore had no match at all. Add the read-side half as `TryProjectRead(value, targetType, out projected)`. It answers the one type a caller asks about instead of publishing a list the caller must search. That direction matters: on read the target is dictated by the caller, so there is nothing to choose, and a composite can recurse into its children without enumerating their cartesian product -- a seven-field tuple of DateTime64s would be 3^7 entries. It returns an Expression, not a delegate, so a compiled per-column read loop can inline the conversion; a Func<,> would cost an indirect call per row. Scale and timezone are embedded as constants, so the result closes over nothing. `ReadableElementTypes` stays, demoted to diagnostics: it is what tells a caller what a column can be read as when nothing matched. It is documented as not authoritative and not exhaustive, and it is now built per call rather than cached, because it is only ever read on a failure path. The four raw-count codecs override the projection. Nullable and LowCardinality recover the inner codec's spelling by undoing their own wrap on the target rather than by lifting the inner's readable list and searching it. The wrap is invertible -- a value type becomes Nullable<T>, a reference type stays itself, and Nullable<Nullable<T>> does not exist -- so the target alone decides both the inner spelling and whether a lift is needed. That deleted a lazily-built cache in each wrapper, and it removes the need to assume that a codec's readings share their value/reference-ness with its ElementType. Both wrappers now hold only readonly state. The conversions move into a new ColumnValueProjections that DateTimeColumn, DateTime64Column, TimeColumn and Time64Column share instead of each keeping a private copy, so a raw count has one calendar reading whichever surface asks. Enum is deliberately excluded. EnumColumnCodec<T> does not override WritableElementTypes either, so offering a string label on read would be a read-only asymmetry rather than a mirror. A test pins that, so adding it later is a visible choice. Composites do not lift their children, so Array(DateTime) reads only as uint[]. That is faithful: their write contracts do not lift either, so the gap is symmetric. Lifting it needs a write-side projection to match, or the shape stays readable-but-not-insertable. DateTime Kind semantics follow the HTTP driver's AbstractDateTimeType.ToDateTime: a zero offset yields Kind=Utc, any other offset the wall clock in the column's zone as Kind=Unspecified. For a bare DateTime/DateTime64 the two clients deliberately differ -- HTTP presents UTC because its type object carries no zone, whereas this client resolves session_timezone and so agrees with what the server would display. Recorded in the PresentAsDateTime doc comment so it is not later "fixed" toward HTTP. Two stand-in codecs in the tests cover what no registered type reaches: a reference-typed element with a value-typed reading, and one with a second reference-typed reading. Both wrappers expose an internal Over(inner) factory so a test can build them over a stand-in, since the registry cannot produce those shapes. Deciding the lift from the inner codec's canonical type instead of the target now fails those tests. Co-Authored-By: Claude <noreply@anthropic.com>
alex-clickhouse
force-pushed
the
tcp/epic-n5-poco
branch
from
August 17, 2026 13:45
48f9a17 to
a9369fa
Compare
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.
Stacked on #462 (
tcp/epic-n1-client) — review that first; this PR's diff is the last commit only.First step of the Branch 2 POCO epic (N5a/N5/N9), landed on its own because it changes an
internalinterface every codec implements.Why
A codec already declares what it accepts on the write path —
WritableElementTypes(preference-ordered),CanWriteas the membership test,NullPlaceholderAs, and the type switch insideWriteColumn.DateTimeColumnCodeceven already holds theTimeZoneInfoand theToUtc/ToUnixSecondshelpers.The read path had no counterpart, because a codec decodes to exactly one canonical type — and for the date/time family that type is the raw wire count:
IColumn<T>DateTimeuint(epoch seconds)DateTime64(s)long(raw count at scale)Time/Time64(s)int/longSo
public DateTime CreatedAt { get; set; }— the most common POCO member there is — has no match at all. Rather than invent a third home for knowledge the write path already keeps on the codec, this adds the read half.What
TryProjectRead(Expression value, Type targetType, out Expression projected)— the authority on which readings exist. It answers the one type a caller asks about rather than publishing a list the caller must search.That direction is the design decision worth reviewing. On write the codec chooses among whatever CLR type the caller's column happens to hold, so an ordered list earns its keep. On read the target is dictated by the POCO member, so there is nothing to choose — the plan always arrives holding one concrete type and one question. Making the contract interrogative buys three things:
Tuple(DateTime, DateTime64, Time)is 18, a seven-field tuple ofDateTime64s is 3⁷ = 2187 — so no enumeration can express it, and any bounded subset is arbitrary. A predicate recurses into children in linear time. (Not done here; see below.)An
Expression, not aFunc<,>, so the compiled per-column read loop the POCO reader will emit can inline the conversion; a delegate would cost an indirect call per row. Scale and timezone are embedded as constants, so the result closes over nothing.ReadableElementTypesstays, demoted to diagnostics. It is what tells a caller what a column can be read as once nothing matched (Array(DateTime)reads asuint[], so aDateTime[]member is told what does work). It is documented as neither authoritative nor exhaustive, and it is now built per call instead of cached, because it is only ever read on a failure path — which deleted a lazily-built cache, two fields and a race-tolerance comment from each wrapper. Both wrappers now hold onlyreadonlystate.The conversions move into a new
ColumnValueProjectionsthatDateTimeColumn,DateTime64Column,TimeColumnandTime64Columnnow share instead of each keeping a private copy, so a raw count has one calendar reading whichever surface asks for it.The latent bug this removes
Both wrappers used to build a lifted surface→inner type pairing, search it, and then decide whether to lift. They now unwrap the target instead. The wrap is invertible — a value type becomes
Nullable<T>, a reference type stays itself, andNullable<Nullable<T>>does not exist — so the target alone decides both the inner spelling and whether a lift is needed, and the source shape is read from the source's own type. Nothing is inferred from the inner codec's canonical type.That matters because the old discriminator was
innerTarget.IsValueTypeagainst a source typed as the canonical surface. The moment a reference-typed inner offered a value-typed reading —FixedString(16)as aGuidbeing the plausible one —ProjectNullablewould spliceExpression.Property(source, "HasValue")onto abyte[]-typed source and throw at expression-build time. Unreachable today, and the previous revision of this PR shipped a guard test that could not fail.It can now. Two stand-in codecs in the tests supply the shapes the registry cannot produce — a reference-typed element with a value-typed reading, and one with a second reference-typed reading — reached through a new
internal Over(inner)factory on each wrapper. Deciding the lift from the inner codec's canonical type fails 2 of them; hardcoding the source unwrap fails 3.Deliberately excluded
Enum.
EnumColumnCodec<T>does not overrideWritableElementTypeseither — write takes only the raw ordinal — so offering astringlabel on read would be a read-only asymmetry rather than a mirror. Pinned by a test so adding it later is a visible choice.Composites.
Array/Map/Tuple/Nesteddo not lift their children, soArray(DateTime)reads only asuint[]. This is faithful: their write contracts do not lift either (ArrayColumnCodec.CanWriteiscolumn is IColumn<TElement[]>over the child's canonicalElementType), so the gap is symmetric today. Lifting read alone would give "I can readDateTime[]out but cannot insert it back", so it waits for a write-side projection to match. The interrogative contract is what makes it possible; it is not attempted here.DateTimeKind semanticsPresentAsDateTimefollows the HTTP driver'sAbstractDateTimeType.ToDateTime: a zero offset yieldsKind=Utc, any other offset the wall clock in the column's zone asKind=Unspecified.For a bare
DateTime/DateTime64the two clients deliberately differ: HTTP presents it in UTC because its type object carries no zone, whereas this client resolvessession_timezoneand so agrees with what the server itself would display. That was weighed and chosen — keeping the whole TCP read path honoringsession_timezonebeat cross-client parity, at the cost of a POCO moved from the HTTP client shifting by the session offset on bare columns. Columns whose type names a timezone match HTTP exactly. Recorded in thePresentAsDateTimedoc comment so it is not later "fixed" toward HTTP.Adjacent and pre-existing: reads use the session timezone while writes use
ResolveContext.ForWrite, which carries none, soToUtcreads anUnspecifiedDateTimeas a UTC wall clock. A round trip through a bare column on a non-UTC session is therefore not the identity.Tests
Nothing consumes this yet, so the tests carry the PR.
1700000000=2023-11-14T22:13:20Z, Berlin+01:00in November and+02:00for a July instant (a fixed-base-offset implementation passes the first and fails the second), scale-9…123456789truncating to.1234567,Time64(9)-1000000001→-00:00:01showing truncation toward zero.ElementType, lists no duplicate, and can project each type it advertises. Deliberately one-directional — the converse stops being assertable once a composite answers for shapes it does not enumerate.LowCardinality(Nullable(DateTime)).Coverage: every line added is covered.
IColumnCodecandDateTime64ColumnCodecat 100%,DateTimeColumnCodec99.0%,LowCardinalityColumnCodec96.2%,ColumnValueProjections95.3%,NullableColumnCodec93.3%; the remaining uncovered lines in those files are all pre-existing. Full TCP suite green against a real server.🤖 Generated with Claude Code