Skip to content

Add POCO read materialization#318

Open
alex-clickhouse wants to merge 25 commits into
mainfrom
poco-reads-final
Open

Add POCO read materialization#318
alex-clickhouse wants to merge 25 commits into
mainfrom
poco-reads-final

Conversation

@alex-clickhouse
Copy link
Copy Markdown
Collaborator

@alex-clickhouse alex-clickhouse commented May 9, 2026

Summary

Adds a streaming POCO read path for IClickHouseClient and ClickHouseDataReader, backed by a per-client PocoTypeRegistry shared with the existing binary-insert path. Strict v1 type-matching: no implicit conversions, with rich diagnostics on mismatch.

API

  • RegisterBinaryInsertType<T>() — existing, unchanged. Insert-only. Kept for backwards compatibility (to be obsoleted in a later release).
  • RegisterPocoType<T>() — new. Canonical registration: sets up both insert and read mappings. Both mappings are validated up front; if either validation throws, neither mapping is committed, so the registry is left untouched.
  • client.QueryAsync<T>(sql, parameters?, options?, ct?)IAsyncEnumerable<T>. Streams rows lazily; the underlying reader is disposed on completion, fault, or early break.
  • reader.MapTo<T>() — materializes the current row into a fresh T without advancing the reader. Pairs with Read() for ADO.NET-style consumers.

[ClickHouseColumn(Name = ...)] aliases and [ClickHouseNotMapped] exclusion behave the same as on the existing write path.

How it works

  1. Registration compiles getters (insert) and setters + a raw-newobj constructor (read) via Expression.Lambda and caches them in a per-client ConcurrentDictionary. The compiled constructor uses raw newobj, so types with required properties are supported.
  2. Headers parse when the reader opens. The first MapTo<T> (or first iteration of QueryAsync<T>) builds an ordinal-ordered binding plan from (column ordinal, property index) pairs and fail-fasts on impossible mappings: each column's FrameworkType is checked against the target property type before any rows are materialized. Polymorphic columns (FrameworkType == typeof(object) — Variant/Dynamic/JSON/Object) skip the static check.
  3. Per-row, the binding plan drives the loop: null/DBNull is gated by CanAssignNull; otherwise the compiled boxed setter is invoked. An InvalidCastException from the setter (only reachable for polymorphic columns or stale mismatches) is caught and rethrown as InvalidOperationException with column name, ClickHouse type, target property, and returned CLR type.

ClickHouseDataReader.Read() is unchanged. Existing readers keep the buffered CurrentRow available for arbitrary ordinal access alongside MapTo<T>.

Type-matching rules (strict v1)

  • Case-sensitive (StringComparer.Ordinal) column-to-property matching.
  • Missing columns in the result leave properties at their default value.
  • Extra columns in the result are ignored.
  • No implicit conversions: no widening (Int8Int32), no enum coercion (Enum8 returns string, not your CLR enum), no ClickHouseDecimaldecimal interop. Mismatches throw InvalidOperationException naming the POCO type, property, column, and returned CLR type.
  • Static mismatches fail fast at the first MapTo<T> / iteration — before any rows are read.

Limitations

  • Read registration requires a public parameterless constructor. Records with primary constructors are rejected on the read side (still fine for RegisterBinaryInsertType, which doesn't construct).
  • Init-only, read-only, and non-public-setter properties are silently skipped on the read side and keep their default value even if a matching column is present. The type still registers if at least one property has a public non-init setter.
  • required properties are supported (the compiled constructor bypasses required-member enforcement via raw newobj).
  • Per-reader bindingPlanCache is a non-concurrent Dictionary — fine because DbDataReader isn't thread-safe by spec; concurrent MapTo<T> calls on a single reader are not supported.
  • No conversion hooks in v1. Future opt-in widening / enum / decimal coercions would be additive (a PocoReadOptions flag or [ClickHouseColumn(Convert = true)] attribute), with no public-API break.

Performance

PocoReadBenchmark reads system.numbers projecting (Id, Name, Value) and compares manual GetValue row construction against QueryAsync<T> POCO materialization.

Method Count Mean Ratio Allocated
ManualGetValue 100k 104.0 ms 1.00 12.73 MB
Poco 100k 115.1 ms 1.14 12.73 MB
ManualGetValue 500k 327.0 ms 1.00 61.55 MB
Poco 500k 341.0 ms 1.05 61.55 MB

Allocations are identical — column values are boxed once during reader buffering and reused. The remaining per-row overhead is the boxed Action<T, object> setter invocation; a future generic-typed-setter pass could close it.

Run on .NET 10 / Linux Ubuntu 22.04 / Intel Core Ultra 7 255U.

Tests

Coverage spans registration edge cases (records, init-only, abstract, write-only, indexers, statics, fields, required, inheritance), the strict-v1 read matrix (alias, missing/extra, case sensitivity, nullable/non-nullable, decimal with and without UseCustomDecimals, type mismatch, unregistered, before-Read, after-end-of-stream, derived, multiple types per reader, parameters, query options), runtime skip of init-only/read-only/private-setter properties, plus review-driven additions:

  • atomicity of RegisterPocoType<T> when either side's validation throws,
  • QueryAsync<T> cancellation mid-iteration with a successful follow-up query,
  • mid-stream server error surfacing as ClickHouseServerException through QueryAsync<T> (gated on ClickHouse 25.11+),
  • full nullable-property matrix (Nullable(Int32) null, Nullable(Int32) non-null, plain Int32int?).

A new example, examples/Select/Select_005_PocoSelect.cs, demonstrates the calling shapes end-to-end.


Note

Medium Risk
New public API on the core client/reader path with strict type rules and shared registry semantics; behavior is well-tested but affects how consumers map query results and register types for insert+read together.

Overview
This PR adds POCO read materialization for query results, mirroring the existing binary-insert POCO story.

Public API: RegisterPocoType<T>() validates and registers both insert and read mappings atomically (either side failing leaves the registry unchanged). RegisterBinaryInsertType<T>() stays insert-only. ClickHouseClient.QueryAsync<T>() streams IAsyncEnumerable<T> and disposes the reader when enumeration ends. ClickHouseDataReader.MapTo<T>() maps the current row without advancing. ClickHouseConnection.RegisterPocoType<T>() forwards to the shared client registry so command readers can use MapTo<T>.

Implementation: The old BinaryInsertTypeRegistry is replaced by a per-client PocoTypeRegistry with separate insert/read mappings, compiled getters/setters/constructors, and readers receive the registry when opened from the client or ADO commands. MapTo<T> builds a per-reader binding plan (case-sensitive column names, static assignability checks, strict v1—no widening/conversions), tracks hasCurrentRow, and clears row state safely on partial read failures.

Docs/tests: v1.4.0 changelog/release notes, a POCO read benchmark, broad unit tests (registration shapes, mapping rules, atomic registration, mid-stream server errors), example Select_006_PocoSelect, and insert tests for ctor-less types vs failed RegisterPocoType.

Reviewed by Cursor Bugbot for commit 76505bd. Bugbot is set up for automated code reviews on this repo. Configure here.

Introduce QueryAsync<T> on IClickHouseClient (streaming IAsyncEnumerable<T>)
and GetRecord<T>() on ClickHouseDataReader, both backed by a per-client
PocoTypeRegistry shared with the existing binary-insert path. v1 assignment
rules are strict: no widening, no enum coercion, no ClickHouseDecimal->decimal.

API:
  - RegisterBinaryInsertType<T>() — existing, unchanged
  - RegisterPocoReadType<T>()     — read-only registration
  - RegisterPocoType<T>()         — convenience: read + insert

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 9, 2026 06:51
@alex-clickhouse alex-clickhouse requested a review from mzitnik as a code owner May 9, 2026 06:51
@alex-clickhouse alex-clickhouse marked this pull request as draft May 9, 2026 06:51
@codecov
Copy link
Copy Markdown

codecov Bot commented May 9, 2026

Codecov Report

❌ Patch coverage is 94.62366% with 10 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...ckHouse.Driver/ADO/Readers/ClickHouseDataReader.cs 88.13% 4 Missing and 3 partials ⚠️
ClickHouse.Driver/ClickHouseClient.cs 84.61% 0 Missing and 2 partials ⚠️
ClickHouse.Driver/Poco/PocoTypeRegistry.cs 99.10% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces first-class POCO read materialization to the ClickHouse .NET client: a per-client POCO registry is shared between the existing binary insert path and new read APIs, enabling streaming materialization via IAsyncEnumerable<T> and row materialization directly from ClickHouseDataReader.

Changes:

  • Added POCO read registration APIs (RegisterPocoReadType<T>(), RegisterPocoType<T>()) and a streaming query API (QueryAsync<T>()) on IClickHouseClient / ClickHouseClient.
  • Implemented a unified PocoTypeRegistry that builds insert/read mappings and is threaded through ClickHouseDataReader for GetRecord<T>().
  • Added extensive NUnit coverage for registration and read materialization behavior, plus a benchmark, and updated public API + release notes/changelog.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
RELEASENOTES.md Documents the new POCO read feature set and API surface.
CHANGELOG.md Adds a v1.4.0 entry describing POCO read support and behavior.
ClickHouse.Driver/PublicAPI/PublicAPI.Unshipped.txt Declares newly added public APIs for compatibility tracking.
ClickHouse.Driver/IClickHouseClient.cs Extends the public client interface with POCO read registration and QueryAsync<T>().
ClickHouse.Driver/ClickHouseClient.cs Implements the new registry-backed registration methods and QueryAsync<T>(); passes registry into readers.
ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs Adds GetRecord<T>() materialization and binding plan caching; stores the per-client registry.
ClickHouse.Driver/ADO/ClickHouseConnection.cs Exposes POCO read registration passthroughs for ADO.NET users.
ClickHouse.Driver/ADO/ClickHouseCommand.cs Ensures commands create readers using the client’s shared POCO registry.
ClickHouse.Driver/Copy/PocoTypeRegistry.cs New unified per-client registry building insert/read mappings and compiled delegates.
ClickHouse.Driver/Copy/PocoMappings.cs New mapping model types for insert/read paths.
ClickHouse.Driver/Copy/BinaryInsertPropertyInfo.cs Extends cached property metadata to support both insert and read scenarios.
ClickHouse.Driver/Copy/BinaryInsertTypeRegistry.cs Removed in favor of PocoTypeRegistry.
ClickHouse.Driver/Copy/BinaryInsertTypeMapping.cs Removed in favor of split insert/read mapping types.
ClickHouse.Driver.Tests/Copy/PocoRegistrationTests.cs Adds unit tests for POCO registration validation rules.
ClickHouse.Driver.Tests/Copy/PocoReadTests.cs Adds end-to-end tests for GetRecord<T>() and QueryAsync<T>().
ClickHouse.Driver.Tests/Copy/PocoEdgeCaseTests.cs Adds extensive edge-case coverage for supported/unsupported POCO shapes.
ClickHouse.Driver.Benchmark/PocoReadBenchmark.cs Adds a benchmark comparing POCO materialization vs manual GetValue.

Comment thread ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs Outdated
Comment thread ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs
Comment thread ClickHouse.Driver/IClickHouseClient.cs
Comment thread ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs
Comment thread RELEASENOTES.md Outdated
Comment thread CHANGELOG.md Outdated
alex-clickhouse and others added 5 commits May 9, 2026 09:39
- Validate-once: column FrameworkType vs property type is checked at plan
  build (fail-fast for impossible mappings before any rows are read).
- Per-row: drop IsAssignableFrom; rely on the compiled setter, catching
  InvalidCastException to rethrow with rich diagnostics. Trims the per-row
  overhead vs. manual GetValue from ~25% to ~5% on 500k rows; allocations
  unchanged.
- Rename ClickHouseDataReader.GetRecord<T>() to MapTo<T>() to avoid
  confusion with C# `record` types and to read more naturally next to
  Read() ("Read advances; MapTo materializes the current row").
- Doc fixes per review: clarify that read materialization requires
  RegisterPocoReadType<T>() or RegisterPocoType<T>() (not
  RegisterBinaryInsertType<T>()), correct the `required` properties claim
  (they are supported), and split the CHANGELOG/RELEASENOTES entries from
  a wall of text into scannable sub-bullets.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- RegisterPocoType<T> is now atomic: read registration runs first (its
  rules are stricter), insert only commits if read succeeds. A thrown
  exception leaves the registry untouched.
- Fix the misleading XML doc on IClickHouseClient.RegisterBinaryInsertType
  to clarify that only the insert side is registered.
- Defensively clear hasCurrentRow before the per-column Read loop so a
  mid-row throw cannot leave a stale CurrentRow visible to MapTo<T>.
- Document why QueryAsync<T> uses sync reader.Read() inside the async
  iterator (no async overload; underlying stream is buffered).
- Add tests: atomicity post-failure, QueryAsync cancellation
  mid-iteration with follow-up query, server-side error mid-stream
  surfaces ClickHouseServerException through QueryAsync.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Demonstrates the new POCO read APIs:
  - QueryAsync<T> streaming via IAsyncEnumerable<T>
  - [ClickHouseColumn(Name)] aliases and [ClickHouseNotMapped] on the read
    path (same semantics as on the write path)
  - ClickHouseDataReader.MapTo<T> on the current row alongside typed
    accessors, for ADO.NET-style consumers

Wired into Program.cs in the SELECTING DATA section.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Single query exercises all three rows of the nullable-property matrix:
Nullable(Int32) NULL, Nullable(Int32) non-null, and plain Int32 — each
mapped to a separate int? property. Closes the gap where existing tests
covered only the NULL and plain cases but not Nullable(Int32) non-null.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the existing null checks on httpResponse and reader. Internal-only
call sites all pass a non-null instance today; this is for defensive
symmetry, not a correctness fix.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 19 out of 19 changed files in this pull request and generated 4 comments.

Comment thread ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs Outdated
Comment thread ClickHouse.Driver/ClickHouseClient.cs Outdated
Comment thread ClickHouse.Driver/IClickHouseClient.cs Outdated
Comment thread ClickHouse.Driver/ADO/ClickHouseConnection.cs Outdated
alex-clickhouse and others added 14 commits May 9, 2026 10:48
- XML doc on RegisterPocoReadType<T> and RegisterPocoType<T> now spells
  out that init-only, read-only, and non-public-setter properties are
  silently skipped on the read path and keep their default value even
  when a matching result column is present.
- New runtime test pins the guarantee: a POCO with one mapped property
  alongside init-only, read-only, and private-setter properties is
  materialized from a result that contains all four columns; only the
  set; property is filled, the others stay at their CLR default.
- Drop the redundant `Copy.` qualifier in ClickHouseDataReader (the
  using directive already imports ClickHouse.Driver.Copy).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move the POCO-only types (PocoMappings, PocoTypeRegistry, and the
property-info struct) out of ClickHouse.Driver.Copy — which historically
held bulk-copy machinery — into a dedicated ClickHouse.Driver.Poco
namespace and folder. Bulk-copy serializers stay in Copy/. Also rename
the BinaryInsertPropertyInfo type to PocoPropertyInfo now that it's
shared by both the insert and read paths and lives under Poco/, so the
old "BinaryInsert" prefix would be misleading.

Internal-only refactor; no public API change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Order alone wasn't enough: a type can pass read validation but fail
insert validation (e.g., a property with a private getter is insert-
unmappable but read-mappable). The previous implementation would have
left a tentative read mapping behind when insert later threw.

PocoTypeRegistry.RegisterForBoth<T>() now builds both mappings up front
and only commits them after both pass validation, so a thrown exception
cannot leave a partial registration behind regardless of which side
fails.

Add a test for the read-passes-insert-fails direction (the new path) to
sit alongside the existing test for the read-fails direction.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Direct happy-path coverage was missing — every existing read-side test
registered through the RegisterPocoType convenience. Pin that read-only
registration is sufficient on its own: a fresh type is registered via
RegisterPocoReadType<T>(), QueryAsync<T> materializes its rows, and the
type is *not* registered for binary insert.

Uses a dedicated POCO so the "no insert mapping" assertion isn't
polluted by other tests in the shared fixture that go through
RegisterPocoType.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The new public surface is just two registration methods:

  - RegisterBinaryInsertType<T>()  — existing, insert-only, kept for
    backwards compatibility (will be obsoleted later).
  - RegisterPocoType<T>()          — canonical: sets up both insert and
    read mappings.

The interim RegisterPocoReadType<T>() is gone. We didn't need a separate
read-only path on the public surface: RegisterPocoType validates both
mappings up front, so registering a read-mappable type that isn't
insert-mappable still fails fast. If a future use case really requires
read-only registration, we can add it back without breaking anyone.

Internal-only changes:
  - PocoTypeRegistry.RegisterForRead<T> removed (dead code).
  - Tests previously calling RegisterPocoReadType now call RegisterPocoType.
  - Redundant per-side tests dropped (the convenience-method test
    already covers the same case).
  - Reader's "not registered" error message simplified to point only at
    RegisterPocoType<T>().

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three tests previously only asserted that RegisterPocoType / Register-
BinaryInsertType did not throw — they did not verify the behavior they
claimed to test.

  - NotMappedProperty:    only checked registration didn't throw; did
    not verify the property was actually excluded from either mapping.
    Now inspects insertMapping.Properties and readMapping.ColumnName-
    ToPropertyIndex to confirm the property is absent from both sides.

  - IndexerProperty:      only checked registration didn't throw; did
    not verify the indexer was excluded. Now inspects both mappings
    and asserts each holds exactly the non-indexer Id property.

  - RegisterBinaryInsertType_TypeWithoutParameterlessConstructor:
    only checked registration didn't throw; did not verify an insert
    mapping was actually produced or that the read side stayed
    unregistered. Now inspects both.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The prior pass over PocoRegistrationTests inspected internal mappings
(Properties arrays, ColumnNameToPropertyIndex lookup, GetInsertMapping
returning non-null) — useful for finding the mechanism, but not what
users actually see. Replace those with integration tests that prove the
same behavior through the public surface against a real ClickHouse.

PocoRegistrationTests.cs now only holds the four registration-time
validation tests that genuinely don't need a DB (no mapped properties,
duplicate column names, whitespace column name, no parameterless ctor).
Everything else moves to a fixture that round-trips through ClickHouse.

New integration coverage:

  PocoReadEdgeCaseTests (PocoEdgeCaseTests.cs):
    - MapTo_NotMappedProperty_StaysAtDefault: a result row that has the
      [ClickHouseNotMapped] column name still leaves the property at
      its CLR default after MapTo<T>.
    - MapTo_PocoWithIndexer_MaterializesProperties: indexers don't
      interfere with registration or materialization.
    - RegisterPocoType_FailedInsertValidation_LeavesReadUnregistered_-
      QueryAsyncThrows: atomicity proven through QueryAsync<T>.

  InsertBinaryPocoTests:
    - InsertBinaryAsync_NoParameterlessConstructor_InsertsSuccessfully:
      end-to-end round-trip of an insert from a POCO whose only ctor
      takes parameters.
    - RegisterPocoType_FailedReadValidation_LeavesInsertUnregistered_-
      InsertBinaryThrows: atomicity proven through InsertBinaryAsync.

Atomicity tests use dedicated POCOs to avoid contamination from the
shared per-fixture client.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The ClickHouse.Driver.Poco using directive was placed where it landed
during the namespace extraction, not in its alphabetical slot.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 2-arg FromHttpResponseAsync overload is only used by tests that
exercise mid-stream exception handling — they have no POCO mapping to
register. Allocating an empty PocoTypeRegistry there was pure overhead
that surfaced on every call.

Pass null through instead and let MapTo<T> null-conditional its way to
the standard "not registered" InvalidOperationException. The ctor
ArgumentNullException on pocoRegistry is replaced with a comment so
the null contract is explicit at the field.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The read mapping previously carried three parallel structures: a
Properties[] array of metadata, a Setters[] array of compiled
delegates, and a ColumnNameToPropertyIndex dict mapping column names
into both. The split was carryover from the insert side, where Properties
and Getters are iterated in lockstep; on the read side we look up by
column name and never iterate, so the index level was dead weight.

Replace with a single IReadOnlyDictionary<string, ColumnBinding<T>> on
PocoReadMapping<T>, where ColumnBinding<T> bundles the PocoPropertyInfo
and the compiled Action<T, object> setter. One dict hit per column at
plan-build, then field access (no further indirection) in the per-row
loop in MapTo<T>.

The non-generic PocoReadMapping base is now an empty marker — kept only
so the registry can store mappings keyed by Type in a non-generic
ConcurrentDictionary. Insert side is unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two fixtures lived in PocoEdgeCaseTests.cs:

  - PocoEdgeCaseTests          — unit-style POCO-shape registration smokes
  - PocoReadEdgeCaseTests      — integration MapTo / QueryAsync edge cases

Their content fits naturally in the two existing per-side integration
fixtures. Fold them so each kind of test lives next to its peers:

  - PocoReadTests gains the read-side RegisterPocoType<T> throws tests
    (positional records, all-init-only, all-private-setter, all-readonly,
    abstract, fields-only, static-only, write-only) and every MapTo /
    QueryAsync edge case (NotMappedProperty, indexer, init/readonly/
    private-setter skip, widening throw, object property, derived,
    multiple types per reader, before-Read, after-end-of-stream,
    required members, decimal-without-custom-decimals, unregistered-on-
    first-yield, cancellation mid-iteration, server error mid-stream,
    failed-insert-rollback proven via QueryAsync).
  - InsertBinaryPocoTests gains the RegisterBinaryInsertType<T>
    registration smokes for shapes that are insert-mappable but not
    read-mappable (positional records, init-only, private-setter,
    read-only, abstract).

Five tests dropped as redundant with the integration coverage that's
now in the same file: RecordWithMixedAccessors / PrivateSetter /
MixedReadOnlyAndReadWrite registration smokes (all covered by the
runtime MapTo skip test), DerivedType registration smoke (covered by
the MapTo derived test), and RequiredMembers registration smoke
(covered by the MapTo required-members test).

PocoEdgeCaseTests.cs is removed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous fold dropped registration-validation tests into PocoReadTests
and InsertBinaryPocoTests next to their respective integration peers, but
those tests don't actually need a DB — they exercise pure client-side
validation on RegisterPocoType<T> / RegisterBinaryInsertType<T>. The right
home is PocoRegistrationTests, the existing no-DB unit fixture.

Final layout:
  - PocoRegistrationTests:   pure client-side validation (no DB).
  - PocoReadTests:           runtime MapTo / QueryAsync (DB required).
  - InsertBinaryPocoTests:   runtime InsertBinaryAsync (DB required).

The shared POCO shapes (PositionalRecord, RecordWithInitProperties,
InitOnlyOnly, AllPrivateSettersPoco, ReadOnlyOnlyPoco, AbstractPoco) are
now defined once in PocoRegistrationTests instead of duplicated between
the two integration fixtures.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@alex-clickhouse alex-clickhouse requested a review from Copilot May 12, 2026 11:13
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 20 out of 20 changed files in this pull request and generated 4 comments.

Comment thread ClickHouse.Driver/Poco/PocoTypeRegistry.cs
Comment thread ClickHouse.Driver/Poco/PocoTypeRegistry.cs
Comment thread ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs Outdated
Comment thread examples/Program.cs
alex-clickhouse and others added 4 commits May 12, 2026 13:43
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
BuildReadMapping<T> only checked that GetConstructor(Type.EmptyTypes)
returned non-null. That works for an abstract class without an explicit
public parameterless ctor (the implicit one is `family`/protected and
GetConstructor with default flags returns null), but it does *not* work
for an abstract class that declares its own public parameterless ctor —
GetConstructor returns it, registration succeeds, and the failure is
deferred to the first MapTo<T> call, where Expression.New on the
compiled constructor delegate throws at instantiation time.

Reject Type.IsAbstract at registration time. IsAbstract is true for both
abstract classes and interfaces, so the single check covers both. The
existing abstract-class test (no explicit ctor) and two new tests
(abstract class with public parameterless ctor; interface) all assert
the new "abstract" error message.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@alex-clickhouse alex-clickhouse marked this pull request as ready for review May 12, 2026 11:49
@alex-clickhouse alex-clickhouse enabled auto-merge (squash) May 12, 2026 11:50
/// skipped — they keep their default value even if a matching result column is present.
/// </summary>
/// <typeparam name="T">The POCO type to register.</typeparam>
void RegisterPocoType<T>()
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.

Small thing - since this adds members to IClickHouseClient, downstream implementors will need to add the new members on upgrade, which is source-breaking for them (eg. custom wrappers for logging/retry/routing).

Could you potentially add something along the lines of:

void RegisterPocoType<T>() where T : class =>
    throw new NotSupportedException(
        $"{GetType().Name} does not implement RegisterPocoType<T>. " +
        "Added in v1.4.0 — update your implementation or use ClickHouseClient.");

Existing implementations keep compiling and wrappers get a clear runtime error until they upgrade.

/// materialized via <see cref="QueryAsync{T}"/> / <see cref="ClickHouseDataReader.MapTo{T}"/>.
/// Both mappings are validated up front; if either validation throws, neither mapping is
/// committed and the registry is left untouched.
/// On the read side, init-only, read-only, and non-public-setter properties are silently
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.

Worth considering whether this should be opt-in rather than the default. The 'silent skip' failure mode is hard to diagnose; registration succeeds, queries run, and every property comes back at its CLR default with no exception, no log, nothing to help diagnose.

Could RegisterPocoType fail registration if the POCO has read-unmappable properties (init-only, read-only, non-public-setter) by default, with a PocoReadOptions type with a bool opt-out for users who genuinely want the current skip behaviour?

# Conflicts:
#	examples/Program.cs
#	examples/README.md
@github-actions
Copy link
Copy Markdown

Triage

Category: featureRisk: high

Summary
This PR adds a full POCO read materialization path that mirrors the existing binary-insert story. It introduces RegisterPocoType<T>() (atomically registers insert + read, validating both up front) on IClickHouseClient and ClickHouseConnection; ClickHouseClient.QueryAsync<T>() returning a lazily-streaming IAsyncEnumerable<T> with reader disposal on completion/fault/break; and ClickHouseDataReader.MapTo<T>() for ADO.NET-style per-row materialization. Internally, the old Copy/BinaryInsertTypeRegistry (plus BinaryInsertTypeMapping.cs and BinaryInsertPropertyInfo.cs) is deleted and replaced by a new Poco/PocoTypeRegistry that owns both insert and read mappings. The registry is threaded through ClickHouseCommand.ExecuteDbDataReaderAsync so command readers can also call MapTo<T>. ClickHouseDataReader.Read() is modified to track hasCurrentRow state. The PR includes a benchmark, broad unit tests, CHANGELOG/RELEASENOTES entries, and a new example.

What this impacts

  • IClickHouseClient / ClickHouseClient (Utility/) — new public API: RegisterPocoType<T>, QueryAsync<T>
  • ADO/ClickHouseDataReader — new public API: MapTo<T>; hot-path state change in Read(); non-concurrent bindingPlanCache per reader
  • ADO/ClickHouseCommand, ADO/ClickHouseConnection — registry wired in; new public RegisterPocoType<T> on connection
  • Copy/ subsystem replaced by Poco/ — internal refactor affecting the binary-insert path via renamed types (PocoInsertMapping, PocoPropertyInfo)
  • PublicAPI/PublicAPI.Unshipped.txt — 4 new entries; both ClickHouseClient and ClickHouseConnection users, plus any ADO.NET consumer using ClickHouseDataReader directly

Concerns

  • Risk: high — cross-module refactor: ADO/ (ClickHouseDataReader, ClickHouseCommand, ClickHouseConnection), Utility/ (ClickHouseClient), Copy/ (three files deleted), new Poco/ directory — four of the named high-risk modules move together.
  • Risk: high — public API surface: Four new entries on consumer-facing types (IClickHouseClient, ClickHouseConnection, ClickHouseDataReader) added to PublicAPI.Unshipped.txt; correctly tracked, but warrants human sign-off on the shape.
  • Risk: high — hot-path state change in Read(): hasCurrentRow is cleared before the per-column deserialization loop and set true only on success. Any mid-row EndOfStreamException (or future exception) now leaves hasCurrentRow = false. This is intentionally defensive but changes observable state on exception paths that existing consumers might currently catch-and-continue.

Required reviewer action

  • High: PR body must include an architectural description before review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants