Repository navigation
TCP converters 2b/6: columnar reads through the converter layer - #668
alex-clickhouse wants to merge 8 commits into
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🟡 Changes recommended
Array reinterpretation can silently change numeric values, and failed conversions can mask index-out-of-range errors.
2 open findings
What changed in this PR
Moves native TCP columnar reads onto the converter layer while retaining the legacy path for POCO mapping and differential tests.
Changes:
- Routes
Block.ReadAs<T>andCanReadthrough converter derivation. - Adds lazy, pooled, block-owned converted views.
- Expands tests, documentation, and changelog coverage.
| File | Description |
|---|---|
docs/tcp.mdx |
Documents conversion behavior and read rules. |
ClickHouse.Driver.Tcp/Types/LegacyColumnProjection.cs |
Preserves the legacy projection path. |
ClickHouse.Driver.Tcp/Types/DerivedColumn.cs |
Adds lazy pooled conversion views. |
ClickHouse.Driver.Tcp/Types/Converters/LeafReaders.cs |
Preserves String byte-reading diagnostics. |
ClickHouse.Driver.Tcp/Types/Converters/ConverterDerivation.cs |
Uses the registry-owned derivation. |
ClickHouse.Driver.Tcp/Types/ColumnReadProjections.cs |
Routes reads through converters and retained views. |
ClickHouse.Driver.Tcp/Types/ColumnProjection.cs |
Creates derived views and manages their lifetime. |
ClickHouse.Driver.Tcp/Types/ColumnCodecRegistry.cs |
Owns the converter derivation. |
ClickHouse.Driver.Tcp/Types/Codecs/TupleColumnCodec.cs |
Retains legacy tuple projections. |
ClickHouse.Driver.Tcp/Types/Codecs/StringColumnCodec.cs |
Extracts the missing-wire-bytes error. |
ClickHouse.Driver.Tcp/Types/Codecs/NullableColumnCodec.cs |
Retains legacy nullable projections. |
ClickHouse.Driver.Tcp/Types/Codecs/MapColumnCodec.cs |
Retains legacy map projections. |
ClickHouse.Driver.Tcp/Types/Codecs/LowCardinalityColumnCodec.cs |
Retains legacy dictionary projections. |
ClickHouse.Driver.Tcp/Types/Codecs/ArrayColumnCodec.cs |
Retains legacy array projections. |
ClickHouse.Driver.Tcp/Types/ClickHouseTcpTypes.cs |
Moves CanRead onto converter derivation. |
ClickHouse.Driver.Tcp/Format/Block.cs |
Owns and releases converted views. |
ClickHouse.Driver.Tcp.Tests/Types/StringColumnCodecTests.cs |
Updates String failure timing and message. |
ClickHouse.Driver.Tcp.Tests/Types/DerivedColumnTests.cs |
Covers conversion, pooling, disposal, and concurrency. |
ClickHouse.Driver.Tcp.Tests/Types/Converters/ReadConverterRegistration.cs |
Registers client read paths in differential tests. |
ClickHouse.Driver.Tcp.Tests/Types/Converters/ReadCombinatorTests.cs |
Extends tuple failure-order coverage. |
ClickHouse.Driver.Tcp.Tests/Types/Converters/LeafTableTests.cs |
Compares leaves with the legacy projection. |
ClickHouse.Driver.Tcp.Tests/Types/Converters/LeafReaderTests.cs |
Updates legacy-reference documentation. |
ClickHouse.Driver.Tcp.Tests/Types/Converters/ConverterHarness.cs |
Uses copied legacy reads as reference. |
ClickHouse.Driver.Tcp.Tests/Types/ColumnReadProjectionTests.cs |
Updates legacy projection assertions. |
ClickHouse.Driver.Tcp.Tests/Differential/LegacyColumnarRead.cs |
Copies the previous read dispatch for comparison. |
ClickHouse.Driver.Tcp.Tests/Differential/DifferentialSelfTests.cs |
Updates reference-arm expectations. |
ClickHouse.Driver.Tcp.Tests/Differential/ClientArms.cs |
Separates legacy and current client arms. |
changelog.d/converter-2b-columnar-reads.improvements.md |
Records the user-visible behavior change. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ? projection | ||
| : Elementwise(codec, targetType); | ||
| } | ||
| Derivation derived = derivation.Derive(column.TypeName, in context, typeof(T), ConversionDirection.Read); |
There was a problem hiding this comment.
This part accepts these casts on purpose. They are the read rules of POCO mapping on main (PocoValueProjection.TryResolve accepts int[] from Array(UInt32)), and this stack gives every read tier the same rules. #678 refuses, in every tier, the array casts that change the sign or the width of the elements, and keeps the casts between an enum array and an array of its exact underlying type. The description of this PR has a note on this.
| public ReadOnlySpan<T> Values => Converted().AsSpan(0, source.RowCount); | ||
|
|
||
| /// <inheritdoc/> | ||
| public T this[int row] => Values[row]; |
There was a problem hiding this comment.
Confirmed and fixed in a2d03d2. The indexer checks the row against RowCount before it converts the column, so a row outside the column throws IndexOutOfRangeException and does not convert the column. A regression test reads rows -1 and 3 of a Nullable(Int32) view with a NULL at row 1.
|
This TCP-only PR routes Final verdict✅ Approve Automated review by Polyglot AI (gpt-6-astra) of |
The columnar read tier moves onto the converter derivation (ClickHouse/integrations#801): - Block.ReadAs<T> returns the column itself when it already is an IColumn<T>. Otherwise it returns a DerivedColumn<T>: on the first access to Values or the indexer, it converts the whole column once, with one bulk read of the derived reader, into a pooled array; both members read that array. The reader is bound at the call, so a column without the decoded shape fails there. - The block keeps one view for each (column, CLR type) (DerivedViews), so a second call gives the same view and the same values. The first access converts under a lock, so threads that access a view together convert it once. Dispose gives every array back to the pool; a view accessed after that throws ObjectDisposedException. - The read rules apply: ReadAs<MyEnum> over an integer column, ReadAs<object>, and ReadAs<int> over Nullable(Int32), which fails at the first NULL with "Column 'x' (Nullable(Int32)) has NULL at row R, and the target type System.Int32 cannot hold NULL. ..." - ClickHouseTcpTypes.CanRead is "the derivation succeeds", so it agrees with ReadAs. - The registry owns one derivation (ColumnCodecRegistry.Converters); ConverterDerivation.Default is the default registry's. The old projection moves unchanged to LegacyColumnProjection. The old codec members (TryProjectColumnRead of Nullable, Array, LowCardinality, Map and Tuple) keep calling it, so the POCO tier reads as before until it moves onto the derivation. Tests: the reference arms of the ReadAs and CanRead tiers run the old dispatch, copied into the test project (LegacyColumnarRead), and the client's ReadAs and CanRead run every facet of their tiers as candidates. Changed on purpose: four DifferentialSelfTests name the reference "Old path: ..." and negate the reference's answer, and StringColumnCodecTests.ReadAs_StringColumnWithoutByteStorage_* expects the failure at the ReadAs call, where the reader is bound. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DerivedColumnTests (ClickHouse/integrations#801): - The view converts the whole column once, on the first access to Values or the indexer, into one rented array, and gives only the rows of the column; eight threads at the first access fill it once. - Release gives the array back to the pool; an access after Release, or after the block is disposed, throws ObjectDisposedException; a failed conversion gives its array back and fails again on the next access. - A NULL read as a value type throws the message that names the column, its type and the row; an index outside the column throws IndexOutOfRangeException; zero rows rent nothing. - A block gives one view for each (column, type), also to eight threads at once, and the column itself when it already reads as the type; Dispose releases every view. - ReadAs and CanRead follow the read rules: an enum from its ordinal, object, a nullable target, a nullable column as a value type, the array cast that reads elements as another type. DerivedColumn takes its pool in an internal constructor, so the tests can count the rented and returned arrays. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… answers docs/tcp.mdx gets a section on Block.ReadAs<T> (ClickHouse/integrations#801): the view converts the whole column on its first access, the block keeps one view for each column and type and releases it when the enumeration advances, and the read rules of POCO mapping apply (enum from ordinal, cast, nullable target, nullable column as a value type that throws at the first NULL). The type mapping and CanRead paragraphs refer to it. Changelog fragment, named by the branch until the pull request has a number. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Block.ReadAs<T> made the block's set of views (two small objects) before it tested whether the column already reads as T, so a read of the column's own type allocated once for each block. The test now comes first (ClickHouse/integrations#801). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sts and the harness The summaries of ConverterHarness and LeafReaderTests named ColumnReadProjections.ReadAs as the old read; it reads through the converter derivation now, and the old read is LegacyColumnarRead (ClickHouse/integrations#801). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The two-failure test reads a Tuple of 1 and of 3 to 7 Time fields whose row 0 fails on its last field and row 1 on its first field, through Fill and Emit, against the old read: every arity reads the range again row by row after its bulk read fails (ClickHouse/integrations#801). The coverage run showed those passes untested for every arity but 2. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review round 1 of the columnar reads (ClickHouse/integrations#801): - F2: a byte[] reading of a String column that does not expose IStringColumn (a column that a caller built with the type name) failed with the generic surface message of the converters. It fails again with the message of StringColumnCodec.RowBytes, which says that only a column decoded from a server response has the bytes; the text is now in one helper (StringColumnCodec.NoWireBytes) for the codec and the String leaf, in Bind and in Emit. The reader is still bound at the ReadAs call, so the failure comes there. StringColumnCodecTests.ReadAs_StringColumnWithoutByteStorage_* asserts the whole message again. - F3: the changelog fragment is in the past tense, as changelog.d says. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The GitHub review of the columnar reads (ClickHouse/integrations#801): the indexer of DerivedColumn<T> converted the whole column before it checked the row. A row outside the column of a view that fails to convert (for example a NULL read as a value type) threw the conversion failure, not IndexOutOfRangeException as the other IColumn<T> indexers do. The indexer checks the row against RowCount first, and a row outside the column does not convert the column. DerivedColumnTests.Indexer_RowOutsideAColumnThatFailsToConvert_* reads rows -1 and 3 of a Nullable(Int32) view with a NULL at row 1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
b5c3e3d to
a2d03d2
Compare


This is part 2b of the stack of pull requests for ClickHouse/integrations#801, a converter layer for the native TCP
client. It is stacked on #667 (part 2a, the read combinators). It moves the columnar read tier,
Block.ReadAs<T>, andClickHouseTcpTypes.CanReadonto the converter derivation. POCO mapping (QueryAsync<T>) does not change in thispart; part 3 moves it.
What changes for a user
Block.ReadAs<T>andClickHouseTcpTypes.CanReadaccept the same readings asQueryAsync<T>POCO mapping(one set of read rules for every tier): a CLR enum from its integer ordinal, a type that the values cast
to (for example
object), a nullable value type for a column that is not nullable (for exampleulong?forUInt64), and a value type for a nullable column. The last one throwsInvalidOperationExceptionat the firstNULL: "Column 'x' (Nullable(Int32)) has NULL at row 3, and the target type System.Int32 cannot hold NULL. Read thecolumn as a nullable type, or remove the NULL values in the query."
CanReadnow gives the answer ofReadAs.UInt64asulong?,Dateas
DateOnly?,TimeandTime64(3)asTimeSpan?,UUIDasGuid?,DateTime('UTC')asDateTime?and asDateTimeOffset?,DateTime64(3, 'UTC')asDateTime?(values);Nullable(Time64(3))asTimeSpan,Nullable(DateTime('UTC'))asDateTimeand asuint,LowCardinality(Nullable(DateTime('UTC')))asDateTimeand as
DateTimeOffset(a failure at the first NULL). The oldReadAsandCanReadrefused all 13; POCO mappingaccepted them.
T,ReadAs<T>returns the column itself, as before. Otherwise it returns a viewthat converts the whole column one time, with one bulk read, on the first access to
Valuesor to the indexer.Both read the same converted values, which are in a pooled array. The block keeps one view for each
column and type, so a second call returns the same view, and it gives the arrays back to the pool when it is
disposed. A view that is read after its block is disposed throws
ObjectDisposedException.Resolved in #678: the stack now refuses, in every tier and in both directions, an array cast that gives the elements another meaning (an integer type of the other sign, an enum over another integer type). It keeps an enum array from an array of its exact underlying type, so
Array(Enum8(...))still reads into aMyEnum[].Open question for the maintainers. The rules copy POCO mapping exactly, and POCO mapping accepts every target that
Type.IsAssignableFromaccepts. The CLR accepts a cast between arrays of integers of the same size and of enums andtheir integer type, so
Array(UInt32)reads asint[]: the array stays the same, and a value aboveint.MaxValuereads as a negative
int.ReadAsandCanReadnow accept these casts too. The rule is in one place(
ReadRules.IsAssignable, withReadRules.ReinterpretsElementsnaming the casts that read elements as another type,and a test that lists all 48 of them). Refusing them in every tier would be a change in that one place.
How
ColumnProjection.Forderives the reader for the column's type (with the read rules), binds it to the column, andreturns a
DerivedColumn<T>. The reader is bound at theReadAscall, so a column without the decoded shape failsthere; the conversion runs on the first access.
DerivedColumn<T>converts under a lock, so threads that access a view for the first time together convert it once.A failed conversion gives its array back and fails again on the next access. A NULL that a read rule meets becomes the
message above, which names the column, its type and the row. Other conversion failures keep their texts.
ReadAs, and releases every view whenit is disposed. A column that already reads as
Tdoes not make the set.LegacyColumnProjection. The composite codecs' old column projectionscall it for their children, and POCO mapping reads through those, so POCO mapping reads exactly as before. The last
part of the stack deletes it with those codec members.
Why
Before this change,
ReadAs<T>converted one row at each indexer access through a compiled delegate, andCanReadanswered a narrower question than POCO mapping. One bulk read is faster, and one rule set makes
ReadAs<T>,CanReadand
QueryAsync<T>agree.Tests
ReadAsandCanReadfor every facet of their tiers, against theold dispatch of the tier, which this part copies into the test project as the reference. The reference outcome counts
do not change. The 13 readings above are declared changes (part 2a).
DerivedColumnTests(22): one conversion of the whole column into one rented array at the first access, also witheight threads at once; the array back to the pool on release, and
ObjectDisposedExceptionafter it; a failedconversion; the NULL message; an index outside the column; zero rows; one view for each column and type of a block,
also with eight threads; the column itself for its own type;
Disposereleases every view; the newReadAsandCanReadanswers.FillandEmit, against the old read.DifferentialSelfTestsname the reference "Old path: ReadAs" and "Old path: CanRead"and negate the reference's answer, because the client's arms are the new path now;
StringColumnCodecTests.ReadAs_StringColumnWithoutByteStorage_SaysWhichColumnHasNoBytesexpects the failure at theReadAscall, where the reader is bound, with the same message as before.docs/tcp.mdx(a section onReadAs, and theCanReadand type mapping text) are in this part.
Benchmarks
The converter benchmarks of part 0 (
TcpConverterRead,TcpConverterWrite), this part against part 0, 3 rounds in aquiet window, each round one run of part 0 and then one run of this part. "ratio" is the median of the three ratios of
the medians (this part / part 0, below 1 is faster); "pairs" is the ratio of each round; a shape fails when it is above
1.05 in every round. Measured on
e39eb9e4; the later commit changes only an error text, a test and the changelog.The columnar shapes that convert are 0.32x to 0.78x of part 0, the identity shapes (
StringAsString,LowCardinalityNullableStringAsString, which return the column itself) are at about 1.0, and the POCO and write shapesare within noise, because this part does not change their code.
Changes after review
ReadAsview checks the row before it converts the column (Copilot).🤖 Generated with Claude Code