Skip to content

TCP I4: Add Map(K, V) support - #443

Open
alex-clickhouse wants to merge 10 commits into
tcp/epic-i3-tuplefrom
tcp/epic-i4-map
Open

TCP I4: Add Map(K, V) support#443
alex-clickhouse wants to merge 10 commits into
tcp/epic-i3-tuplefrom
tcp/epic-i4-map

Conversation

@alex-clickhouse

@alex-clickhouse alex-clickhouse commented Jul 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

Adds Map(K, V) to the TCP/Native client (epic I4), stacked on the Tuple branch (I3).

On the wire, Map(K, V) is byte-identical to Array(Tuple(K, V)): the key then value codec's state prefix, a per-row UInt64 offsets stream, then a flat keys stream and a flat values stream, positionally aligned (pair i = (keys[i], values[i])). The codec reuses Array's offset framing and Tuple's per-element streams.

Design decision: KeyValuePair<K, V>[], not Dictionary<K, V>

Each row surfaces as KeyValuePair<K, V>[] so that duplicate keys and pair order — both meaningful on the wire — round-trip intact. A Dictionary would silently collapse duplicates. This intentionally diverges from the HTTP driver's Dictionary<K, V> (the two clients are permitted to surface different CLR types), and is also more span/pool-friendly.

Also accepting dictionary rows on the write path was tried as an experiment and dropped as not worth it. Advertising Dictionary<K, V>/IReadOnlyDictionary<K, V> as extra writable element types meant a third specialized flatten path, plus a per-row size re-check with a throw on mismatch — unlike a pair array, a dictionary can change size between the flatten's measure and copy passes, and a stale trailing slot would be stored as real data. That is a lot of machinery to save a caller one enumeration into pairs, so a write column supplies KeyValuePair<K, V>[].

  • Map rows are non-nullable; the server rejects Nullable(Map(...)), so nullability composes inside the value as Map(K, Nullable(V)). Map keys are themselves non-nullable in ClickHouse.
  • Nested composites recurse: Map(String, Array(T)), Map(String, Tuple(...)), Array(Map(K, V)).

Changes

  • New Types/Codecs/MapColumnCodec.cs — codec + IMapShape/MapShape<K,V>/MapShapes bridge (mirrors ArrayColumnCodec, carrying two child codecs over one offsets vector).
  • New Types/MapColumn.cs — IColumn<KeyValuePair<K,V>[]> (mirrors ArrayValueColumn).
  • Types/ColumnCodecRegistry.cs — register AddFactory("Map", …).
  • New Tests/Types/MapColumnCodecTests.cs and Map cases in Tests/Utilities/InsertRoundTripCase.cs.
  • Repointed two tests (ColumnCodecRegistryTests, NullableColumnCodecTests) that used Map(...) as their "well-formed but unimplemented type" placeholder to Nested(...), the next unimplemented composite.

Testing

  • 692/692 TCP tests pass (net9.0), including a new MapColumnCodecTests and 7 integration round-trip cases proven against a real ClickHouse server (Map(String, UInt32), Map(UInt8, String), empty/all-empty, Map(String, Nullable(UInt32)), Map(String, Array(Int32)), Map(String, Tuple(Int32, String)), Array(Map(String, UInt32))).
  • Builds clean across net6.0–net10.0.
  • Coverage: MapColumn.cs 100%, MapColumnCodec.cs 92.9% (remainder = multi-GB overflow guards + cleanup catch-blocks, same as Array).
  • A review pass found no Critical/Major issues; applied two Minor fixes (null-tolerant flatten guard for Array parity; pooled scratch buffers in MeasureRow so the common Map(String, …) path doesn't allocate per row).

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Jul 16, 2026 •
edited
Loading

Copy link
Copy Markdown

Triage

Category: feature • Risk: high

Summary
Implements Map(K, V) support for the TCP/Native client (epic I4, stacked on Tuple/I3). The wire format is byte-identical to Array(Tuple(K, V)): the codec delegates the state-prefix phase to the key then value child codecs, then reads/writes a per-row UInt64 offsets stream followed by two flat streams (all keys, then all values, positionally aligned). Each row is surfaced as KeyValuePair<K, V>[] rather than Dictionary<K, V> to preserve duplicate keys and pair order. Two new files add the codec (MapColumnCodec.cs, 487 lines including the IMapShape/MapShape<K,V>/MapShapes bridge) and the column type (MapColumn.cs, 184 lines); ColumnCodecRegistry.cs gains a four-line AddFactory("Map", ...) call. Tests include 12 codec unit tests (wire bytes, error paths, API surface, ownership) and 9 integration round-trip cases (Map(String, UInt32), Map(UInt8, String), all-empty, Nullable(V), Array(V), Tuple(V), both-fixed, Map(K, Map(...)), Array(Map(...))).

What this impacts

  • ClickHouse.Driver.Tcp/Types/Codecs/ — new binary read/write codec for Map(K, V) with full offsets-plus-two-streams framing
  • ClickHouse.Driver.Tcp/Types/ — new MapColumn<TKey, TValue> column type; existing ColumnCodecRegistry registration
  • Test matrix: MapColumnCodecTests (unit), InsertRoundTripCase (integration, 9 new cases), ColumnCodecRegistryTests (1-line placeholder swap)
  • Public-facing CLR element type diverges from HTTP client: TCP returns KeyValuePair<K,V>[], HTTP returns Dictionary<K,V>

Concerns

  • Type system / binary protocol rule fires: The codec implements the full binary read and write path for a new composite type with two child streams and a pooled offsets buffer — a single byte-order or offset-framing bug silently corrupts data.
  • Reflection in MapShapes.Build: Activator.CreateInstance(typeof(MapShape<,>).MakeGenericType(...), nonPublic: true) is called once per key/value type pair and cached in a ConcurrentDictionary. This follows the established NullableShape/TupleShape pattern, so the reflection is not hot-path. However, the high-risk rule flags any new reflection usage; reviewers should confirm the cache is hit on the common path.
  • DRAFT state: The PR is marked as a draft. Formal review may be premature; confirm readiness with the author before investing review time.
  • Stacked on I3 (Tuple branch): PR body says it is stacked on the Tuple PR. If I3 has not yet merged, reviewers need to diff against the correct base to see only the Map changes.
  • HTTP client divergence: The intentional KeyValuePair<K,V>[] vs Dictionary<K,V> split between TCP and HTTP clients is documented in the PR body, but will need guidance in user-facing docs to avoid confusion.

Required reviewer action

  • High risk: PR body already includes an architectural description (wire format, design decisions, test coverage); at least one human reviewer must validate the binary codec correctness, the pooled-offset lifecycle, and the MapShapes caching behavior before merge.

@alex-clickhouse
alex-clickhouse requested a review from Copilot July 16, 2026 11:35
Copilot AI reviewed Jul 16, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds native/TCP (ClickHouse binary protocol) support for the ClickHouse Map(K, V) type by introducing a dedicated column shape and codec that mirror the existing Array(T)/Tuple(...) composite patterns.

Changes:

  • Added MapColumnCodec implementing the Map(K, V) wire layout (offsets + flat key stream + flat value stream), including typed shape-bridging and pooled-buffer write paths.
  • Added MapColumn<TKey, TValue> as the dense decoded column representation (flat key/value inner columns + offsets) surfaced as KeyValuePair<TKey, TValue>[] per row.
  • Registered the new Map codec and expanded unit + round-trip test coverage; updated prior “unsupported composite” placeholders from Map(...) to Nested(...).

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
ClickHouse.Driver.Tcp/Types/MapColumn.cs New dense decoded column type for Map(K,V) exposing rows as KeyValuePair[] while retaining zero-copy inner streams for re-serialization.
ClickHouse.Driver.Tcp/Types/ColumnCodecRegistry.cs Registers the Map factory in the default codec registry.
ClickHouse.Driver.Tcp/Types/Codecs/MapColumnCodec.cs New Map(K,V) codec: state prefix delegation, offsets parsing, dense/jagged write paths, and row sizing logic.
ClickHouse.Driver.Tcp.Tests/Utilities/InsertRoundTripCase.cs Adds integration-style round-trip cases for multiple Map shapes and nesting.
ClickHouse.Driver.Tcp.Tests/Types/NullableColumnCodecTests.cs Updates “unsupported inner” test placeholder from Map(...) to Nested(...).
ClickHouse.Driver.Tcp.Tests/Types/MapColumnCodecTests.cs New focused unit tests covering read/write, slicing, null-row rejection, offset corruption guards, and sizing paths.
ClickHouse.Driver.Tcp.Tests/Types/ColumnCodecRegistryTests.cs Updates “well-formed but unsupported” test placeholder from Map(...) to Nested(...).

@alex-clickhouse alex-clickhouse mentioned this pull request Jul 21, 2026
Draft
@alex-clickhouse alex-clickhouse changed the title Add Map(K, V) support for the TCP client TCP I4: Add Map(K, V) support Jul 23, 2026
@alex-clickhouse
alex-clickhouse requested a review from Copilot July 24, 2026 07:11
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

alex-clickhouse and others added 10 commits July 30, 2026 11:45
@alex-clickhouse @claude
Add Map(K, V) support for the TCP client
9d724e4 
Map is byte-identical to Array(Tuple(K, V)) on the wire: the key then the
value codec's state prefix, a per-row UInt64 offsets stream, then a flat keys
stream and a flat values stream, positionally aligned. MapColumnCodec reuses
Array's offset framing and Tuple's per-element streams, bridging the two child
codecs to a typed MapColumn<TKey, TValue> through a cached per-type-pair shape.

Each row surfaces as KeyValuePair<K, V>[] rather than Dictionary<K, V> so that
duplicate keys and pair order — both meaningful on the wire — round-trip intact;
a dictionary would silently collapse duplicates. Map rows are non-nullable and
the server rejects Nullable(Map(...)), so nullability composes inside the value
as Map(K, Nullable(V)); Map keys are themselves non-nullable.

Two existing tests used Map as their "well-formed but unimplemented type"
placeholder; repointed them to Nested(...), the next unimplemented composite.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@alex-clickhouse @claude
Add Densify for Map
67504fb 
Map gains IColumnCodec.Densify: it projects the ergonomic KeyValuePair[]-per-row
column into the dense map column (offsets + a flat key column + a flat value
column) once, recursing the key and value codecs' own Densify so a composite
key/value (Map(K, Array(V)), Map(K, Tuple(...)), Map(K, Nullable(V))) becomes
dense all the way down; an already-dense map is returned by reference when neither
child changed. This is what lets the pipeline hand Map's children the dense form
their now-single dense write path expects.

When only one of the key/value columns needs densifying, the rebuilt MapColumn
keeps the other by reference (still owned by the source column) and builds the
changed one fresh. MapColumn previously always disposed both, so disposing such a
rebuilt wrapper would double-dispose the borrowed column (returning pooled buffers
twice). It now takes per-child ownership (RestrictOwnership) and the rebuild owns
only the column it created.

The dense-form end-to-end coverage — InsertAsync_DenseReadbackReinserted_RoundTripsThroughSelect,
introduced earlier in the stack — extends to Map automatically: it re-inserts the
dense read-back MapColumn for every Map InsertRoundTripCase alongside the ergonomic one.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@alex-clickhouse @claude
Ergonomic Map write; drop densify/measure
eaa0eaa 
Map writes its offsets then the flattened key/value runs straight from
the ergonomic source, recursing into the key and value codecs, with no
densify pre-pass or byte measurement.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@alex-clickhouse @claude
Cut Map unit tests that duplicate the server round-trips, and close t…
214f2db 
…he gaps

Six codec round-trips asserted values the Map cases in InsertRoundTripCase
already prove through a real INSERT/SELECT:

  ..._FixedWidthKeyAndValueRoundTripsWithEmptyRows -> Maps<string,uint> / Maps<byte,string>
  ..._StringKeyRoundTrips                          -> Maps<string,uint>
  ..._NullableValueRoundTrips                      -> Maps<string,uint?> (same rows)
  ..._ArrayValueRoundTrips                         -> Maps<string,int[]> (same rows)
  ReadColumn_EveryRowEmpty_RoundTripsAsAllEmpty    -> the "every row empty" case
  WriteColumn_DenseMapColumn_...                   -> InsertAsync_DenseReadbackReinserted

The dense one is worth noting: it looks like an internal shape unreachable
from integration, but InsertAsync_DenseReadbackReinserted re-inserts the
read-back MapColumn for every Map case, so that path is covered end to end.

ReadColumn_WriteThenRead_DuplicateKeysWithinRowArePreserved stays -- the
server rejects duplicate keys on insert, so no round-trip can express it --
and it now also owns the Values coverage, widened with an empty-map row and a
RowCount assertion. MapColumn.Values materializes the whole jagged cache in
one pass while GetValue takes the uncached indexer, and the length == 0 branch
of Values was otherwise unreached.

Two integration cases added: a Map with both key and value fixed-width (the
existing cases always pair one fixed with one variable), and a Map whose value
is itself a Map -- the only path recursing the map shape through itself, which
neither layer covered.

Verified against ClickHouse 26.6 -- 823 tests pass, integration included.

Co-Authored-By: Claude <noreply@anthropic.com>
@alex-clickhouse @claude
Bound Map's row offsets to RowCount instead of reading the raw buffer
8e1866f 
Same defect the Array column had: Materialize read offsets[row] and offsets[row + 1] out of
the offsets buffer, which is normally a pooled array longer than the column. A row past
RowCount read the tail, and since a previous, larger read leaves monotonic offsets behind
there, the pair is usually still increasing — so the column handed out real-looking pairs
from the flat key and value columns rather than throwing.

The row count stays a parameter for the same reason it does on Array: the key and value
columns are flat (one entry per pair across every row) and the offsets are pooled, so the
constructor validates the offsets length instead of deriving anything.

Co-Authored-By: Claude <noreply@anthropic.com>
@alex-clickhouse
Expose the map column's flat key and value columns publicly
a6f2249 
Add IMapColumn<TKey, TValue>, the public read surface for the wire layout under
a Map(K, V): the two flat, positionally aligned key and value columns plus the
per-row offsets that delimit each row's entries. Same shape IArrayColumn
exposes — Map is byte-identical to Array(Tuple(K, V)) — with the run split
across two columns.

Beyond avoiding the per-row KeyValuePair[] the materialized surface allocates,
this lets a caller read only the keys or only the values, and preserves
duplicate keys and entry order, which the pair array carries but no
dictionary-shaped view could.

Offsets stays sliced to RowCount + 1, so the public span cannot expose the tail
of the pooled buffer behind it.
@alex-clickhouse
Sharpen the map read surface docs
fa71516 
Same two corrections as the nullable and array branches: "zero-copy" overclaimed
(reading a child column's values still materializes for string-like and composite
element types), and the two child columns are IDisposable, so say plainly that
they are the block's to dispose rather than the caller's.
@alex-clickhouse @claude
Cover a Map element inside a Tuple
8f02b1a 
Tuple had cases for an Array and a nested Tuple element but none for a Map, so
nothing proved the map's offsets and key/value streams get written from the
per-element column the tuple projects rather than from the tuple's own column.

Co-Authored-By: Claude <noreply@anthropic.com>
@alex-clickhouse @claude
Drop Map's forwarded-column prefix branch
9113614 
Last of the three. The shape's BeginWrite is now called unconditionally, so a
column this codec cannot write fails there rather than silently emitting the
key and value prefixes against a foreign column.

Co-Authored-By: Claude <noreply@anthropic.com>
@alex-clickhouse @claude
Name the state Map expected when it gets a foreign one
22f1c0a 
Map cannot type-test in the codec the way Array and Tuple do: its state is a
private nested type of the generic shape, unnameable from the non-generic codec,
which is why the codec only null-tests and hands anything non-null through.
The hard cast on the other side then failed as a bare InvalidCastException from
inside the shape.

That cast now reports which state arrived and which was wanted. The cast stays
first so the succeeding path builds no type name -- the shape is shared by every
map codec with the same key and value CLR types, so it has nowhere to cache one.

Co-Authored-By: Claude <noreply@anthropic.com>
@alex-clickhouse
alex-clickhouse requested a review from Copilot August 3, 2026 16:56
Copilot AI reviewed Aug 3, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

@alex-clickhouse alex-clickhouse mentioned this pull request Aug 4, 2026
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments
@mzitnik mzitnik Awaiting requested review from mzitnik mzitnik is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

risk:high triage:feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@alex-clickhouse