TCP N10: lift composite children's element types on read and write - #564
Draft
alex-clickhouse wants to merge 8 commits into
Draft
TCP N10: lift composite children's element types on read and write#564alex-clickhouse wants to merge 8 commits into
alex-clickhouse wants to merge 8 commits into
Conversation
alex-clickhouse
marked this pull request as draft
August 17, 2026 17:28
alex-clickhouse
force-pushed
the
tcp/epic-n10-composite-lift
branch
from
August 17, 2026 17:40
d463021 to
cd10534
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Extends TCP codecs so composite child types lift recursively across read and write paths.
Changes:
- Adds interrogative write-type acceptance and column element-type discovery.
- Implements recursive lifting for arrays, maps, tuples, and low-cardinality columns.
- Adds broad codec, POCO, equivalence, and integration coverage.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
ClickHouse.Driver.Tcp/Types/IColumnCodec.cs |
Adds write-type interrogation. |
ClickHouse.Driver.Tcp/Types/IColumn.cs |
Exposes column element types. |
ClickHouse.Driver.Tcp/Types/CompositeElementProjections.cs |
Builds composite read projections. |
ClickHouse.Driver.Tcp/Types/Codecs/VariantColumnCodec.cs |
Preserves variant write gating. |
ClickHouse.Driver.Tcp/Types/Codecs/ValueNullableShape.cs |
Replaces value-type probes. |
ClickHouse.Driver.Tcp/Types/Codecs/TupleColumnCodec.cs |
Lifts tuple fields recursively. |
ClickHouse.Driver.Tcp/Types/Codecs/ReferenceNullableShape.cs |
Replaces reference-type probes. |
ClickHouse.Driver.Tcp/Types/Codecs/NullableLowCardinalityShape.cs |
Uses interrogative inner acceptance. |
ClickHouse.Driver.Tcp/Types/Codecs/NullableColumnCodec.cs |
Gates nullable write types. |
ClickHouse.Driver.Tcp/Types/Codecs/NothingColumnCodec.cs |
Rejects all write types. |
ClickHouse.Driver.Tcp/Types/Codecs/NestedColumnCodec.cs |
Rejects row-shaped writes. |
ClickHouse.Driver.Tcp/Types/Codecs/MapColumnCodec.cs |
Lifts map keys and values. |
ClickHouse.Driver.Tcp/Types/Codecs/LowCardinalityShape.cs |
Replaces inner write probes. |
ClickHouse.Driver.Tcp/Types/Codecs/LowCardinalityColumnCodec.cs |
Adds lifted write shapes. |
ClickHouse.Driver.Tcp/Types/Codecs/IArrayWriteShape.cs |
Adds lazy array write shapes. |
ClickHouse.Driver.Tcp/Types/Codecs/ArrayColumnCodec.cs |
Lifts array elements. |
ClickHouse.Driver.Tcp/Poco/PocoWriteConversion.cs |
Uses codec write interrogation. |
ClickHouse.Driver.Tcp/Poco/PocoColumnBuilder.cs |
Updates composite diagnostics. |
ClickHouse.Driver.Tcp.Tests/Utilities/InsertRoundTripCase.cs |
Documents canonical array cases. |
ClickHouse.Driver.Tcp.Tests/Types/WritePathEquivalenceTests.cs |
Verifies lifted wire equivalence. |
ClickHouse.Driver.Tcp.Tests/Types/TupleColumnCodecTests.cs |
Covers tuple rejection paths. |
ClickHouse.Driver.Tcp.Tests/Types/CompositeLiftMatrixTests.cs |
Sweeps nested lift combinations. |
ClickHouse.Driver.Tcp.Tests/Types/CompositeElementProjectionTests.cs |
Tests projection behavior. |
ClickHouse.Driver.Tcp.Tests/Types/ColumnWriteAcceptanceTests.cs |
Tests write-type acceptance. |
ClickHouse.Driver.Tcp.Tests/Types/ColumnElementTypeTests.cs |
Tests element-type resolution. |
ClickHouse.Driver.Tcp.Tests/Types/ArrayColumnCodecTests.cs |
Covers lifted null-row errors. |
ClickHouse.Driver.Tcp.Tests/Poco/PocoReadPlanTests.cs |
Tests lifted POCO reads. |
ClickHouse.Driver.Tcp.Tests/Integration/PocoWriteIntegrationTests.cs |
Adds server round trips. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
alex-clickhouse
force-pushed
the
tcp/epic-n10-composite-lift
branch
2 times, most recently
from
August 18, 2026 07:03
eef7be6 to
c8b2167
Compare
alex-clickhouse
changed the base branch from
tcp/epic-n9-poco-write
to
tcp/epic-b9-tls
August 18, 2026 07:03
alex-clickhouse
force-pushed
the
tcp/epic-n10-composite-lift
branch
from
August 18, 2026 07:29
c8b2167 to
7ce2650
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-n10-composite-lift
branch
from
August 18, 2026 08:11
7ce2650 to
eb7ddf6
Compare
alex-clickhouse
force-pushed
the
tcp/epic-n10-composite-lift
branch
from
August 21, 2026 13:07
eb7ddf6 to
7aead06
Compare
alex-clickhouse
force-pushed
the
tcp/epic-n10-composite-lift
branch
from
August 22, 2026 16:47
7aead06 to
216408d
Compare
alex-clickhouse
force-pushed
the
tcp/epic-n10-composite-lift
branch
from
August 22, 2026 17:06
216408d to
007d917
Compare
alex-clickhouse
force-pushed
the
tcp/epic-n10-composite-lift
branch
from
August 22, 2026 17:25
007d917 to
4e8c058
Compare
alex-clickhouse
force-pushed
the
tcp/epic-n10-composite-lift
branch
3 times, most recently
from
August 26, 2026 15:40
995f3f4 to
86aeff2
Compare
alex-clickhouse
force-pushed
the
tcp/epic-n10-composite-lift
branch
from
August 26, 2026 16:38
86aeff2 to
8d2e631
Compare
alex-clickhouse
force-pushed
the
tcp/epic-n10-composite-lift
branch
from
August 26, 2026 18:59
8d2e631 to
39409bd
Compare
alex-clickhouse
force-pushed
the
tcp/epic-n10-composite-lift
branch
from
August 28, 2026 11:03
39409bd to
67f8c87
Compare
alex-clickhouse
force-pushed
the
tcp/epic-n10-composite-lift
branch
from
August 28, 2026 12:18
67f8c87 to
9c377d1
Compare
PR 548 made the read contract interrogative so a container could recurse into its children without enumerating their cartesian product. Only the two wrappers used it. Array, Map and Tuple kept the identity-only default, so Array(DateTime) read as uint[] and nothing else, and Tuple(DateTime, String) only as ValueTuple<uint, string>. Each container now overrides TryProjectRead and asks its children. The structural half is shared: a map row is an array of pairs, so both reduce to one element-wise loop in CompositeElementProjections, with the caller's element projection building the new pair. A tuple rebuilds field-wise and needs no loop. The loop binds the row expression to a local, so a projection evaluates the expression it was handed once however many elements the row has. Containers recurse through containers, so Array(Array(DateTime)) and a tuple with an array field both lift. Nested, Variant and Dynamic cannot and never will: their element type is a fixed object[][] or object, so no per-child CLR type survives to the surface to lift into. ReadableElementTypes stays canonical-only. The honest list for a container is its children's cartesian product, which costs a materialized Type per combination on a failure path; TryProjectRead is the authority and answers targets the list omits. One plan test pinned the removed limitation. It becomes two: the lifted property now fills, and a reading no child offers still reports what the column does read as. Co-Authored-By: Claude <noreply@anthropic.com>
Seven places wanted to know whether a codec accepts a given CLR element type, and all seven asked by building a throwaway one-element column and calling CanWrite on it. PocoWriteConversion went further and built its probe reflectively, through Activator.CreateInstance over a MakeGenericType. CanWriteElementType(Type) is that question asked directly. It defaults to membership of WritableElementTypes, so every leaf keeps its answer, and the four nullable/low-cardinality shapes, Map's shape, Array, Tuple and Variant all drop their probe columns. Two codecs have to override it: Nothing has no values to encode, and Nested is written only from its own wire-shaped NestedColumn. Neither is describable as an element type, which is what keeps a row-oriented insert reporting that no property type can fill such a column. The member is interrogative rather than a longer list for the same reason TryProjectRead is: a composite will answer by asking its children about the matching part of the type, so it never enumerates the cartesian product of what they each accept. No behavior change; this is the contract the composite write lifting needs. Co-Authored-By: Claude <noreply@anthropic.com>
Reads lifted, writes did not: a DateTime[] property could be read from an Array(DateTime) column but not inserted into one, and PocoWriteConversion's doc recorded a LowCardinality(DateTime) column reading into a DateTime property while accepting only raw epoch seconds. Each container now answers CanWriteElementType by asking its children about the matching part of the type, and resolves its write shape from the column's own element type rather than its canonical one. So an Array(DateTime) column takes an IColumn<DateTime[]>, flattens it into a ConcatColumn<DateTime>, and the inner codec converts as it writes -- through the case IColumn<DateTime> that already existed. Nothing is copied and nothing is allocated per row: the flattening view is lazy, as it already was for the canonical type. Most of the generic machinery was already in the right shape. MapShape's write members were parameterized on the key and value types and took the codecs as arguments, so lifting Map was resolving a different shape. TupleFieldColumn<T> is generic over the field type and reads through ITuple, so lifting Tuple was closing its per-field projection builders over the source tuple's field types. Only Array needed the ergonomic branch extracted, into ArrayWriteShape<TWrite>; its dense branch stays on the codec's own type argument, a dense column being canonical by definition. Shapes resolve lazily and cache per element type, following MapShapes.For and LowCardinalityShapes.For. That is what keeps the accepted set unmaterialized: a seven-field tuple of DateTime64 accepts 3^7 element types and builds a shape only for those actually written. CanWrite has to interrogate a column whose element type it does not know statically, so IColumn gains ElementType: a default interface member resolving the T of the implemented IColumn<T>, cached per column type. Enumerating shapes instead would not compose, because a container child reports only its canonical type. The consumer keeps its existing walk first -- those types a caller already holds in the shape the writer wants -- and falls back to asking the codec, in which case the write type is the property's own type and no conversion is emitted. Write stays inside read, which is what keeps a POCO round-tripping. A column with null rows refuses a bare value type, so Array(Nullable(Int32)) is not writable from int[]: it would insert and then fail to select back into the same property. A sweep over a candidate pool asserts that invariant for every shape in the matrix. Co-Authored-By: Claude <noreply@anthropic.com>
Three reachable failures had no test. The Array null-row message moved into ArrayWriteShape when the ergonomic branch was extracted, and nothing asserted it -- it is the message that names the column, the row and the two ways out, so it is worth pinning, and pinning for a lifted element type too, since the offsets are now computed by a shape resolved from the row's own type. The other two are contract guards with no other way to observe them: the element variable ProjectArray is handed must match the row's elements, and a column class surfacing two element types has no single one and is refused rather than resolved to whichever interface reflection listed first. IColumn.ElementType is read through the interface in these tests, which is how every codec reaches it: a default interface member is not visible on the implementing class without a cast. Co-Authored-By: Claude <noreply@anthropic.com>
Coverage found the lifted LowCardinality write path untested: the acceptance tests only ask CanWriteElementType, which never reaches the shape resolution, so the headline case -- a LowCardinality(DateTime) column written from a DateTime property -- had no test at all. It now round-trips through a real server, with uniqExact asserting the dictionary really deduplicated and the raw epoch seconds asserted on the wire, so a wrong dictionary shows up as a wrong value rather than merely a different encoding. WritePathEquivalenceTests gains seven lifted cases. Its property is the strongest available here: the dense read-back of a column is always in the canonical CLR type, so byte equality between it and a lifted ergonomic write is what proves lifting changes the CLR surface and nothing else. One case lifts through two levels over an inner that has a state prefix of its own -- Array(LowCardinality(DateTime)) -- where the Array resolves a shape for DateTime and the LowCardinality it hands the flattened view resolves one too. These also cover the state-free WriteStatePrefix and WriteColumn overloads with a lifted shape, which nothing reached before. Two Tuple guards behind CanWrite: a dense column of a different arity, refused on its own CLR tuple type before any child is consulted, and a flat tuple column of field types no child accepts. The remaining uncovered lines in the changed files are an Array.MaxLength overflow guard, which needs more elements than one array can hold, and pre-existing read paths. Co-Authored-By: Claude <noreply@anthropic.com>
Nullable and Variant refuse a column when their child cannot be written at all -- innerCanWrite, allChildrenWritable -- but neither overrode CanWriteElementType, so the interface default answered from WritableElementTypes and skipped that gate. Since every container now asks the interrogative question, the gate was bypassed transitively, breaking the contract stated one member above: the two must agree wherever both can answer. This was a regression, not a missing feature. The server accepts Map(String, Nullable(Nothing)) and Array(Variant(String, Nested(...))) as real table columns, and the insert gate used to refuse them before any byte went out. With the gate bypassed the write faults part-way through a block, leaving a half-written INSERT on the wire, and the POCO path lost its accurate plan-build message for a generic one. Both overrides now apply the same condition their CanWrite applies. Eleven cases cover the wrappings the previous test missed -- a bare Nothing and a bare Nested were covered, Nullable(Nothing) and Variant(..., Nested(...)) were not -- and a new test asserts the contract itself rather than case by case, so the next codec to gate on extra state is caught. The default also no longer builds a Type[] to answer the canonical type, which is the common answer and is asked once per column per slice. Four stale doc comments: a cref to a member that never shipped, the "LowCardinality is asymmetric today" paragraph -- which the revert restored, and which prescribed a remedy this change did not need -- and two references to probe columns that no longer exist. Co-Authored-By: Claude <noreply@anthropic.com>
TryGetArrayElement tested IsArray plus rank one, which also admits the non-zero-based T[*] that Type.MakeArrayType(1) builds: rank one, right element type, distinct type. Both directions were wrong for it, and Map inherited the fault through the same helper. On a read the projection builds its result with MakeArrayType(), which is always zero-based, so the codec returned true with an expression whose type was not the one asked for -- the one thing the contract promises it will not do. On a write, acceptance returned true and the failure moved to ArrayWriteShape<T> casting the column to IColumn<T[]>, turning a plan-build refusal into a cast failure with the insert already open. IsSZArray is exactly the predicate: single dimension and zero-based. One helper, so the fix covers Array's read projection, Array's write acceptance and Map's pair test at once. Four tests, one per direction per container; all four fail without the fix. Co-Authored-By: Claude <noreply@anthropic.com>
alex-clickhouse
force-pushed
the
tcp/epic-n10-composite-lift
branch
from
August 30, 2026 09:07
b43646b to
1de32e9
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 #563 (
tcp/epic-b9-tls), the current tip of the TCP stack.Summary
This PR lets TCP composite codecs expose and accept the CLR types supported by their children. The composition works recursively through
Array,Map,Tuple,Nullable, andLowCardinalitywithout enumerating every possible combination.Examples:
Array(DateTime('UTC'))can read and writeDateTime[]instead of requiring rawuint[]epoch seconds.Map(String, Array(DateTime('UTC')))can map toKeyValuePair<string, DateTime[]>[].Array(Array(Tuple(DateTime('UTC'), String)))can map to(DateTime, string)[][].Nullable(Tuple(DateTime('UTC'), String))can write(DateTime, string)?when the server feature is enabled.LowCardinality(DateTime('UTC'))can be written from an ergonomicArrayColumn<DateTime>while building its dictionary from encoded epoch seconds.The review also found and fixed two write-path correctness issues:
Nullablenow asks its inner codec whether a CLR type is writable instead of searching a finite diagnostic list.LowCardinalitynow deduplicates values after converting them to the representation its inner codec writes. The conversion and serialization paths share the same code; there is no separate wire comparer.Nested,Variant, andDynamickeep their existing fixedobject-based surfaces. Their child CLR types do not survive at the outer surface, so there is no type shape to lift.How the contracts fit together
ElementTypeReadableElementTypesTryProjectReadWritableElementTypesCanWriteElementTypeCanWriteNullPlaceholderAsBeginWriteCanonicalWriteElementTypeToCanonicalWriteColumnWriteCanonicalColumnThe enumerated type lists are intentionally not authoritative. Enumerating a seven-field tuple whose children each accept three CLR types would require materializing 3^7 constructed tuple types. The interrogative contracts only inspect the exact shape a caller requests.
Read path
ElementTypefrom the Native stream.TryProjectReadfor the target property type.For
Array(DateTime('UTC')), the canonical value isuint[]. When the target isDateTime[],Arrayrecognizes the array shape and asks the DateTime codec to project eachuinttoDateTime.Write path
CanWriteElementTypewhether the property type is accepted.BeginWriteprepares flattened or projected child columns and child state once.Nullable composites
The Native encoding of
Nullable(T)contains a null map followed by an encodedTvalue for every row, including null rows. The hidden value has no semantic meaning, but the inner codec must still be able to serialize it.For a null
(DateTime, string)?row:Nullablewrites1to the null map.Tuplefor a writable(DateTime, string)placeholder.Tupleasks its DateTime and String children for valid placeholders and constructs one tuple.Nullable(Tuple(...))is gated to ClickHouse 26.6 or newer. Its real-server test enablesenable_nullable_tuple_type=1only for that query and is skipped on older servers.Canonical LowCardinality writes
An ergonomic LowCardinality write now has one source of truth for conversion and equality:
ToCanonicalWriteColumn.WriteCanonicalColumnwrites the dictionary without repeating the conversion.For
LowCardinality(DateTime('America/New_York'))written fromArrayColumn<DateTime>:DateTimevalues can have equal CLR ticks but differentKindvalues. One may mean a UTC instant while the other is a New York wall clock, so they convert to differentuintepoch seconds and remain separate dictionary entries.uintand share one dictionary entry.The same contract covers:
DateTime64,Time, andTime64countsFloat32andFloat64bit patterns, including+0versus-0BFloat16narrowingFixedStringbyte contentOrdinary non-LowCardinality writes retain their direct loops and bulk-copy paths. They call the same per-value conversion routines but do not allocate a projected view. Non-nullable LowCardinality also keeps a dedicated tight loop with no per-row nullable check.
Performance
Ad-hoc BenchmarkDotNet comparison on .NET 9 against the original PR snapshot (
9c377d11), using 10,000 rows, 100 distinct LowCardinality values, 3 warmups, and 8 measured iterations:LowCardinality(String)LowCardinality(DateTime)Nullable(Int32)LowCardinality(DateTime)now converts every source row before dictionary lookup; the original path converted only distinct CLR values and was incorrect when CLR equality disagreed with the encoding. Canonicaluintdictionary entries reduce allocation.Nullable(Int32)adds one small write-state object per block so prefix and body phases can share prepared child state.The benchmark harness was diagnostic and is not included in the PR.
Validation
git diff --checkpasses.CHANGELOG.mdandRELEASENOTES.mdare unchanged; this is TCP-client work.Checklist