Repository navigation
TCP converters 6c/6: refuse array casts that change the sign of the elements - #678
alex-clickhouse wants to merge 7 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
44fa38c to
6f70eca
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The centralized rule is correct, documented, comprehensively tested across all TCP tiers, and current CI and coverage checks pass.
0 open findings
What changed in this PR
This TCP-client change prevents CLR array casts that reinterpret signedness while preserving enum/underlying-type casts.
Changes:
- Rejects unsafe array reinterpretations in shared read/write rules.
- Adds comprehensive unit, integration, and insert-route coverage.
- Updates public documentation and adds a breaking-change fragment.
| File | Description |
|---|---|
ReadRules.cs |
Filters unsafe array casts. |
WriteRules.cs |
Applies the shared cast filter to writes. |
ClickHouseTcpTypes.cs |
Documents CanRead/CanWrite behavior. |
ReadRulesTests.cs |
Covers accepted and rejected read casts. |
WriteRulesTests.cs |
Covers all write tiers and cast targets. |
WriteRuleWriterTests.cs |
Verifies retained enum-array writes. |
DerivedColumnTests.cs |
Updates CanRead expectations. |
ArrayElementCastIntegrationTests.cs |
Tests all public read/write tiers. |
DecodedColumnInsertIntegrationTests.cs |
Covers decoded-column refusals. |
WriteRulesIntegrationTests.cs |
Retains enum-array integration coverage. |
DecodedColumnConversionTests.cs |
Tests decoded composite routes. |
InsertWritePinTests.cs |
Pins dynamic and dense insert behavior. |
docs/tcp.mdx |
Documents the breaking cast rules. |
converter-6c-array-sign-casts.breaking.md |
Records the breaking change. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Summary Final verdict✅ Approve Automated review by Polyglot AI (gpt-6-astra) of |
The cast rule of the reads and the writes (ReadRules.IsAssignable, which WriteRules.CastTargets also uses) accepted every cast that the CLR allows, also the array casts that read each element as another type of the same size: Array(UInt32) read as int[], so 3000000000 read as -1294967296, and a uint[] was written into Array(Int32). This follows the user's ruling 1c on ClickHouse/integrations#801: refuse, in every tier and both directions, an array cast between integer types of the other sign and an enum array whose underlying type is not the other element type, also in jagged arrays and in the generic collection interfaces of an array. An enum array and an array of its underlying type keep the value of each element, so that cast stays: Array(Enum8(...)) reads as MyEnum[] for an enum over sbyte, and a MyEnum[] is written into Array(Int8). ReinterpretsElements now answers whether the cast gives the elements another meaning, and IsAssignable refuses those casts. ReadAs, CanRead, POCO reads, InsertAsync, the row inserts and CanWrite take the rule from there. An object, IEnumerable or other non-generic target is not affected. Tests changed to the new answers: the read and write tables of ReadRulesTests and WriteRulesTests, the list of array casts (12 accepted, 36 refused, where 48 were accepted), the write cast targets (4 enum arrays as arrays of their underlying type), and the tests that used a sign cast as their example of a cast (now an enum array, or a refusal with its message). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…erver Array(Enum8(...)) reads as an array of an enum over sbyte through POCO mapping and ReadAs. Array(UInt32) read as int[] is refused by POCO mapping and ReadAs, and a uint[] into Array(Int32) by InsertAsync, InsertRowsAsync<T> and InsertRowsAsync with object[] rows, before a block goes out; CanRead and CanWrite say false. The tests pin each message, which names the type that the column reads as (uint[]) or is written from (int[]). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The cast rules of docs/tcp.mdx say that an enum array reads from and is written as an array of its underlying type, and that an array cast that gives each element another meaning is refused. A breaking fragment says so for the release notes: code that read Array(UInt32) into an int[] property, or wrote a uint[] into Array(Int32), now gets a refusal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…utes Two write routes of an insert give a column to the converter tree of its CLR type, so the cast rule of the writes decides them as for any column: an array that ClickHouseTcpColumn.CreateArray builds (a caller's inner column or one that a query read), and a column that a query read, also a part of it that the insert writes part by part next to a part of another DateTime64 scale or next to a Nullable. The tests pin that each route refuses a uint (or an enum over uint) into Array(Int32), also jagged, with the reason where the route gives one, offline and against the server. With the cast rule of ClickHouse/integrations#801 part 6a, each of these cases writes the bits of the uint values as Int32 values. A uint[] value in a Dynamic column is not a cast: the type comes from the CLR type of the elements, so the value is an Array(UInt32) and keeps its values. A pin case shows the bytes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The helper calls the test-side WriteFull, which writes a column as an insert writes it: the codec for a column of its storage, else the converter tree of the column's CLR type. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The columnar insert bullet for a column that a query read says that only the parts of another meaning are converted and that the other parts keep the bytes that the server sent. An array cast that the write rules refuse is refused in those other parts too, for example an Array(UInt32) part into an Array(Int32) part (ClickHouse/integrations#801, ruling 1c). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An untyped insert takes the write type of a column from its first value, and the gather checks each later value with Type.IsInstanceOfType. The CLR casts a uint[] to an int[], so a uint[] after an int[] passed that check and Array(Int32) stored 3000000000 as -1294967296, in one block and in a later block. The gather now refuses a later value whose cast to the write type gives the elements another meaning (ReadRules.ReinterpretsElements), as the derivation refuses it as the first value. An enum array and an array of its exact underlying type still share a column. A value of the write type passes with one type comparison; the other checks run only for a value of another type. Tests: unit tests for a later uint[], sbyte[], jagged uint[][] and enum array over uint, also across blocks, and for the kept enum case; an integration test against the server with one and two blocks. The docs of untyped inserts say that the refused array casts apply. Part of ClickHouse/integrations#801 (ruling 1c). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
6f70eca to
d5b9839
Compare
This is part 6c of the stack of pull requests for ClickHouse/integrations#801, a converter layer for the native TCP
client. It is stacked on #677 (part 6b), which is on #674 (part 6a), #673, #672, #671, #670, #668, #667, #666 and #665.
It refuses, in every read and write tier, the .NET array casts that give each element of an array another meaning.
What changes for a user
QueryAsync<T>,Block.ReadAs,InsertAsync,InsertRowsAsync<T>,InsertRowsAsyncwithobject[]rows,ClickHouseTcpTypes.CanReadandClickHouseTcpTypes.CanWrite:Array(UInt32)does not read asint[](3000000000 read as-1294967296), a
uint[]is not written intoArray(Int32), and the same forbyte/sbyte,ushort/shortandulong/long;Array(Int32)does not read as an array of an enum overuint, and an array of an enum oversbyteis not written intoArray(UInt8)orString;IReadOnlyList<int>forArray(UInt32)).InsertAsyncrefuses these casts in each way that it writes a column: a column of your own, an array thatClickHouseTcpColumn.CreateArraybuilds, and a column that a query read, also when the insert writes such a columnpart by part (for example an
Array(UInt32)part next to aDateTime64(3)part, into aTuple(DateTime64(6), Array(Int32))column).Array(Enum8(...))reads asMyEnum[]for an
enum MyEnum : sbyte, through POCO mapping andReadAs, and aMyEnum[]is written intoArray(Int8)orArray(Enum8(...)). Each element keeps its integer value. Casts toobject,IEnumerableand other non-generictypes are not affected. A
uint[]value in aDynamiccolumn is not a cast: it is written asArray(UInt32), asbefore.
for example "Column 'value' (Array(UInt32)) maps to property 'CountsRow.Value' of type System.Int32[], which it cannot
be read as. It reads as System.UInt32[]. ..." for POCO mapping, and "... It accepts System.Int32[] ..." for the
inserts.
untyped inserts accepted: change the property or value type to the one that the message names (
uint[]forArray(UInt32)).How
The read rules and the write rules share one cast rule,
ReadRules.IsAssignable: the casts that the CLR allows,except an array cast that gives the elements another meaning (
ReadRules.ReinterpretsElements). For two value-typeelement types of one size, which are the only ones that the CLR casts between, the cast keeps the meaning only between
an enum and its exact underlying type. Reference elements are checked one level down, so jagged arrays follow the same
rule. The write rules take their cast targets through the same check, so every tier refuses the same set. Every insert
route that writes a column through the converter tree of its CLR type gets the rule from there: a column of your own,
an array that
CreateArraybuilds, and a column that a query read (whole, or a part of another type whose values keeptheir meaning). The
routes that write the stored bytes of a column, and
DynamicandVariant, which choose a type from the runtime type ofeach value, make no cast. No read or write loop changes: the rule runs when a converter is derived, and the derivation
is cached.
Why
A .NET array cast between
uint[]andint[]keeps the array and reads its bits as the other type, so a value aboveint.MaxValuesilently becomes a negative number, and back. The client accepted such casts because the CLR allowsthem. Refusing them in the one rule makes every tier give the same answer, and the message points to the type that
keeps the values.
Tests
Array(UInt32)losesint[],IntEnum[],IEnumerable<int>andIReadOnlyList<int>, and the string types losesbyte[]writes).size is listed: 12 accepted (an enum array from an array of its underlying type) and 36 refused. The write cast
targets of the matrix sources keep 4 element casts (an enum array as an array of its underlying type).
uint[]intoArray(Int32)is refused by the columnar insert, the POCO insert and the untyped insert, andCanWritesays false;Array(UInt32)asint[]is refused byReadAsandCanRead, with the messages.CreateArraybuilds over auintcolumn (also over a column that a query read, anenum over
uint, and a jagged array) is refused intoArray(Int32); a column that a query read with anArray(UInt32)is refused intoArray(Int32)as a whole column and part by part (next to a part of another scale,and next to a
Nullable), and each test checks which of these ways the insert takes. With the rule of part 6b, eachof these cases writes the bits of the
uintvalues. A byte pin shows auint[]and anint[]in aDynamiccolumn,written as
Array(UInt32)andArray(Int32)with their values.Array(Enum8(...))into an enum array through POCO mapping andReadAs;Array(UInt32)asint[]refused by POCO mapping andReadAs;uint[]intoArray(Int32)refused by the three inserts and by aCreateArrayinsert before a block goes out; a column that a query read,Array(UInt32)andTuple(DateTime64(3, 'UTC'), Array(UInt32)), refused into theInt32forms with nothing stored. Each message ispinned.
untyped write of a cast target, the cast of references over segments, the
AssignWriterderivation, theInsertAsyncwrite rule against the server), or assert the refusal (the read and write examples of the reinterpretation, one
CanReadcase, the two lists of array casts, the two tables). No test of part 6b changes its expected outcome; thispart adds tests to three of its test files.
net10.0, the offline suite (no server): 4514 passed each. The public API does not
change.
Benchmarks
No benchmark change is expected: the rule runs only when a converter is derived, and the derivation is cached. No read
or write loop changes.
Where the speedup of the stack comes from
This section is about the whole stack (parts 0 to 6c). The ratios were measured on part 6a; part 6b made some LowCardinality writes faster again.
The converter layer does not convert values faster. Both the old and the new code call the same conversion code
for each value (for example
DateTimeToOffset, with the timezone resolved once when the codec is built, and thesame UTF-8 encode and decode). The gains come from work that the old structure did around each value:
TryProjectColumnReadgave aview, and
TryProjectReadgave an expression that was compiled into a delegate for each value. A composite codecput the view of its child inside its own view. So one converted value cost a delegate call, an interface cast,
and interface calls into the column (each of which built a span again), and, for a composite, the same again
for each element. The POCO scatter used its fast tier only for columns that implement
IStoredValuesColumn;String,Nullable(...)andLowCardinality(...)columns do not, so each row went through the column'sindexer. The writes read the caller's column one element at a time through
IColumn<T>[i], andNullableput asubstitute view around it. The new code derives one tree for the whole type.
Binddoes the casts and buildsthe spans once for each column (once for each window of 256 rows in a POCO read). The bulk
Fillloops aregenerics closed over struct conversions, so the conversion is inlined into the loop. A POCO read splices the
per-row expression of the whole tree into one compiled loop for each column. For example:
LowCardinality(String)read asbyte[]: a delegate call, an interface cast, two interface calls and atype-checked array store for each row, against two loads and a store.
Array(String)read asbyte[][]: about 120,000 dispatches for each block of 10,000 rows, against 30,000.Nullable(DateTime)written fromDateTimeOffset?: three interface calls for each row, against none.ReadAs<T>allocated a new array for the whole convertedcolumn in each block. At 10,000 rows a
DateTimeOffset[](160 KB) orDateTimeOffset?[](240 KB) is on the largeobject heap, and a reader that keeps going runs a garbage collection about once in 20 blocks. The new view uses a
pooled array that the block owns (about 300 bytes allocated for each block). The old LowCardinality writer
built a new
Dictionarywith no capacity in each block: for 10,000 distinct values that is 11 resizes, 941 KBfor each block (719 KB on the large object heap) and a generation 2 collection about once in 10 blocks, and it
hashed each new value two times. The new interners keep their tables in pooled buffers that grow with the
number of distinct values, and hash each value one time (1.8 KB for each block for
FixedString).ClickHouseBinaryWritercall foreach null byte, each
UInt32and each dictionary key. The new writes put the null map in one copy, convert into4 KB chunks on the stack, and write the keys in chunks.
The work that both versions share limits the gain. A read that converts a
Stringleaf only (Stringasstring,DateTimein a POCO) does the same work for each row in both versions. ALowCardinalitydictionary isconverted one time for each block in both. A repeated
stringintoLowCardinality(String)costs one dictionarylookup for each row in both.
Ratios in the default runtime mode (tiered compilation with dynamic PGO, the steady state), new time / old time
for a block of 10,000 rows, measured with the old and the new code side by side:
Block.ReadAs:DateTime('UTC')asDateTimeOffsetBlock.ReadAs:Nullable(DateTime('UTC'))asDateTimeOffset?Block.ReadAs:LowCardinality(String)asbyte[]Block.ReadAs:Array(String)asbyte[][]LowCardinality(Nullable(String))asstringLowCardinality(String)asbyte[]Array(String)asbyte[][]Nullable(DateTime('UTC'))asDateTimeOffset?Stringasstring,DateTime('UTC')asDateTimeOffsetInsertAsync:Nullable(DateTime('UTC'))fromDateTimeOffset?InsertAsync:LowCardinality(FixedString(16))frombyte[], all distinctInsertAsync:LowCardinality(String)fromstring, all distinctInsertAsync:LowCardinality(String)fromstring, 100 distinctInsertRowsAsync<T>:Nullable(DateTime('UTC'))fromDateTimeOffset?The gate of each part runs with tiered compilation off, so that the code does not change during a run.
Its ratios differ from the ones above: in that mode the JIT cannot remove the old per-row interface calls of
ordinary methods, and it compiles the new loops without the inlining that tier 1 adds.
Changes after review
uint[]after anint[]inArray(Int32)is refused with its row, also across blocks (Polyglot).docs/tcp.mdxsays so.🤖 Generated with Claude Code