TCP R1-R4 + C9: declare and enforce the public API surface - #592
Draft
alex-clickhouse wants to merge 5 commits into
Draft
TCP R1-R4 + C9: declare and enforce the public API surface#592alex-clickhouse wants to merge 5 commits into
alex-clickhouse wants to merge 5 commits into
Conversation
…tion Epic R1-R4 plus C9. The TCP client's public surface is now declared and enforced, and every public member is documented. Surface changes, from the review of what to expose: - Every public type moves into the ClickHouse.Driver.Tcp namespace, so a caller needs one using rather than four. IDynamicColumn, IVariantColumn and ColumnElementTypes get their own files to allow it. - ClickHouseDecimal becomes ClickHouseTcpDecimal. The HTTP driver already publishes a ClickHouseDecimal, and one package cannot carry two public types of that name without confusing every caller. - ClickHouseTcpColumn.Create builds the columns InsertAsync takes. Until now every concrete column was internal and there was no factory, so the method was public but uncallable from outside the assembly. Columns built this way carry no type name: the insert reads the target's type from the server's schema, so asking the caller for one only invites a wrong answer we would ignore. - GetServerInfoAsync reports the handshake's version, protocol revision and timezone, which nothing exposed before. - ExecuteScalarAsync returns the first cell of the first row. - ClickHouseTcpDataSource owns a client and hands out views whose disposal does nothing, so a scoped consumer cannot close a pool it does not own. C9: the column codecs reinterpret wire bytes as CLR values, which holds only on a little-endian host. One check at client and connection construction replaces the per-codec guards. The analyzer covers ClickHouse.Driver.Tcp and ClickHouse.Driver.Common, each with an .editorconfig raising RS0016/RS0017/RS0024 to error. RS0048 is an error too: the analyzer needs exactly one Shipped/Unshipped pair and runs no surface check at all when given one file or two pairs, and RS0048 is the only sign it has stopped looking. ClickHouse.Driver is left alone; wiring it needs ~230 symbols declared first, and is filed separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9040442. Configure here.
Contributor
There was a problem hiding this comment.
Pull request overview
Declares and enforces the native TCP client’s public API while adding missing public entry points and centralizing platform validation.
Changes:
- Adds column factories, scalar queries, server information, and data-source ownership APIs.
- Moves public TCP types into the root namespace and renames the TCP decimal type.
- Enables Public API analyzers and adds integration/unit coverage.
Reviewed changes
Copilot reviewed 55 out of 56 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
Directory.Packages.props |
Pins the API analyzer. |
ClickHouse.Driver.Tcp/Types/VariantColumn.cs |
Extracts the public interface. |
ClickHouse.Driver.Tcp/Types/IVariantColumn.cs |
Declares the variant surface. |
ClickHouse.Driver.Tcp/Types/ITupleColumn.cs |
Moves interface namespace. |
ClickHouse.Driver.Tcp/Types/IQBitColumn.cs |
Moves interface namespace. |
ClickHouse.Driver.Tcp/Types/INullableColumn.cs |
Moves interface namespace. |
ClickHouse.Driver.Tcp/Types/INestedColumn.cs |
Moves interface namespace. |
ClickHouse.Driver.Tcp/Types/IMapColumn.cs |
Moves interface namespace. |
ClickHouse.Driver.Tcp/Types/ILowCardinalityColumn.cs |
Moves interface namespace. |
ClickHouse.Driver.Tcp/Types/IDynamicColumn.cs |
Declares the dynamic surface. |
ClickHouse.Driver.Tcp/Types/IColumn.cs |
Moves and documents column APIs. |
ClickHouse.Driver.Tcp/Types/IArrayColumn.cs |
Moves interface namespace. |
ClickHouse.Driver.Tcp/Types/DynamicColumn.cs |
Extracts the public interface. |
ClickHouse.Driver.Tcp/Types/ColumnElementTypes.cs |
Extracts internal type resolution. |
ClickHouse.Driver.Tcp/Types/ColumnCodecRegistry.cs |
Updates namespace usage. |
ClickHouse.Driver.Tcp/Types/Codecs/DynamicTypeInference.cs |
Applies decimal rename. |
ClickHouse.Driver.Tcp/Types/Codecs/DecimalColumnCodec.cs |
Uses the renamed decimal type. |
ClickHouse.Driver.Tcp/Types/ClickHouseTcpColumn.cs |
Adds public column factories. |
ClickHouse.Driver.Tcp/PublicAPI/PublicAPI.Unshipped.txt |
Declares the TCP API surface. |
ClickHouse.Driver.Tcp/PublicAPI/PublicAPI.Shipped.txt |
Establishes shipped API tracking. |
ClickHouse.Driver.Tcp/Protocol/HostEndianness.cs |
Adds little-endian validation. |
ClickHouse.Driver.Tcp/Protocol/ClickHouseTcpConnection.cs |
Guards connection construction. |
ClickHouse.Driver.Tcp/Protocol/ClickHouseBinaryWriter.cs |
Updates namespace usage. |
ClickHouse.Driver.Tcp/Protocol/ClickHouseBinaryReader.cs |
Updates namespace usage. |
ClickHouse.Driver.Tcp/Poco/PocoScatterTier.cs |
Updates API documentation reference. |
ClickHouse.Driver.Tcp/Parameters/TcpParameterFormatter.cs |
Formats renamed decimals. |
ClickHouse.Driver.Tcp/Parameters/ParameterTypeInference.cs |
Infers renamed decimals. |
ClickHouse.Driver.Tcp/Numerics/UInt256.cs |
Moves and documents operators. |
ClickHouse.Driver.Tcp/Numerics/Int256.cs |
Moves and documents operators. |
ClickHouse.Driver.Tcp/Numerics/ClickHouseTcpDecimal.cs |
Renames and documents decimal API. |
ClickHouse.Driver.Tcp/Format/Block.cs |
Moves Block to the root namespace. |
ClickHouse.Driver.Tcp/Client/IClickHouseTcpOperations.cs |
Adds scalar and server-info APIs. |
ClickHouse.Driver.Tcp/Client/ClickHouseTcpSession.cs |
Delegates the new operations. |
ClickHouse.Driver.Tcp/Client/ClickHouseTcpServerInfo.cs |
Adds handshake information model. |
ClickHouse.Driver.Tcp/Client/ClickHouseTcpDataSource.cs |
Adds shared-client ownership API. |
ClickHouse.Driver.Tcp/Client/ClickHouseTcpClient.cs |
Implements new operations and guard. |
ClickHouse.Driver.Tcp/ClickHouse.Driver.Tcp.csproj |
Enables API analysis. |
ClickHouse.Driver.Tcp/.editorconfig |
Enforces API diagnostics. |
ClickHouse.Driver.Tcp.Tests/Utilities/InsertRoundTripCase.cs |
Updates decimal test cases. |
ClickHouse.Driver.Tcp.Tests/Types/NullableColumnCodecTests.cs |
Updates namespace usage. |
ClickHouse.Driver.Tcp.Tests/Types/FixedWidthColumnCodecTests.cs |
Updates namespace usage. |
ClickHouse.Driver.Tcp.Tests/Types/DynamicTypeInferenceTests.cs |
Updates decimal inference tests. |
ClickHouse.Driver.Tcp.Tests/Types/DecimalColumnCodecTests.cs |
Updates decimal codec tests. |
ClickHouse.Driver.Tcp.Tests/Types/ClickHouseTcpColumnTests.cs |
Tests column factory behavior. |
ClickHouse.Driver.Tcp.Tests/Protocol/ClickHouseBinaryReaderWriterTests.cs |
Updates namespace usage. |
ClickHouse.Driver.Tcp.Tests/Parameters/TcpParameterFormatterTests.cs |
Updates decimal formatting tests. |
ClickHouse.Driver.Tcp.Tests/Parameters/TcpParameterFormatterEdgeCaseTests.cs |
Updates inference edge cases. |
ClickHouse.Driver.Tcp.Tests/Numerics/Int256Tests.cs |
Updates namespace usage. |
ClickHouse.Driver.Tcp.Tests/Numerics/ClickHouseTcpDecimalTests.cs |
Tests renamed decimal type. |
ClickHouse.Driver.Tcp.Tests/Integration/PublicSurfaceIntegrationTests.cs |
Exercises the new public APIs. |
ClickHouse.Driver.Tcp.Tests/Integration/ClickHouseTcpDataSourceIntegrationTests.cs |
Tests data-source ownership. |
ClickHouse.Driver.Tcp.Tests/Integration/ClickHouseTcpClientQueryIntegrationTests.cs |
Updates namespace usage. |
ClickHouse.Driver.Tcp.Tests/Client/ClickHouseTcpServerInfoTests.cs |
Tests server information behavior. |
ClickHouse.Driver.Common/ClickHouse.Driver.Common.csproj |
Enables API analysis. |
ClickHouse.Driver.Common/.editorconfig |
Enforces API diagnostics. |
changelog.d/418-tcp-native-client.features.md |
Documents the release feature. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Returning at the first value left the stream early, which is the abandon path: the connection sends Cancel and terminates. On a session that destroyed the session outright, temporary tables and settings included, for a query as small as SELECT count(); on a pooled client it cost a reconnect on every call. Draining to completion keeps the connection. Every row now crosses the wire, so the remarks say so and point large reads at StreamAsync. The test that claimed to prove connection reuse only proved the pool could redial after a termination. Both scalar-lifetime tests now run on a session with a temporary table as the marker, which is the one thing that cannot survive a replaced connection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AddClickHouseTcpDataSource registers the data source and the client it owns, mirroring the HTTP AddClickHouseDataSource: a connection string, options, an options factory, or a data source factory, each with an optional service key. Only singletons, because the pool has to outlive every consumer. The data source hands out the client itself rather than a view that swallows disposal, so a consumer that disposes what it was injected closes the pool. The docs say so, and the pool teardown is idempotent, so the container disposing both the data source and the client at shutdown closes it once. ClickHouseTcpClient implements IDisposable alongside IAsyncDisposable so a container can dispose it under either path. A synchronous ServiceProvider.Dispose rejects a tracked service that offers only IAsyncDisposable, and rejects it instead of disposing the rest of its list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
alex-clickhouse
marked this pull request as draft
August 29, 2026 09:23
A DateTime column surfaces as IColumn<uint> and a DateTime64 as IColumn<long>, because that is the count the wire carried. Turning a count into an instant needs the timezone the column type declared, which no IColumn member reports and which the concrete column classes hold internally, so a caller on the block tier could not do it at all: only QueryAsync<T> into a POCO converted. Time and Time64 were the same, minus the timezone. IDateTimeColumn and ITimeColumn expose that conversion the way IQBitColumn and IVariantColumn already expose their layouts — a public interface over an internal column, reached by pattern-matching. Scale says which unit the raw count is in, so a caller reading Values directly knows what it has. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ITimeColumn test queried Time without enabling it. On 25.8, the floor of the CI matrix, the type is setting-gated, so the test would have failed there while passing on a local 26.6 where it has graduated. Every Time case in InsertRoundTripCase already passes both flags. With them set, toTime resolves to toTimeWithFixedDate, which takes a Date or DateTime rather than a String, so the query builds its value with a cast instead. Two calendar assertions formatted without a culture, so a non-Gregorian current culture would render a different year and fail on correct values. IDateTimeColumn.TimeZone claimed a type naming no timezone resolves to the server's. A query's session_timezone comes first. The projections also count 100 ns ticks, so scale 8 and 9 truncate; the concrete columns document that and the interfaces a consumer actually sees did not. Found by a codex review of 0d099f6. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 #591. Epic R1-R4 plus C9.
The native client's public surface is now declared, enforced by an analyzer, and fully documented. Along the way this settles the open questions about what the client should expose.
Surface decisions
InsertAsync?ClickHouseDecimalcollides with the HTTP driver's typeClickHouseTcpDecimalInt256/UInt256vs the HTTP driver'sBigIntegerClickHouseErrorCodeTcpinfix and the separate hierarchy[Experimental]scopeBlock's borrowed lifetimeClickHouse.Driver.Tcpref struct, a scoped callback and a disposable lease were all considered for makingBlock's borrowing visible in the type. Aref structcannot cross anawait, which rules outIAsyncEnumerable<Block>; the other two cost theawait foreachor add an allocation per block to turn silent corruption into a runtime throw. None earns its price.New public API
ClickHouseTcpColumn.Create(name, values)— builds the columnsInsertAsynctakes. This closes a real hole: every concrete column wasinternaland there was no factory, soInsertAsync(string, IReadOnlyList<IColumn>, ...)was public but uncallable from outside the assembly. Tests only reached it throughInternalsVisibleTo.A built column reports a null
TypeName. The insert matches columns to the target by name and takes the type from the schema the server sends, so requiring a type string from the caller would only invite a wrong answer that we then ignore.GetServerInfoAsync/ClickHouseTcpServerInfo— version, protocol revision, timezone and display name off the handshake, so callers can branch on a server version without parsingSELECT version(). Nothing exposed this before.ExecuteScalarAsync— first column of the first row. Abandons the rest of the result, so it stays cheap against a query that would return many rows.ClickHouseTcpDataSource— owns a client and hands out views whoseDisposeAsyncdoes nothing, so a scoped service that disposes what it was injected cannot take the shared pool down with it. It implementsIDisposableas well asIAsyncDisposable, because a synchronousServiceProvider.Dispose()rejects an async-only singleton.C9: one-time little-endian guard
The column codecs reinterpret wire bytes as CLR values with
MemoryMarshal.Cast, which is correct only on a little-endian host. The per-codec guards were removed as redundant; this replaces them with one check at client and connection construction. A guard, not a fallback — every runtime .NET currently targets is little-endian.The analyzer, and what enabling it uncovered
Microsoft.CodeAnalysis.PublicApiAnalyzerswas never actually wired up in this repo: no package reference, noAdditionalFiles, no CI job. ThePublicAPI/*.txtfiles have been inert, despiteAGENTS.mddescribing them as analyzer-enforced.Wiring it up surfaced why the drift went unnoticed. The analyzer only checks the surface when handed exactly one
Shipped/Unshippedpair, and both mis-configurations fail silently:AdditionalFilesgivenMeasured on this repo: deleting a public type from a single-file configuration produced no RS0016;
ClickHouse.Driveron net10.0 reports 0 RS0016 with both its pairs loaded and 228 errors with only the root pair.So
RS0048is escalated to error alongside RS0016/RS0017/RS0024 in the new project-level.editorconfigfiles. It is the only signal that the checks have been switched off.Scope: this PR wires
ClickHouse.Driver.TcpandClickHouse.Driver.Commononly. Both are genuinely in sync.ClickHouse.Driveris left exactly as it was — itsPublicAPI/net8.0/folder is a second pair, so it has tracked nothing for a long time and has roughly 230 undeclared public symbols,ClickHouseClientandIClickHouseClientamong them. SinceClickHouse.Driver/.editorconfigalready raises RS0016 to error, wiring it without declaring those first breaksrelease.ymlandexamples.yml, neither of which passesWarningLevel=0. That cleanup is filed separately.R2 and R4
R2 needed no change; the packaging was already right. Verified from the built package:
ClickHouse.Driver.Tcp.dllinlib/net8.0|net9.0|net10.0and correctly absent fromlib/net6.0, neither Tcp nor Common leaking into the nuspec dependency list, and the third-party notices shipping as content.R4 follows the HTTP driver: no
GenerateDocumentationFileanywhere. Every public TCP member is documented — verified by an ad-hoc-p:GenerateDocumentationFile=truebuild reporting zero CS1591, which required documenting 20 operators on the numeric structs. Turning generation on repo-wide is a separate task; the HTTP driver has around 540 undocumented public members.Testing
3483 pass, 1 skipped (a
QBit(Int8)case, correctly gated off against a 26.6 server).New coverage:
PublicSurfaceIntegrationTestsround-trips factory-built columns through a real insert across the fixed-width, string, nullable and jagged-array shapes, and covers name-matching, column subsets and the wrong-CLR-type error;ClickHouseTcpDataSourceIntegrationTestsproves the ownership split against a live pool;ClickHouseTcpColumnTestsandClickHouseTcpServerInfoTestscover what a server round-trip cannot reach.The analyzer gate was verified by breaking it on purpose — deleting a type from the API file produces
error RS0016— rather than by assuming a clean build meant a working check.Per the practice in this stack, one changelog fragment covers the client as a whole rather than this PR alone.
🤖 Generated with Claude Code