TCP K: geo aliases, Geometry, SimpleAggregateFunction and AggregateFunction - #577
Draft
alex-clickhouse wants to merge 1 commit into
Draft
TCP K: geo aliases, Geometry, SimpleAggregateFunction and AggregateFunction#577alex-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 TCP/native support for geo aliases, Geometry, and aggregate-function column types while improving ambiguous Variant handling.
Changes:
- Registers structural codecs for geo and aggregate aliases.
- Rejects ambiguous Variant CLR mappings.
- Adds unit and integration coverage.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
ColumnCodecRegistry.cs |
Registers new type factories. |
VariantColumnCodec.cs |
Handles ambiguous CLR alternatives and aliases. |
TupleColumnCodec.cs |
Supports alias type names. |
GeoColumnCodecs.cs |
Defines geo alias structures. |
ArrayColumnCodec.cs |
Supports alias type names. |
AggregateFunctionColumnCodecs.cs |
Resolves or rejects aggregate types. |
InsertRoundTripCase.cs |
Adds round-trip cases. |
VariantColumnCodecTests.cs |
Tests Variant ambiguity behavior. |
NullableColumnCodecTests.cs |
Updates unsupported-type coverage. |
ColumnCodecRegistryTests.cs |
Tests new registrations and diagnostics. |
PocoWriteIntegrationTests.cs |
Tests Geometry POCO refusal. |
GeometryIntegrationTests.cs |
Validates Geometry discriminator ordering. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
alex-clickhouse
force-pushed
the
tcp/epic-k-type-aliases
branch
from
August 21, 2026 13:07
ce7b178 to
0ca474d
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-k-type-aliases
branch
2 times, most recently
from
August 22, 2026 17:06
5919ae3 to
68b1c8a
Compare
alex-clickhouse
force-pushed
the
tcp/epic-k-type-aliases
branch
from
August 22, 2026 17:25
68b1c8a to
922a9fa
Compare
alex-clickhouse
force-pushed
the
tcp/epic-k-type-aliases
branch
2 times, most recently
from
August 26, 2026 15:24
13913b2 to
f234164
Compare
alex-clickhouse
force-pushed
the
tcp/epic-k-type-aliases
branch
from
August 26, 2026 15:40
f234164 to
3a6bfbd
Compare
alex-clickhouse
force-pushed
the
tcp/epic-k-type-aliases
branch
from
August 26, 2026 16:39
3a6bfbd to
0e5341d
Compare
alex-clickhouse
force-pushed
the
tcp/epic-k-type-aliases
branch
from
August 26, 2026 19:00
0e5341d to
8f1d4b5
Compare
alex-clickhouse
force-pushed
the
tcp/epic-k-type-aliases
branch
from
August 28, 2026 11:04
8f1d4b5 to
06995d1
Compare
alex-clickhouse
force-pushed
the
tcp/epic-k-type-aliases
branch
2 times, most recently
from
August 28, 2026 16:02
521261e to
d6cd6ad
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
ClickHouse.Driver.Tcp/Types/ColumnCodecRegistry.cs:188
- This adds user-visible type support and changes
Variantwrite behavior, but the PR has nochangelog.dfragment.AGENTS.md:342-365requires every client behavioral change to add one; please add a short feature/fix fragment for these changes.
AddFactory("SimpleAggregateFunction", static (TypeNode node, in ResolveContext context, ColumnCodecRegistry registry) => AggregateFunctionColumnCodecs.CreateSimple(node, in context, registry));
AddFactory("AggregateFunction", static (TypeNode node, in ResolveContext _, ColumnCodecRegistry _) => AggregateFunctionColumnCodecs.RefuseAggregateFunction(node));
Comment on lines
557
to
558
| throw new ArgumentException( | ||
| $"Variant '{TypeName}' has no alternative for a value of CLR type '{clrType}'. Supported CLR types: {string.Join(", ", discriminatorByClrType.Keys)}."); |
Epic K, minus QBit. Four type names the server puts in a column header that the client had no factory for, so each one failed the whole query with "not supported by this client yet". Point is Tuple(Float64, Float64) and Ring, LineString, Polygon, MultiLineString and MultiPolygon are arrays layered over it, so none needs a codec: only a registration that resolves the structure. The name on the wire is always the alias, though, so ArrayColumnCodec, TupleColumnCodec and VariantColumnCodec take an optional type name and report the alias rather than the structure it stands for. The aliases are defined in terms of each other, so a Polygon's inner codec calls itself Ring. Geometry is an alias to a Variant over those six. Its order is the server's name-sorted one, which nothing on the wire carries, so GeometryIntegrationTests compares the client's list against what the server calls each discriminator. No round trip can do that job: the client applies its own order to the write and to the read, so a transposition of the two structurally identical pairs cancels out. Several alternatives can surface one CLR type, so the runtime type alone does not always name one. Where the value settles it, it does: IColumnCodec.ClaimsValue asks each colliding alternative and exactly one claimant wins, which is how IPv4 and IPv6 resolve from the address family. Where nothing settles it -- JSON against String, Int64 against DateTime64 and Time64, Ring against LineString -- the write is refused and the message names the alternatives it could not choose between, prescribing nothing, because a caller has no way yet to say which one is meant. The unambiguous lookup stays a single dictionary hit, so a collision costs only the columns that have one. That refusal is per value rather than per column on purpose: an IColumn<object> says nothing about the runtime types it holds, and Variant(IPv4, IPv6, String) must keep taking a string. It also settles the Variant(JSON, String) ambiguity left open by J5. SimpleAggregateFunction(func, T) encodes as a bare T, so it resolves to T's codec itself -- no renaming wrapper, which would cost a virtual call per operation for a cosmetic name. AggregateFunction is refused with a merge query that runs: the combinator attaches to the bare function name, and a parameterized one keeps its parameters ahead of the column, so quantiles suggests quantilesMerge(0.5, 0.9)(column). Geometry is gated on TcpFeature.Geometry; on the 25.8 floor the server resolves it to String at DDL time. The geo aliases and the aggregate types need no gate. Co-Authored-By: Claude <noreply@anthropic.com>
alex-clickhouse
force-pushed
the
tcp/epic-k-type-aliases
branch
from
August 28, 2026 16:32
d6cd6ad to
c9a1fce
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.
Four families of type name the server puts in a column header that the client had no factory for. Each one failed the whole query with
ClickHouse type '...' is not supported by this client yet.Geo aliases
PointisTuple(Float64, Float64);RingandLineStringareArray(Point);PolygonandMultiLineStringare arrays over those;MultiPolygonisArray(Polygon). None needs a codec — only a registration that resolves the structure. They surface the structural CLR value ((double, double)up to(double, double)[][][]), which is what the TCP parameter formatter already accepted for these names and structurally what the HTTP driver surfaces.The name on the wire is always the alias — a header says
Point, neverTuple(Float64, Float64)— soArrayColumnCodec.Create,TupleColumnCodec.CreateandVariantColumnCodec.Createtake an optional type name and report the alias. The aliases are defined in terms of each other, mirroring the HTTP driver's class hierarchy, so aPolygon's inner codec calls itselfRing.Ring/LineStringandPolygon/MultiLineStringare structurally identical, so each has its own round-trip case: a registration pointing one at the other's codec would still pass the other's case.Geometry, and Variant collisions
Geometryis an alias toVariant(LineString, MultiLineString, MultiPolygon, Point, Polygon, Ring)— the server's canonical name-sorted order, which nothing on the wire carries.Several alternatives can surface one CLR type, so the runtime type alone does not always name one.
VariantColumnCodecpreviously resolved this with aDictionary.TryAddtie-break, silently writing aRingas aLineString. It now asks the value.Where the value settles the collision, it does.
IColumnCodec.ClaimsValueis a new default interface member, so 22 of 24 codecs are untouched. It is narrower thanCanWrite: not "could you store this" but "is this value yours".IPv4andIPv6override it onAddressFamily, andVariant(IPv4, IPv6, String)now writes an address of either family. IPv6 declines an IPv4 address even though its writer maps one, because beside an IPv4 alternative that address means IPv4; a standaloneIPv6column, or anIPv6alternative with noIPv4sibling, resolves by type and never reaches the hook.Where nothing settles it, the write is refused. Exactly one claimant resolves a collision; zero or several is a refusal. That rule is what makes the hook safe — first-match-wins (what the HTTP driver's
VariantTypedoes) would have silently pickedJSONfor a string inVariant(JSON, String), reintroducing the bug. The default claims everything, so a codec with no value-level test can never win a tie on its own. Refused:JSONvsString,Int64vsDateTime64vsTime64(all three surface the rawlong), and the twoGeometrypairs.The refusal message names the alternatives it could not choose between and prescribes nothing. It used to say "supply a dense
VariantColumn", which isinternaland unreachable from outside the assembly — a remedy pointing at a door that is not open. A public way to name an alternative explicitly is filed as K6.The refusal is per value, not per column. An
IColumn<object>says nothing about the runtime types it holds, so refusing the whole column would also reject every unambiguous value in it. AstringinVariant(IPv4, IPv6, String)and aulonginVariant(JSON, String, UInt64)must keep working, and they do. This also settles theVariant(JSON, String)ambiguity left open by J5.The unambiguous lookup stays a single dictionary hit; candidates are walked only on a miss, so a collision costs only the columns that have one.
Aggregate states
SimpleAggregateFunction(func, T)encodes as a bareT, so it resolves toT's codec itself rather than a renaming wrapper, which would cost a virtual call per operation for a cosmetic type name. The inner goes back through the registry, so a composite, nullable, parameterized or itself-aliasedTall work.AggregateFunctionis refused with a merge query that actually runs. The combinator attaches to the bare function name and a parameterized function keeps its parameters ahead of the column, soAggregateFunction(quantiles(0.5, 0.9), UInt64)suggestsquantilesMerge(0.5, 0.9)(column)— notquantiles(0.5, 0.9)Merge(column), which is not a function, and not a barequantilesMerge(column), which the server rejects for wanting its parameters.Testing
Round-trip corpus cases for every alias, for
Variant(IPv4, IPv6, String), and for fourSimpleAggregateFunctionshapes (scalar, nullable, composite, parameterized function). Codec unit tests for the resolution surface and the refusal paths only, per the test-layering rules.The three-way collision case (
Variant(DateTime64(3), Int64, Time64(3))) is mutation-checked: without the fix it writes discriminator 2, storing anInt64as aTime64. So is the IP tie-break — reverting either override fails the unit test and both corpus round-trips, because a misplaced IPv4 reads back as::ffff:10.0.0.1.GeometryIntegrationTestspins the discriminator order against the server. No round trip can: the client applies its own order to the write and to the read, so a transposition cancels out, and for the two structurally identical pairs the block is byte-identical as well. Nor is it enough to write discriminator i and ask the server what it calls i — that answers from the server's own list. The expectation is read out of the codec and compared with the server's name for the row written at each position; mutation-checked by transposingRingandLineString, which the first version of the test did not catch.Coverage on the new files is 100%; the touched composites are 91–96%.
Geometryis gated onTcpFeature.Geometry— on the 25.8 CI floor the server resolves it toStringat DDL time, so an ungated case would silently exercise aStringcolumn there. Verified by enumerating the corpus atCLICKHOUSE_VERSION=25.8: the Geometry cases disappear, the geo aliases stay.🤖 Generated with Claude Code