From 5a3bb054fa294515d3b12308112daeaea8e78514 Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Sun, 2 Aug 2026 12:14:35 +0200 Subject: [PATCH 01/16] feat(poco): read wire-transparent wrapper types on the box-free fast path SimpleAggregateFunction and Object are pass-through on the RowBinary wire in exactly the way LowCardinality is: Read/Write delegate to the wrapped type and FrameworkType reports the wrapped type's. But PocoReadExpressionFactory only special-cased LowCardinality, so a SimpleAggregateFunction(sum, UInt64) column bound to a ulong property silently fell off the typed read path onto the boxed one, even though it decodes byte-for-byte identically to a bare UInt64. Extract the unwrap into TransparentWrapper so the rule lives in one place and loops, which also handles nesting (LowCardinality over SimpleAggregateFunction and vice versa). Nullable is deliberately excluded: it prefixes a marker byte and so is not wire-transparent. Also correct AGENTS.md's claim that a Roslyn analyzer enforces PublicAPI/*.txt. Microsoft.CodeAnalysis.PublicApiAnalyzers is not referenced anywhere in the solution; those files are hand-maintained. Co-Authored-By: Claude --- AGENTS.md | 8 ++-- .../Copy/PocoReadFastPathParityTests.cs | 32 ++++++++++++++-- .../Copy/PocoReadFastPathTests.cs | 29 +++++++++++++++ .../Poco/PocoReadExpressionFactory.cs | 13 ++++--- ClickHouse.Driver/Types/TransparentWrapper.cs | 37 +++++++++++++++++++ 5 files changed, 106 insertions(+), 13 deletions(-) create mode 100644 ClickHouse.Driver/Types/TransparentWrapper.cs diff --git a/AGENTS.md b/AGENTS.md index bb64ae649..ea4b1b36c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -22,7 +22,7 @@ ClickHouse.Driver.sln │ ├── Types/ # 60+ ClickHouse type implementations + TypeConverter.cs │ ├── Copy/ # Binary serialization (used internally by ClickHouseClient) │ ├── Http/ # HTTP layer & connection pooling -│ └── PublicAPI/ # Public API surface tracking (analyzer-enforced) +│ └── PublicAPI/ # Public API surface tracking (hand-maintained) ├── ClickHouse.Driver.Tests/ # NUnit tests (multi-framework) ├── ClickHouse.Driver.IntegrationTests/ # Integration tests (net10.0) └── ClickHouse.Driver.Benchmark/ # BenchmarkDotNet performance tests @@ -35,7 +35,9 @@ Prefer using LSP to grep when navigating the codebase. - **Type system**: `Types/TypeConverter.cs` (14KB, complex), `Types/Grammar/` (type parsing) - **ADO.NET layer**: `ADO/ClickHouseConnection.cs`, `ADO/ClickHouseCommand.cs`, `ADO/Readers/` - **Feature detection**: `Utility/ClickHouseFeatureMap.cs` (version-based capabilities) -- **Public API**: `PublicAPI/*.txt` (Roslyn analyzer enforces shipped signatures) +- **Public API**: `PublicAPI/*.txt` (hand-maintained record of shipped signatures; the + `Microsoft.CodeAnalysis.PublicApiAnalyzers` package is *not* referenced, so nothing checks these + files at build time — keep them in sync yourself) - **Config**: `.editorconfig` (file-scoped namespaces, StyleCop suppressions) ### API Architecture @@ -241,7 +243,7 @@ If the value is null/`DBNull` and no explicit type or hint is provided, resoluti - **Connection state**: Clear logging of connection lifecycle events ### Public API Surface -- **Breaking changes**: Must update `PublicAPI/*.txt` files (analyzer enforces) +- **Breaking changes**: Must update `PublicAPI/*.txt` files (by hand — no analyzer enforces this) - **ADO.NET compliance**: Follow ADO.NET patterns and interfaces correctly - **Dispose patterns**: Proper `IDisposable` implementation, no resource leaks diff --git a/ClickHouse.Driver.Tests/Copy/PocoReadFastPathParityTests.cs b/ClickHouse.Driver.Tests/Copy/PocoReadFastPathParityTests.cs index 7f2f7b16b..1a676c62f 100644 --- a/ClickHouse.Driver.Tests/Copy/PocoReadFastPathParityTests.cs +++ b/ClickHouse.Driver.Tests/Copy/PocoReadFastPathParityTests.cs @@ -356,11 +356,35 @@ public void TryBuildReadBody_MismatchedTargetType_ReturnsNull(string typeName, T public void TryBuildReadBody_CompositeColumn_ReturnsNull(string typeName, Type target) => AssertNoFastPath(typeName, target); - // SimpleAggregateFunction is wire-transparent like LowCardinality, but the factory does not unwrap it, - // so it currently falls back to the boxed path. Pins today's behaviour; see the note in the PR. + // SimpleAggregateFunction is wire-transparent like LowCardinality, so it must reach the same typed + // reader the bare column would. Nesting checks the unwrap loop rather than a single-level special case. + [TestCase("SimpleAggregateFunction(sum, UInt64)", typeof(ulong))] + [TestCase("SimpleAggregateFunction(any, LowCardinality(String))", typeof(string))] + [TestCase("LowCardinality(SimpleAggregateFunction(any, String))", typeof(byte[]))] + public void TryBuildReadBody_WireTransparentWrapper_ResolvesToWrappedTypedRead(string typeName, Type target) + { + var body = TryBuild(Parse(typeName), target); + Assert.That(body, Is.Not.Null, $"expected a fast path for {typeName} -> {target}"); + Assert.That(body.Type, Is.EqualTo(target)); + } + + // The wrapper is transparent, not permissive: unwrapping must not widen the target-type match either. [Test] - public void TryBuildReadBody_SimpleAggregateFunctionColumn_ReturnsNull() - => AssertNoFastPath("SimpleAggregateFunction(sum, UInt64)", typeof(ulong)); + public void TryBuildReadBody_WireTransparentWrapperWithMismatchedTarget_ReturnsNull() + => AssertNoFastPath("SimpleAggregateFunction(sum, UInt64)", typeof(long)); + + // SimpleAggregateFunction(any, Nullable(T)) still has to go through the null-marker read. + [Test] + public void TryBuildReadBody_WireTransparentWrapperOverNullable_ReadsNullMarker() + { + var type = Parse("SimpleAggregateFunction(any, Nullable(Int32))"); + Assert.Multiple(() => + { + Assert.That(TryBuild(type, typeof(int?))?.Type, Is.EqualTo(typeof(int?))); + // A non-nullable target on a nullable column cannot represent the null and must decline. + Assert.That(TryBuild(type, typeof(int)), Is.Null); + }); + } private static void AssertNoFastPath(string typeName, Type targetClrType) { diff --git a/ClickHouse.Driver.Tests/Copy/PocoReadFastPathTests.cs b/ClickHouse.Driver.Tests/Copy/PocoReadFastPathTests.cs index 56f750cf4..64e12e7e0 100644 --- a/ClickHouse.Driver.Tests/Copy/PocoReadFastPathTests.cs +++ b/ClickHouse.Driver.Tests/Copy/PocoReadFastPathTests.cs @@ -390,6 +390,35 @@ public async Task QueryAsync_LowCardinalityColumn_ReadsUnderlyingBoxFree() Assert.That(rows[7].Name, Is.EqualTo("n7")); } + public class AggregatePoco + { + public string Name { get; set; } + public ulong Total { get; set; } + } + + [Test] + public async Task QueryAsync_SimpleAggregateFunctionColumn_ReadsUnderlyingBoxFree() + { + client.RegisterPocoType(); + var table = CreateTableName(); + await client.ExecuteNonQueryAsync( + $"CREATE TABLE {table} (Name String, Total SimpleAggregateFunction(sum, UInt64)) " + + "ENGINE AggregatingMergeTree ORDER BY Name"); + await client.ExecuteNonQueryAsync($"INSERT INTO {table} VALUES ('a', 3), ('a', 4), ('b', 10)"); + + var sql = $"SELECT Name, sum(Total) AS Total FROM {table} GROUP BY Name ORDER BY Name"; + + using (var reader = (ClickHouseDataReader)await client.ExecuteReaderAsync($"SELECT Name, Total FROM {table}")) + Assert.That(reader.TryGetRowMaterializer(out _, out _), Is.True, + "SimpleAggregateFunction is wire-transparent and must reach the wrapped typed reader"); + + var rows = new List(); + await foreach (var row in client.QueryAsync(sql)) + rows.Add(row); + + Assert.That(rows.Select(r => (r.Name, r.Total)), Is.EqualTo(new[] { ("a", 7ul), ("b", 10ul) })); + } + public class NullablePropPoco { public long? Id { get; set; } diff --git a/ClickHouse.Driver/Poco/PocoReadExpressionFactory.cs b/ClickHouse.Driver/Poco/PocoReadExpressionFactory.cs index 5addf4600..688dcfc1b 100644 --- a/ClickHouse.Driver/Poco/PocoReadExpressionFactory.cs +++ b/ClickHouse.Driver/Poco/PocoReadExpressionFactory.cs @@ -18,8 +18,9 @@ namespace ClickHouse.Driver.Poco; /// /// Dispatch is driven by : a column type takes the fast path for a property CLR /// type iff it implements ITypedReader<thatType>, which also lets one column offer several -/// representations. and are transparent. Anything -/// with no typed reader returns null, and the caller falls back to the boxed path for that column. +/// representations. and the wire-transparent wrappers (see +/// ) are handled transparently. Anything with no typed reader returns +/// null, and the caller falls back to the boxed path for that column. /// internal static class PocoReadExpressionFactory { @@ -42,10 +43,10 @@ internal static class PocoReadExpressionFactory /// The bound property's CLR type; the returned expression has this type. public static Expression TryBuildReadBody(ClickHouseType type, Expression reader, Type targetClrType) { - // LowCardinality is transparent on the RowBinary wire (LowCardinalityType.Read delegates to the - // underlying), so read the underlying value directly. - if (type is LowCardinalityType lowCardinality) - return TryBuildReadBody(lowCardinality.UnderlyingType, reader, targetClrType); + // LowCardinality / SimpleAggregateFunction / Object are transparent on the RowBinary wire (their + // Read delegates to the wrapped type), so read the wrapped value directly. Without this a wrapped + // column would silently fall back to the boxed path despite decoding identically to a bare one. + type = TransparentWrapper.Unwrap(type); if (type is NullableType nullableType) return TryBuildNullableRead(nullableType, reader, targetClrType); diff --git a/ClickHouse.Driver/Types/TransparentWrapper.cs b/ClickHouse.Driver/Types/TransparentWrapper.cs new file mode 100644 index 000000000..b3c03eb16 --- /dev/null +++ b/ClickHouse.Driver/Types/TransparentWrapper.cs @@ -0,0 +1,37 @@ +namespace ClickHouse.Driver.Types; + +/// +/// Column types that are pass-through on the RowBinary wire: their +/// and +/// delegate straight to the +/// wrapped type, and they report the wrapped type's . +/// +/// Any fast path that dispatches on the concrete column type has to look through these, or a wrapped +/// column silently falls back to the boxed path even though it decodes identically to a bare one. +/// +internal static class TransparentWrapper +{ + /// + /// Returns the innermost non-wrapper type, or itself if it is not a wrapper. + /// is deliberately not unwrapped: it prefixes a null marker byte and + /// so is not wire-transparent. + /// + public static ClickHouseType Unwrap(ClickHouseType type) + { + while (true) + { + var inner = type switch + { + LowCardinalityType lowCardinality => lowCardinality.UnderlyingType, + SimpleAggregateFunctionType simpleAggregate => simpleAggregate.UnderlyingType, + ObjectType obj => obj.UnderlyingType, + _ => null, + }; + + if (inner is null) + return type; + + type = inner; + } + } +} From 1ca63c964b3d73a790a29be42f65d4900b81dd26 Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Sun, 2 Aug 2026 12:28:17 +0200 Subject: [PATCH 02/16] perf(ado): decode rows into typed column slots instead of a boxed object[] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ClickHouseDataReader.Read() decoded every column via ClickHouseType.Read, which returns object, and stored the result in a shared object[] row buffer. That boxed every value-type cell once per value per row, eagerly, whether or not the caller ever asked for that column: a 500k x 10 query allocated 145 MB / 304 B per row, and the figure was identical whether the caller read all ten columns, two, or none. Replace the buffer with one ColumnSlot per wire column, allocated once per reader and overwritten in place. A column gets a typed slot iff its type implements ITypedReader — iff it can decode straight into the CLR type its boxed Read would have returned. Everything else keeps a BoxedSlot that calls the same Read as before. This commit is deliberately behaviour-only-neutral: every accessor still boxes via GetBoxed(), so the win so far is that the box is lazy and per-call rather than eager and per-row. Untyped callers that read every column (Dapper) pay the same allocation as before; ones that project a subset stop paying for what they skip. GetFieldValue and the typed accessors are de-boxed in the next commits. IsDBNull now asks the slot, so a null check neither materializes nor boxes a value. It still never invokes a configured IReadValueConverter. The one observable difference: GetValue boxes per call, so two GetValue(i) calls on the same value-type cell return two distinct boxes. They still compare equal by Equals — only ReferenceEquals changes. Slots are built from the resolved type instance, never a per-query-shape cache, so ReadStringsAsByteArrays and UseBigDecimal are handled without any cache-key hazard. The factory pre-filters on the concrete ClickHouseType class so a composite column's FrameworkType (long[], Dictionary, ...) cannot grow the constructor cache without bound. protected object[] CurrentRow is removed. It is absent from every PublicAPI txt file, and ClickHouseDataReader's only constructor is private, so no external assembly can derive from it and observe the member. Verified behaviour-neutral: the net9.0 suite reports 9714 passed / 142 skipped / 0 failed both before and after. ColumnSlotTests adds 140 more, the core of which is a differential parity check — for every ITypedReader shape, bare and under Nullable in both states, the slot's GetBoxed() must equal the boxed Read's result in value and CLR type, and consume the same bytes (checked with a trailing sentinel). Co-Authored-By: Claude --- .../ADO/ColumnSlotTests.cs | 286 ++++++++++++++++++ .../ADO/Readers/ClickHouseDataReader.cs | 60 ++-- ClickHouse.Driver/ADO/Readers/ColumnSlot.cs | 120 ++++++++ .../ADO/Readers/ColumnSlotFactory.cs | 101 +++++++ 4 files changed, 539 insertions(+), 28 deletions(-) create mode 100644 ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs create mode 100644 ClickHouse.Driver/ADO/Readers/ColumnSlot.cs create mode 100644 ClickHouse.Driver/ADO/Readers/ColumnSlotFactory.cs diff --git a/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs b/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs new file mode 100644 index 000000000..be7259c78 --- /dev/null +++ b/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs @@ -0,0 +1,286 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Net; +using System.Numerics; +using ClickHouse.Driver.ADO.Readers; +using ClickHouse.Driver.Formats; +using ClickHouse.Driver.Numerics; +using ClickHouse.Driver.Types; + +namespace ClickHouse.Driver.Tests.ADO; + +/// +/// Server-free tests for the typed column slots that replaced ClickHouseDataReader's shared +/// object[] row buffer. +/// +/// The whole design rests on one invariant: a slot must be observationally identical to the +/// boxed path it replaced. must consume exactly the bytes +/// would have, and +/// must return exactly the value it would have returned — same CLR type, and for a +/// NULL. asserts precisely that, differentially, +/// against the real boxed reader rather than against hand-written expectations. +/// +/// The rest pins the things parity alone cannot see: which slot kind a column resolves to (a +/// silent demotion to is invisible — values stay correct, only the allocation +/// disappears), and , which has no boxed counterpart to compare against. +/// +[TestFixture] +public class ColumnSlotTests +{ + // Written after the value under test; a slot that consumes the wrong number of bytes decodes garbage here + // rather than silently corrupting the next column of a real row. + private const long Sentinel = 0x1234_5678_09AB_CDEFL; + + private const string Enum8Def = "Enum8('a' = -5, 'b' = 7)"; + + private static ClickHouseType Parse(string typeName, TypeSettings? settings = null) + => TypeConverter.ParseClickHouseType(typeName, settings ?? TypeSettings.Default); + + private static byte[] Write(ClickHouseType type, object value) + { + using var stream = new MemoryStream(); + using var writer = new ExtendedBinaryWriter(stream); + type.Write(writer, value); + new Int64Type().Write(writer, Sentinel); + writer.Flush(); + return stream.ToArray(); + } + + private static (object Value, long Trailer) ReadBoxed(ClickHouseType type, byte[] payload) + { + using var stream = new MemoryStream(payload); + using var reader = new ExtendedBinaryReader(stream); + return (type.Read(reader), reader.ReadInt64()); + } + + private static (object Value, long Trailer, ColumnSlot Slot) ReadSlot(ClickHouseType type, byte[] payload) + { + var slot = ColumnSlotFactory.Create(type); + using var stream = new MemoryStream(payload); + using var reader = new ExtendedBinaryReader(stream); + slot.Read(reader); + return (slot.GetBoxed(), reader.ReadInt64(), slot); + } + + // ---- Differential parity against the boxed reader ---- + + // (type, value) pairs covering every ITypedReader shape a slot can be built over, both bare and under + // Nullable, plus the composites that must fall back. The value is written by the type's own Write, so the + // payload is exactly what the server would send. + private static IEnumerable ParityCases() + { + var settingsByName = new Dictionary + { + ["ReadAsBytes"] = TypeSettings.Default with { readStringsAsByteArrays = true }, + ["BigDecimal"] = TypeSettings.Default with { useBigDecimal = true }, + }; + + foreach (var (typeName, value) in Values()) + { + yield return Case(typeName, value, TypeSettings.Default, string.Empty); + + // Nullable(X) in both states. Composites are covered bare; Nullable(Array(..)) etc. have no typed + // slot anyway and the bare case already pins that. + if (!typeName.StartsWith("Array(", StringComparison.Ordinal) && + !typeName.StartsWith("Tuple(", StringComparison.Ordinal) && + !typeName.StartsWith("Map(", StringComparison.Ordinal)) + { + yield return Case($"Nullable({typeName})", value, TypeSettings.Default, "Present"); + yield return Case($"Nullable({typeName})", DBNull.Value, TypeSettings.Default, "Null"); + } + } + + // The two settings that change FrameworkType, and so which typed reader a slot binds to. + yield return Case("String", "abc", settingsByName["ReadAsBytes"], "ReadAsBytes"); + yield return Case("FixedString(5)", "abcde", settingsByName["ReadAsBytes"], "ReadAsBytes"); + yield return Case("Nullable(String)", "abc", settingsByName["ReadAsBytes"], "ReadAsBytes"); + yield return Case("Nullable(String)", DBNull.Value, settingsByName["ReadAsBytes"], "ReadAsBytesNull"); + yield return Case("Decimal(10, 2)", 12.34m, settingsByName["BigDecimal"], "BigDecimal"); + yield return Case("Nullable(Decimal(10, 2))", DBNull.Value, settingsByName["BigDecimal"], "BigDecimalNull"); + + // Wire-transparent wrappers must behave exactly like the type they wrap. + yield return Case("LowCardinality(String)", "abc", TypeSettings.Default, string.Empty); + yield return Case("LowCardinality(Nullable(String))", "abc", TypeSettings.Default, "Present"); + yield return Case("LowCardinality(Nullable(String))", DBNull.Value, TypeSettings.Default, "Null"); + yield return Case("SimpleAggregateFunction(sum, UInt64)", 42ul, TypeSettings.Default, string.Empty); + yield return Case("SimpleAggregateFunction(any, Nullable(Int32))", DBNull.Value, TypeSettings.Default, "Null"); + + // Passed as a closure rather than as arguments because TypeSettings is internal, and an internal + // parameter type on a public test method does not compile. + static TestCaseData Case(string typeName, object value, TypeSettings settings, string suffix) + => new TestCaseData((Action)(() => AssertParity(typeName, value, settings))) + .SetName($"Parity_{TestUtilities.SanitizeTableName(typeName)}{suffix}_MatchesBoxedRead"); + + static IEnumerable<(string TypeName, object Value)> Values() + { + yield return ("Int8", (sbyte)-8); + yield return ("Int16", (short)-16); + yield return ("Int32", -32); + yield return ("Int64", -64L); + yield return ("UInt8", (byte)8); + yield return ("UInt16", (ushort)16); + yield return ("UInt32", 32u); + yield return ("UInt64", 64ul); + yield return ("Int128", (BigInteger)(-128)); + yield return ("UInt128", (BigInteger)128); + yield return ("Int256", (BigInteger)(-256)); + yield return ("UInt256", (BigInteger)256); + yield return ("Float32", 1.5f); + yield return ("Float64", 2.5d); + yield return ("BFloat16", 1.25f); + yield return ("Bool", true); + yield return ("String", "abc"); + yield return ("FixedString(5)", "abcde"); + yield return ("UUID", Guid.Parse("11223344-5566-7788-99aa-bbccddeeff00")); + yield return ("IPv4", IPAddress.Parse("10.0.0.1")); + yield return ("IPv6", IPAddress.Parse("2001:db8::1")); + yield return ("Date", new DateTime(2021, 3, 4, 0, 0, 0, DateTimeKind.Utc)); + yield return ("Date32", new DateTime(2021, 3, 4, 0, 0, 0, DateTimeKind.Utc)); + yield return ("DateTime('UTC')", new DateTime(2021, 3, 4, 5, 6, 7, DateTimeKind.Utc)); + yield return ("DateTime64(3, 'UTC')", new DateTime(2021, 3, 4, 5, 6, 7, 123, DateTimeKind.Utc)); + yield return ("Time", TimeSpan.FromSeconds(3661)); + yield return ("Time64(3)", TimeSpan.FromMilliseconds(3661123)); + yield return ("Decimal(10, 2)", 12.34m); + yield return ("Decimal(30, 4)", 1234.5678m); + yield return (Enum8Def, "b"); + + // No typed reader: these must fall back and still round-trip byte-for-byte. + yield return ("Array(Int32)", new[] { 1, 2, 3 }); + yield return ("Tuple(Int32, String)", Tuple.Create(1, "a")); + yield return ("Map(String, Int32)", new Dictionary { ["a"] = 1 }); + } + } + + [TestCaseSource(nameof(ParityCases))] + public void Parity_SlotMatchesBoxedRead_InValueAndByteCount(Action assertion) => assertion(); + + private static void AssertParity(string typeName, object value, TypeSettings settings) + { + var type = Parse(typeName, settings); + var payload = Write(type, value); + + var boxed = ReadBoxed(type, payload); + var slot = ReadSlot(type, payload); + + Assert.Multiple(() => + { + Assert.That(slot.Trailer, Is.EqualTo(Sentinel), + $"{typeName}: the slot consumed the wrong number of bytes"); + Assert.That(boxed.Trailer, Is.EqualTo(Sentinel), + $"{typeName}: the boxed reader consumed the wrong number of bytes (bad test payload)"); + Assert.That(slot.Value, Is.EqualTo(boxed.Value), + $"{typeName}: slot value differs from the boxed read"); + Assert.That(slot.Value?.GetType(), Is.EqualTo(boxed.Value?.GetType()), + $"{typeName}: slot boxed the value as a different CLR type than the boxed read"); + Assert.That(slot.Slot.IsNull, Is.EqualTo(boxed.Value is null or DBNull), + $"{typeName}: IsNull disagrees with the boxed read's null representation"); + }); + } + + // ---- Slot selection: a silent demotion to BoxedSlot costs the allocation win but changes no value ---- + + [TestCase("Int32", typeof(ValueSlot))] + [TestCase("UInt64", typeof(ValueSlot))] + [TestCase("Float64", typeof(ValueSlot))] + [TestCase("Bool", typeof(ValueSlot))] + [TestCase("String", typeof(ValueSlot))] + [TestCase("FixedString(3)", typeof(ValueSlot))] + [TestCase("UUID", typeof(ValueSlot))] + [TestCase("IPv6", typeof(ValueSlot))] + [TestCase("Date", typeof(ValueSlot))] + [TestCase("DateTime64(3, 'UTC')", typeof(ValueSlot))] + [TestCase("Time64(3)", typeof(ValueSlot))] + [TestCase("Decimal(10, 2)", typeof(ValueSlot))] // TypeSettings.Default is useBigDecimal + [TestCase("Int256", typeof(ValueSlot))] + [TestCase(Enum8Def, typeof(ValueSlot))] + [TestCase("LowCardinality(String)", typeof(ValueSlot))] + [TestCase("SimpleAggregateFunction(sum, UInt64)", typeof(ValueSlot))] + [TestCase("Nullable(Int32)", typeof(NullableSlot))] + [TestCase("Nullable(String)", typeof(NullableSlot))] + [TestCase("Nullable(UUID)", typeof(NullableSlot))] + [TestCase("LowCardinality(Nullable(String))", typeof(NullableSlot))] + [TestCase("SimpleAggregateFunction(any, Nullable(Int32))", typeof(NullableSlot))] + // No ITypedReader for the column's own FrameworkType: composites, polymorphic and geo types. + [TestCase("Array(Int32)", typeof(BoxedSlot))] + [TestCase("Tuple(Int32, String)", typeof(BoxedSlot))] + [TestCase("Map(String, Int32)", typeof(BoxedSlot))] + [TestCase("Nullable(Array(Int32))", typeof(BoxedSlot))] + [TestCase("Variant(Int64, String)", typeof(BoxedSlot))] + [TestCase("Point", typeof(BoxedSlot))] + [TestCase("Nothing", typeof(BoxedSlot))] + public void Create_Column_ResolvesToExpectedSlotKind(string typeName, Type expected) + => Assert.That(ColumnSlotFactory.Create(Parse(typeName)), Is.TypeOf(expected)); + + // FrameworkType is instance state, not class state, so the factory has to read it off the resolved + // instance. Caching by ClickHouseType class alone would bind every String column to one representation. + [TestCase(false, typeof(ValueSlot))] + [TestCase(true, typeof(ValueSlot))] + public void Create_StringColumn_BindsToTheRepresentationTheSettingsSelect(bool asByteArray, Type expected) + { + var settings = TypeSettings.Default with { readStringsAsByteArrays = asByteArray }; + Assert.That(ColumnSlotFactory.Create(Parse("String", settings)), Is.TypeOf(expected)); + } + + [TestCase(false, typeof(ValueSlot))] + [TestCase(true, typeof(ValueSlot))] + public void Create_DecimalColumn_BindsToTheRepresentationTheSettingsSelect(bool useBigDecimal, Type expected) + { + var settings = TypeSettings.Default with { useBigDecimal = useBigDecimal }; + Assert.That(ColumnSlotFactory.Create(Parse("Decimal(10, 2)", settings)), Is.TypeOf(expected)); + } + + // ---- IsNull: no boxed counterpart, so parity cannot cover it ---- + + [Test] + public void NullableSlot_AfterNullThenValue_TracksPresencePerRow() + { + var type = Parse("Nullable(Int32)"); + var slot = ColumnSlotFactory.Create(type); + + using var stream = new MemoryStream(Concat(Write(type, DBNull.Value), Write(type, 7))); + using var reader = new ExtendedBinaryReader(stream); + + slot.Read(reader); + Assert.That(slot.IsNull, Is.True); + Assert.That(slot.GetBoxed(), Is.EqualTo(DBNull.Value)); + Assert.That(reader.ReadInt64(), Is.EqualTo(Sentinel)); + + slot.Read(reader); + Assert.That(slot.IsNull, Is.False); + Assert.That(slot.GetBoxed(), Is.EqualTo(7)); + Assert.That(reader.ReadInt64(), Is.EqualTo(Sentinel)); + } + + // A slot is reused across rows, so a null must clear the previous row's value rather than leaving it + // reachable — otherwise a single null cell pins the last string/byte[] for the life of the reader. + [Test] + public void NullableSlot_ValueThenNull_ClearsPreviousRowsReference() + { + var type = Parse("Nullable(String)"); + var slot = (NullableSlot)ColumnSlotFactory.Create(type); + + using var stream = new MemoryStream(Concat(Write(type, "kept-alive"), Write(type, DBNull.Value))); + using var reader = new ExtendedBinaryReader(stream); + + slot.Read(reader); + Assert.That(slot.Value, Is.EqualTo("kept-alive")); + reader.ReadInt64(); + + slot.Read(reader); + Assert.That(slot.Value, Is.Null, "a null cell must release the previous row's value"); + } + + // A non-nullable column has no null marker on the wire, so nothing it decodes is ever null. + [TestCase("Int32", 5)] + [TestCase("String", "")] + public void ValueSlot_AfterRead_IsNeverNull(string typeName, object value) + { + var type = Parse(typeName); + var (_, _, slot) = ReadSlot(type, Write(type, value)); + Assert.That(slot.IsNull, Is.False); + } + + private static byte[] Concat(byte[] first, byte[] second) => first.Concat(second).ToArray(); +} diff --git a/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs b/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs index 0f9411430..62febec42 100644 --- a/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs +++ b/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs @@ -36,6 +36,10 @@ public class ClickHouseDataReader : DbDataReader, IEnumerator, IEnu private readonly string[] columnTypeNames; // Raw server-sent type strings, exactly as declared private readonly PocoTypeRegistry pocoRegistry; private readonly Dictionary bindingPlanCache = new(); + + // Per-column typed storage for the current row; replaces the old shared object[] buffer. Built once per + // reader, mutated in place by every Read(). Always non-null and always FieldNames.Length long. + private readonly ColumnSlot[] slots; private bool hasCurrentRow; private ClickHouseDataReader(HttpResponseMessage httpResponse, ExtendedBinaryReader reader, PooledReadBufferStream pooledReadBuffer, string[] names, ClickHouseType[] types, string[] rawTypeNames, PocoTypeRegistry pocoRegistry, ExceptionTagAwareStream exceptionTagStream = null, IReadValueConverter readValueConverter = null, Stream decompressor = null) @@ -52,8 +56,11 @@ private ClickHouseDataReader(HttpResponseMessage httpResponse, ExtendedBinaryRea this.pocoRegistry = pocoRegistry; RawTypes = types; FieldNames = names; - CurrentRow = new object[FieldNames.Length]; columnTypeNames = rawTypeNames; + + slots = new ColumnSlot[types.Length]; + for (var i = 0; i < types.Length; i++) + slots[i] = ColumnSlotFactory.Create(types[i]); } internal static Task FromHttpResponseAsync(HttpResponseMessage httpResponse, TypeSettings settings) @@ -149,8 +156,6 @@ internal ClickHouseType GetEffectiveClickHouseType(int ordinal) public override int RecordsAffected { get; } - protected object[] CurrentRow { get; set; } - protected string[] FieldNames { get; set; } private protected ClickHouseType[] RawTypes { get; set; } @@ -211,41 +216,44 @@ public override int GetOrdinal(string name) public override string GetString(int ordinal) => GetValue(ordinal)?.ToString(); + /// + /// The one boxing entry point on the read path. Boxes lazily, per call, so a query that projects ten + /// columns and reads two pays for two — where the old object[] buffer boxed all ten during + /// regardless. + /// + /// + /// Consequence of boxing per call rather than once per row: two GetValue(i) calls on the same + /// value-type cell now return two distinct boxes. They compare equal by + /// (the ADO.NET-relevant comparison) but no longer by . + /// public override object GetValue(int ordinal) => readValueConverter == null - ? CurrentRow[ordinal] - : readValueConverter.ConvertValue(CurrentRow[ordinal], FieldNames[ordinal], columnTypeNames[ordinal]); + ? slots[ordinal].GetBoxed() + : readValueConverter.ConvertValue(slots[ordinal].GetBoxed(), FieldNames[ordinal], columnTypeNames[ordinal]); public override int GetValues(object[] values) { - if (CurrentRow == null) - { - throw new InvalidOperationException(); - } - - var count = Math.Min(CurrentRow.Length, values.Length); + var count = Math.Min(slots.Length, values.Length); if (readValueConverter != null) { for (var i = 0; i < count; i++) - values[i] = readValueConverter.ConvertValue(CurrentRow[i], FieldNames[i], columnTypeNames[i]); + values[i] = readValueConverter.ConvertValue(slots[i].GetBoxed(), FieldNames[i], columnTypeNames[i]); } else { - Array.Copy(CurrentRow, values, count); + for (var i = 0; i < count; i++) + values[i] = slots[i].GetBoxed(); } return count; } public override bool IsDBNull(int ordinal) - { - // Read CurrentRow directly rather than going through GetValue so a configured - // IReadValueConverter does not run during a null check — it could throw, do - // expensive work, or change the nullness of the result. - var value = CurrentRow[ordinal]; - return value is DBNull || value is null; - } + // Asks the slot directly rather than going through GetValue, for two reasons: a configured + // IReadValueConverter must not run during a null check (it could throw, do expensive work, or + // change the nullness of the result), and a null check has no business materializing a box. + => slots[ordinal].IsNull; public override bool NextResult() => false; @@ -293,7 +301,7 @@ public override T GetFieldValue(int ordinal) } } - var value = (T)CurrentRow[ordinal]; + var value = (T)slots[ordinal].GetBoxed(); if (readValueConverter != null) return readValueConverter.ConvertValue(value, FieldNames[ordinal], columnTypeNames[ordinal]); return value; @@ -515,11 +523,8 @@ private void ValidateBinding(Type pocoType, PocoPropertyInfo propInfo, int colum public override bool Read() { - var count = RawTypes.Length; - var data = CurrentRow; - // Clear before the per-column loop so a mid-row throw cannot leave a stale - // CurrentRow visible to MapTo if the caller catches and continues. + // row visible to MapTo if the caller catches and continues. hasCurrentRow = false; try { @@ -528,10 +533,9 @@ public override bool Read() if (reader.PeekChar() == -1) return false; // End of stream reached - for (var i = 0; i < count; i++) + for (var i = 0; i < slots.Length; i++) { - var rawType = RawTypes[i]; - data[i] = rawType.Read(reader); + slots[i].Read(reader); } hasCurrentRow = true; return true; diff --git a/ClickHouse.Driver/ADO/Readers/ColumnSlot.cs b/ClickHouse.Driver/ADO/Readers/ColumnSlot.cs new file mode 100644 index 000000000..ad35fba68 --- /dev/null +++ b/ClickHouse.Driver/ADO/Readers/ColumnSlot.cs @@ -0,0 +1,120 @@ +using System; +using ClickHouse.Driver.Formats; +using ClickHouse.Driver.Types; + +namespace ClickHouse.Driver.ADO.Readers; + +/// +/// Per-column storage for the reader's current row. One instance per wire column, allocated once per +/// and overwritten in place on every . +/// +/// This replaces the shared object[] row buffer, whose every value-type cell was boxed once per +/// value per row by — whether or not the caller ever +/// asked for that column. A slot decodes into strongly-typed storage instead, so the box happens only when +/// someone actually calls an untyped accessor (), and never at all for the typed ones. +/// +/// Every slot must be observationally identical to the boxed path it replaces: +/// has to return exactly what would have returned for +/// the same bytes, including for a SQL NULL, and has to consume +/// exactly the same bytes. +/// +internal abstract class ColumnSlot +{ + /// Decodes this column's bytes from the row stream into typed storage. + public abstract void Read(ExtendedBinaryReader reader); + + /// + /// Boxes the current value on demand for the untyped path. Returns for a SQL + /// NULL, matching . + /// + public abstract object GetBoxed(); + + /// Null check that neither materializes nor boxes the value. + public abstract bool IsNull { get; } +} + +/// +/// A non-nullable column whose type can decode straight into . +/// is always the column type's , so the +/// stored value is exactly what the boxed produces. +/// +internal sealed class ValueSlot : ColumnSlot +{ + // typeof(T) is a JIT-time constant per closed generic, so this static folds to a constant load (the same + // trick ClickHouseDataReader.FieldValueDispatcher uses). For a value-typed T the IsNull check below + // then folds to `false` outright and the boxing conversion in it is never reached. + private static readonly bool CanBeNull = !typeof(T).IsValueType; + + private readonly ITypedReader typedReader; + + public T Value; + + public ValueSlot(ITypedReader typedReader) => this.typedReader = typedReader; + + public override void Read(ExtendedBinaryReader reader) => Value = typedReader.ReadValue(reader); + + public override object GetBoxed() => Value; + + // A non-nullable column has no null marker on the wire, so the only way this can be null is a + // reference-typed reader handing one back — none currently do, but the check keeps GetBoxed and IsNull + // agreeing with the boxed path if one ever did. + public override bool IsNull => CanBeNull && (object)Value is null; +} + +/// +/// A Nullable(T) column: the decoded value plus a presence flag, so a NULL costs no object at all +/// (the boxed path allocated nothing for it either — it returned the singleton — +/// but it did box every non-null cell). +/// +internal sealed class NullableSlot : ColumnSlot +{ + private readonly ITypedReader typedReader; + + public T Value; + + public bool HasValue; + + public NullableSlot(ITypedReader typedReader) => this.typedReader = typedReader; + + public override void Read(ExtendedBinaryReader reader) + { + // Byte-identical to NullableType.Read: a marker > 0 means NULL, and in that case the underlying type + // wrote nothing, so nothing more is consumed. + if (reader.ReadByte() > 0) + { + HasValue = false; + Value = default; // don't pin the previous row's value (a string/byte[] would stay reachable) + } + else + { + Value = typedReader.ReadValue(reader); + HasValue = true; + } + } + + // Boxes the *underlying* value, never a Nullable — NullableType.Read did the same, and + // GetFieldValue on a Nullable(Int64) column depends on finding a boxed long here. + public override object GetBoxed() => HasValue ? (object)Value : DBNull.Value; + + public override bool IsNull => !HasValue; +} + +/// +/// Fallback for any column with no for its own +/// — composites (Array, Tuple, Map, Nested), the polymorphic types +/// (Variant, Dynamic, JSON), geo types, and so on. Byte-for-byte and value-for-value the pre-slot behaviour. +/// +internal sealed class BoxedSlot : ColumnSlot +{ + private readonly ClickHouseType type; + + public object Value; + + public BoxedSlot(ClickHouseType type) => this.type = type; + + public override void Read(ExtendedBinaryReader reader) => Value = type.Read(reader); + + public override object GetBoxed() => Value; + + public override bool IsNull => Value is null || Value is DBNull; +} diff --git a/ClickHouse.Driver/ADO/Readers/ColumnSlotFactory.cs b/ClickHouse.Driver/ADO/Readers/ColumnSlotFactory.cs new file mode 100644 index 000000000..23f4d7d95 --- /dev/null +++ b/ClickHouse.Driver/ADO/Readers/ColumnSlotFactory.cs @@ -0,0 +1,101 @@ +using System; +using System.Collections.Concurrent; +using System.Linq; +using System.Reflection; +using ClickHouse.Driver.Types; + +namespace ClickHouse.Driver.ADO.Readers; + +/// +/// Builds the for a resolved column type. +/// +/// A column gets a typed slot iff its type implements ITypedReader<FrameworkType> — that +/// is, iff it can decode straight into the very CLR type its boxed +/// would have returned. Every current +/// implementor satisfies that by construction (its Read body is the typed read, or picks +/// between typed reads by the same setting that picks FrameworkType), so a typed slot and the boxed +/// path always produce the same value from the same bytes. Anything else gets a . +/// +/// Slots are built from the resolved type instance, never from a cached shape, so settings that +/// change FrameworkType — ReadStringsAsByteArrays (string vs byte[]), UseBigDecimal +/// (decimal vs ClickHouseDecimal) — are handled without any of the cache-key hazards a per-query-shape cache +/// would have. +/// +internal static class ColumnSlotFactory +{ + private static readonly MethodInfo CreateValueSlotMethod = + typeof(ColumnSlotFactory).GetMethod(nameof(CreateValueSlot), BindingFlags.NonPublic | BindingFlags.Static); + + private static readonly MethodInfo CreateNullableSlotMethod = + typeof(ColumnSlotFactory).GetMethod(nameof(CreateNullableSlot), BindingFlags.NonPublic | BindingFlags.Static); + + // Concrete ClickHouseType class -> the CLR types it can read box-free. Keyed on the class, not the + // instance, because the implemented interface list is fixed per class. Bounded by the number of + // ClickHouseType subclasses (~60), and it is what keeps SlotConstructors below bounded too: without it, + // a composite column's FrameworkType (long[], Dictionary, Tuple<...>, ...) would be an unbounded + // family of keys, none of which could ever produce a typed slot. + private static readonly ConcurrentDictionary TypedReadTargets = new(); + + // CLR type -> closed-generic slot constructors. Bounded by the union of every ITypedReader's T + // (~25 types), because a key only ever gets here after passing the TypedReadTargets check. + private static readonly ConcurrentDictionary ConstructorCache = new(); + + /// + /// Returns the slot for . Never null: falls back to a + /// over the original (still-wrapped) type, so the wire read is unchanged for anything unsupported. + /// + public static ColumnSlot Create(ClickHouseType type) + { + var unwrapped = TransparentWrapper.Unwrap(type); + + // Nullable is not wire-transparent — it prefixes a marker byte — so it selects the slot kind rather + // than being unwrapped away. Its underlying may itself be wrapped, e.g. Nullable(LowCardinality(T)). + if (unwrapped is NullableType nullableType) + return TryCreateTyped(TransparentWrapper.Unwrap(nullableType.UnderlyingType), nullable: true) ?? new BoxedSlot(type); + + return TryCreateTyped(unwrapped, nullable: false) ?? new BoxedSlot(type); + } + + private static ColumnSlot TryCreateTyped(ClickHouseType type, bool nullable) + { + var targets = TypedReadTargets.GetOrAdd(type.GetType(), FindTypedReadTargets); + if (targets.Length == 0) + return null; + + var clrType = type.FrameworkType; + if (Array.IndexOf(targets, clrType) < 0) + return null; + + var constructors = ConstructorCache.GetOrAdd(clrType, BuildSlotConstructors); + return nullable ? constructors.Nullable(type) : constructors.Value(type); + } + + private static Type[] FindTypedReadTargets(Type clickHouseTypeClass) => clickHouseTypeClass + .GetInterfaces() + .Where(i => i.IsGenericType && i.GetGenericTypeDefinition() == typeof(ITypedReader<>)) + .Select(i => i.GetGenericArguments()[0]) + .ToArray(); + + private static SlotConstructors BuildSlotConstructors(Type clrType) => new( + (Func)CreateValueSlotMethod + .MakeGenericMethod(clrType) + .CreateDelegate(typeof(Func)), + (Func)CreateNullableSlotMethod + .MakeGenericMethod(clrType) + .CreateDelegate(typeof(Func))); + + // The `is ITypedReader` here is the real check; TypedReadTargets is only a cheap pre-filter that keeps + // the constructor cache bounded, so these still return null rather than assuming the cast succeeds. + private static ColumnSlot CreateValueSlot(ClickHouseType type) + => type is ITypedReader typedReader ? new ValueSlot(typedReader) : null; + + private static ColumnSlot CreateNullableSlot(ClickHouseType type) + => type is ITypedReader typedReader ? new NullableSlot(typedReader) : null; + + private readonly struct SlotConstructors(Func value, Func nullable) + { + public Func Value { get; } = value; + + public Func Nullable { get; } = nullable; + } +} From 8223e0dc637bd9b1ef70949f9bfce1b74510bd1f Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Sun, 2 Aug 2026 12:34:14 +0200 Subject: [PATCH 03/16] perf(ado): read GetFieldValue straight out of the typed slot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the tiered dispatch on top of the column slots: GetFieldValue now takes the value from ValueSlot or NullableSlot directly, and only falls back to (T)GetBoxed() when neither matches. On the common path — a caller asking for exactly the type the column decodes to — the box and the matching unbox both disappear. Dispatch is two sealed-class isinst checks rather than one generic-interface check. A generic IValueGetter implemented twice would let a single slot serve both long and long?, but it resolves through the shared-generics dictionary and benchmarks at 3.5-5.5x the cost of the unbox it replaces, against 1.5-2.4x for the sealed-class checks. So T = U? is deliberately left to the boxed fallback: it is rare in practice (linq2db pairs IsDBNull with GetInt64, Dapper uses GetValue) and it still works, just without the win. No semantic change. A configured IReadValueConverter still sees ConvertValue with the identical T — that overload exists for exactly this case — and the fallback is the pre-slot expression verbatim, so exact-type strictness is preserved by construction: no widening, and reading a NULL as a non-nullable T still throws the runtime's own "cannot cast DBNull" InvalidCastException. BoxFreeReaderAccessorTests pins the declined cases against a live server, since those are the ones a future "helpful" coercion would silently break. Co-Authored-By: Claude --- .../ADO/BoxFreeReaderAccessorTests.cs | 240 ++++++++++++++++++ .../ADO/Readers/ClickHouseDataReader.cs | 34 ++- 2 files changed, 273 insertions(+), 1 deletion(-) create mode 100644 ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs diff --git a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs new file mode 100644 index 000000000..46445175d --- /dev/null +++ b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs @@ -0,0 +1,240 @@ +using System; +using System.Threading.Tasks; +using ClickHouse.Driver.ADO.Readers; +using ClickHouse.Driver.Utility; + +namespace ClickHouse.Driver.Tests.ADO; + +/// +/// Pins the accessor semantics of the typed-column-slot reader against a live server. +/// +/// proves a slot decodes the same bytes to the same value as the boxed +/// reader. What it cannot see is the layer above: which slot each accessor reaches for, and — more +/// importantly — that the cases the fast path deliberately declines still fail in exactly the way they used +/// to. Widening, reading a NULL as a non-nullable target, and T = U? all fall through to the boxed +/// cast, and their s are part of the ADO.NET contract callers rely on. +/// +[TestFixture] +public class BoxFreeReaderAccessorTests : AbstractConnectionTestFixture +{ + private async Task ReadOneAsync(string selectList) + { + var reader = (ClickHouseDataReader)await connection.ExecuteReaderAsync($"SELECT {selectList}"); + Assert.That(reader.Read(), Is.True); + return reader; + } + + // ---- GetFieldValue: the typed slot must produce exactly what the boxed cast produced ---- + + [TestCase("toInt8(-8)", typeof(sbyte), (sbyte)-8)] + [TestCase("toInt16(-16)", typeof(short), (short)-16)] + [TestCase("toInt32(-32)", typeof(int), -32)] + [TestCase("toInt64(-64)", typeof(long), -64L)] + [TestCase("toUInt8(8)", typeof(byte), (byte)8)] + [TestCase("toUInt16(16)", typeof(ushort), (ushort)16)] + [TestCase("toUInt32(32)", typeof(uint), 32u)] + [TestCase("toUInt64(64)", typeof(ulong), 64ul)] + [TestCase("toFloat32(1.5)", typeof(float), 1.5f)] + [TestCase("toFloat64(2.5)", typeof(double), 2.5d)] + [TestCase("true", typeof(bool), true)] + [TestCase("'abc'", typeof(string), "abc")] + public async Task GetFieldValue_ExactTargetType_ReturnsValue(string expression, Type target, object expected) + { + using var reader = await ReadOneAsync($"{expression} AS c"); + Assert.That(GetFieldValueDynamic(reader, target), Is.EqualTo(expected)); + } + + // Invariant in T, as the boxed unbox-any was: no widening, ever. + [TestCase("toInt32(1)", typeof(long))] + [TestCase("toFloat32(1)", typeof(double))] + [TestCase("toUInt8(1)", typeof(int))] + [TestCase("toInt64(1)", typeof(string))] + public async Task GetFieldValue_WideningTarget_ThrowsInvalidCast(string expression, Type target) + { + using var reader = await ReadOneAsync($"{expression} AS c"); + Assert.Throws(() => GetFieldValueDynamic(reader, target)); + } + + // A Nullable target over a non-nullable column has no typed slot (the slot holds U, not U?), but the + // boxed fallback lifts it, so it keeps working. + [Test] + public async Task GetFieldValue_NullableTargetOverNonNullableColumn_LiftsThroughBoxedFallback() + { + using var reader = await ReadOneAsync("toInt64(9) AS c"); + Assert.That(reader.GetFieldValue(0), Is.EqualTo(9L)); + } + + [Test] + public async Task GetFieldValue_NullableColumnWithValue_ReadsUnderlyingType() + { + using var reader = await ReadOneAsync("toInt64OrNull('7') AS c"); + Assert.Multiple(() => + { + Assert.That(reader.IsDBNull(0), Is.False); + Assert.That(reader.GetFieldValue(0), Is.EqualTo(7L)); + // T = U? is deliberately left to the boxed fallback (a generic-interface dispatch that would + // serve both costs 3.5-5.5x the sealed-class check). It must still work. + Assert.That(reader.GetFieldValue(0), Is.EqualTo(7L)); + }); + } + + // A NULL cell has no value to hand back as a non-nullable T, so the boxed DBNull cast throws — the + // pre-slot behaviour, preserved by letting the null case fall through to GetBoxed(). + [Test] + public async Task GetFieldValue_NullCellAsNonNullableTarget_ThrowsInvalidCast() + { + using var reader = await ReadOneAsync("CAST(NULL AS Nullable(Int64)) AS c"); + Assert.That(reader.IsDBNull(0), Is.True); + Assert.Throws(() => reader.GetFieldValue(0)); + } + + // Also pre-slot behaviour, and the surprising one: DBNull does not unbox to Nullable either. + [Test] + public async Task GetFieldValue_NullCellAsNullableTarget_ThrowsInvalidCast() + { + using var reader = await ReadOneAsync("CAST(NULL AS Nullable(Int64)) AS c"); + Assert.Throws(() => reader.GetFieldValue(0)); + } + + [Test] + public async Task GetFieldValue_ObjectTarget_ReturnsTheBoxedValueAndDBNullForNull() + { + using var reader = await ReadOneAsync("toInt64(5) AS a, CAST(NULL AS Nullable(Int64)) AS b"); + Assert.Multiple(() => + { + Assert.That(reader.GetFieldValue(0), Is.EqualTo(5L)); + Assert.That(reader.GetFieldValue(1), Is.EqualTo(DBNull.Value)); + }); + } + + // A composite has no typed slot; GetFieldValue must keep working through the boxed fallback. + [Test] + public async Task GetFieldValue_CompositeColumn_ReadsThroughBoxedFallback() + { + using var reader = await ReadOneAsync("array(toInt32(1), toInt32(2), toInt32(3)) AS c"); + Assert.That(reader.GetFieldValue(0), Is.EqualTo(new[] { 1, 2, 3 })); + } + + // ---- IsDBNull now answers from the slot's presence flag rather than inspecting a boxed value ---- + + [Test] + public async Task IsDBNull_AcrossColumnKinds_MatchesNullability() + { + using var reader = await ReadOneAsync( + "toInt64(1) AS nonNullableValue, " + + "'s' AS nonNullableRef, " + + "toInt64OrNull('1') AS nullableWithValue, " + + "CAST(NULL AS Nullable(Int64)) AS nullableValue, " + + "CAST(NULL AS Nullable(String)) AS nullableRef, " + + "array(1) AS composite, " + + "CAST(NULL AS Nullable(UUID)) AS nullableUuid"); + + Assert.Multiple(() => + { + Assert.That(reader.IsDBNull(0), Is.False); + Assert.That(reader.IsDBNull(1), Is.False); + Assert.That(reader.IsDBNull(2), Is.False); + Assert.That(reader.IsDBNull(3), Is.True); + Assert.That(reader.IsDBNull(4), Is.True); + Assert.That(reader.IsDBNull(5), Is.False); + Assert.That(reader.IsDBNull(6), Is.True); + }); + } + + // A slot is reused across rows, so presence has to be re-decoded every row rather than latched. + [Test] + public async Task IsDBNull_AlternatingNullsAcrossRows_TracksEachRow() + { + using var reader = (ClickHouseDataReader)await connection.ExecuteReaderAsync( + "SELECT if(number % 2 = 0, NULL, toInt64(number)) AS c FROM system.numbers LIMIT 6"); + + for (var row = 0; row < 6; row++) + { + Assert.That(reader.Read(), Is.True); + Assert.That(reader.IsDBNull(0), Is.EqualTo(row % 2 == 0), $"row {row}"); + if (row % 2 != 0) + Assert.That(reader.GetFieldValue(0), Is.EqualTo((long)row), $"row {row}"); + } + } + + // ---- GetValues / GetValue keep the boxed contract the BCL data-binding stack depends on ---- + + [Test] + public async Task GetValues_MixedRow_ProducesTheSameBoxedValuesAsGetValue() + { + using var reader = await ReadOneAsync( + "toInt64(1) AS a, 'b' AS b, CAST(NULL AS Nullable(Int64)) AS c, array(1, 2) AS d"); + + var values = new object[reader.FieldCount]; + Assert.That(reader.GetValues(values), Is.EqualTo(4)); + Assert.Multiple(() => + { + Assert.That(values[0], Is.EqualTo(1L)); + Assert.That(values[1], Is.EqualTo("b")); + Assert.That(values[2], Is.EqualTo(DBNull.Value)); + Assert.That(values[3], Is.EqualTo(new[] { 1, 2 })); + for (var i = 0; i < values.Length; i++) + Assert.That(values[i], Is.EqualTo(reader.GetValue(i)), $"column {i}"); + }); + } + + // GetValues writes as many cells as the caller's array has room for, and no more. + [Test] + public async Task GetValues_ShorterDestinationArray_FillsOnlyWhatFits() + { + using var reader = await ReadOneAsync("toInt64(1) AS a, toInt64(2) AS b, toInt64(3) AS c"); + + var values = new object[2]; + Assert.That(reader.GetValues(values), Is.EqualTo(2)); + Assert.That(values, Is.EqualTo(new object[] { 1L, 2L })); + } + + // Boxing is now per call rather than once per row, so the same cell yields two distinct boxes. They must + // still compare equal — that is the comparison ADO.NET consumers and DataTable actually use. + [Test] + public async Task GetValue_CalledTwiceOnAValueTypeCell_ReturnsEqualValues() + { + using var reader = await ReadOneAsync("toInt64(42) AS c"); + Assert.That(reader.GetValue(0), Is.EqualTo(reader.GetValue(0))); + } + + // Values handed out must not be invalidated by advancing the reader: GetValue returns a fresh box, and a + // reference-typed cell hands out the reference the caller then owns. DataReaderTests exercises the same + // shape via LINQ over IDataRecord; this states the guarantee directly. + [Test] + public async Task GetValue_RetainedAcrossRead_KeepsTheOriginalRowsValue() + { + using var reader = (ClickHouseDataReader)await connection.ExecuteReaderAsync( + "SELECT toInt64(number) AS n, toString(number) AS s FROM system.numbers LIMIT 2"); + + Assert.That(reader.Read(), Is.True); + var firstNumber = reader.GetValue(0); + var firstString = reader.GetValue(1); + + Assert.That(reader.Read(), Is.True); + Assert.Multiple(() => + { + Assert.That(firstNumber, Is.EqualTo(0L)); + Assert.That(firstString, Is.EqualTo("0")); + Assert.That(reader.GetValue(0), Is.EqualTo(1L)); + }); + } + + // Reflection is the only way to parametrize over T. Unwraps TargetInvocationException so the assertions + // above see the exception the caller would actually get. + private static object GetFieldValueDynamic(ClickHouseDataReader reader, Type target) + { + var method = typeof(ClickHouseDataReader) + .GetMethod(nameof(ClickHouseDataReader.GetFieldValue), [typeof(int)]) + .MakeGenericMethod(target); + try + { + return method.Invoke(reader, [0]); + } + catch (System.Reflection.TargetInvocationException ex) when (ex.InnerException != null) + { + System.Runtime.ExceptionServices.ExceptionDispatchInfo.Capture(ex.InnerException).Throw(); + throw; // unreachable + } + } +} diff --git a/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs b/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs index 62febec42..6890cc0de 100644 --- a/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs +++ b/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs @@ -301,12 +301,44 @@ public override T GetFieldValue(int ordinal) } } - var value = (T)slots[ordinal].GetBoxed(); + var value = GetSlotValue(ordinal); if (readValueConverter != null) return readValueConverter.ConvertValue(value, FieldNames[ordinal], columnTypeNames[ordinal]); return value; } + /// + /// Extracts the current row's column value as , without boxing where the slot + /// already holds exactly that type. + /// + /// + /// The two sealed-class checks are ordered by cost. For a value-typed the + /// runtime JITs a dedicated instantiation, so each is a plain isinst against a known method table — + /// measured at 1.5–2.4x the cost of the unbox it replaces, against roughly 10 ns per column of decode. + /// A generic IValueGetter<T> interface implemented twice would let one slot serve both + /// long and long?, but it goes through the shared-generics dictionary and measured + /// 3.5–5.5x instead — so T = U? is deliberately left to the boxed fallback. + /// + /// The fallback is the pre-slot expression verbatim, which is what preserves the exact-type + /// strictness callers depend on: GetFieldValue<long> over an Int32 column throws, it + /// does not widen, and reading a NULL as a non-nullable throws the runtime's + /// own "cannot cast DBNull" exactly as before. + /// + private T GetSlotValue(int ordinal) + { + var slot = slots[ordinal]; + + if (slot is ValueSlot valueSlot) + return valueSlot.Value; + + // A Nullable(T) column holds the underlying T, so this also serves GetFieldValue over + // Nullable(Int64) — and when the cell is null it falls through to the box, which throws. + if (slot is NullableSlot nullableSlot && nullableSlot.HasValue) + return nullableSlot.Value; + + return (T)slot.GetBoxed(); + } + /// /// Per- cached predicate driving 's /// dispatch. The .NET runtime instantiates this generic exactly once per closed From 20a2881599443ad684ed81707897b82b765593cb Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Sun, 2 Aug 2026 12:43:44 +0200 Subject: [PATCH 04/16] perf(ado): read the typed accessors straight out of the slot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GetInt64/GetDouble/GetDateTime/GetGuid and friends were every one of them `(T)GetValue(ordinal)`, so each paid for a box it immediately unboxed. They now go through the same tiered slot dispatch GetFieldValue uses. This is the path compiled ORM mappers drive — linq2db registers GetBoolean/GetInt16/GetInt32/ GetInt64/GetFloat/GetDouble/GetString/GetDecimal/GetDateTime/GetGuid and inlines them per column per row — so it is where the win is worth the most. Three accessors are handled specially rather than mechanically: - GetBoolean coerces (Convert.ToBoolean) rather than casts, so only an exact Bool column short-circuits; everything else keeps the widening and its exception messages. - GetString coerces via ToString(), which includes the quirk that a NULL yields "" rather than null, because DBNull.Value.ToString() is empty. Only a non-nullable String column takes the fast path, leaving that behaviour alone. - GetDecimal reaches the value box-free under both decimal representations, since UseCustomDecimals decides whether the slot holds decimal or ClickHouseDecimal. GetChar and GetTuple have no slot to hit and stay on the boxed cast. With an IReadValueConverter configured the typed accessors keep routing through GetValue, so they still call ConvertValue(object, ...) exactly as before. De-boxing them would mean calling ConvertValue instead, which is observable to a converter whose two overloads disagree — not worth a silent semantic change for the rare converter case when everyone else gets the fast path anyway. GetFieldValue is unaffected either way: it already called ConvertValue. Measured on a 4-value-column payload, bytes allocated per row: Read() only 0 (was 96) typed accessors 0 (was 96) GetFieldValue 0 (was 96) GetValue 96 (unchanged, by design) BoxFreeReaderAllocationTests pins those numbers. It is the only test in the suite that can fail if boxing creeps back — every value assertion would still pass — so it is deliberately server-free and synchronous, driving a pre-built RowBinary payload on the test thread so GetAllocatedBytesForCurrentThread is exact rather than a sample of whatever else the process is doing. Co-Authored-By: Claude --- .../ADO/BoxFreeReaderAccessorTests.cs | 147 ++++++++++++++++++ .../ADO/BoxFreeReaderAllocationTests.cs | 139 +++++++++++++++++ .../ADO/Readers/ClickHouseDataReader.cs | 75 +++++++-- 3 files changed, 344 insertions(+), 17 deletions(-) create mode 100644 ClickHouse.Driver.Tests/ADO/BoxFreeReaderAllocationTests.cs diff --git a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs index 46445175d..0385590ad 100644 --- a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs +++ b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs @@ -1,5 +1,6 @@ using System; using System.Threading.Tasks; +using ClickHouse.Driver.ADO; using ClickHouse.Driver.ADO.Readers; using ClickHouse.Driver.Utility; @@ -220,6 +221,152 @@ public async Task GetValue_RetainedAcrossRead_KeepsTheOriginalRowsValue() }); } + // ---- Typed accessors: what linq2db's compiled mapper actually calls, one per column per row ---- + + [Test] + public async Task TypedAccessors_ExactColumnTypes_ReturnValues() + { + using var reader = await ReadOneAsync( + "toInt8(-8) AS a, toInt16(-16) AS b, toInt32(-32) AS c, toInt64(-64) AS d, " + + "toUInt8(8) AS e, toUInt16(16) AS f, toUInt32(32) AS g, toUInt64(64) AS h, " + + "toFloat32(1.5) AS i, toFloat64(2.5) AS j, true AS k, 'abc' AS l, " + + "toUUID('11223344-5566-7788-99aa-bbccddeeff00') AS m, " + + "toIPv6('2001:db8::1') AS n, toInt256(-256) AS o, " + + "toDateTime('2025-01-15 12:00:00', 'UTC') AS p"); + + Assert.Multiple(() => + { + Assert.That(reader.GetSByte(0), Is.EqualTo((sbyte)-8)); + Assert.That(reader.GetInt16(1), Is.EqualTo((short)-16)); + Assert.That(reader.GetInt32(2), Is.EqualTo(-32)); + Assert.That(reader.GetInt64(3), Is.EqualTo(-64L)); + Assert.That(reader.GetByte(4), Is.EqualTo((byte)8)); + Assert.That(reader.GetUInt16(5), Is.EqualTo((ushort)16)); + Assert.That(reader.GetUInt32(6), Is.EqualTo(32u)); + Assert.That(reader.GetUInt64(7), Is.EqualTo(64ul)); + Assert.That(reader.GetFloat(8), Is.EqualTo(1.5f)); + Assert.That(reader.GetDouble(9), Is.EqualTo(2.5d)); + Assert.That(reader.GetBoolean(10), Is.True); + Assert.That(reader.GetString(11), Is.EqualTo("abc")); + Assert.That(reader.GetGuid(12), Is.EqualTo(Guid.Parse("11223344-5566-7788-99aa-bbccddeeff00"))); + Assert.That(reader.GetIPAddress(13), Is.EqualTo(System.Net.IPAddress.Parse("2001:db8::1"))); + Assert.That(reader.GetBigInteger(14), Is.EqualTo(new System.Numerics.BigInteger(-256))); + Assert.That(reader.GetDateTime(15), Is.EqualTo(new DateTime(2025, 1, 15, 12, 0, 0, DateTimeKind.Utc))); + Assert.That(reader.GetDateTimeOffset(15), Is.EqualTo(new DateTimeOffset(2025, 1, 15, 12, 0, 0, TimeSpan.Zero))); + }); + } + + // A Nullable(T) slot holds the underlying T, so the typed accessors reach it without a box. + [Test] + public async Task TypedAccessors_NullableColumnsWithValues_ReturnUnderlyingValues() + { + using var reader = await ReadOneAsync( + "toInt64OrNull('7') AS a, toFloat64OrNull('1.5') AS b, " + + "CAST('s' AS Nullable(String)) AS c, CAST(true AS Nullable(Bool)) AS d"); + + Assert.Multiple(() => + { + Assert.That(reader.GetInt64(0), Is.EqualTo(7L)); + Assert.That(reader.GetDouble(1), Is.EqualTo(1.5d)); + Assert.That(reader.GetString(2), Is.EqualTo("s")); + Assert.That(reader.GetBoolean(3), Is.True); + }); + } + + // Pre-slot behaviour: the typed accessors were all `(T)GetValue(ordinal)`, so a NULL threw. + [Test] + public async Task TypedAccessors_NullCell_ThrowInvalidCast() + { + using var reader = await ReadOneAsync( + "CAST(NULL AS Nullable(Int64)) AS a, CAST(NULL AS Nullable(Float64)) AS b, " + + "CAST(NULL AS Nullable(UUID)) AS c, CAST(NULL AS Nullable(Bool)) AS d"); + + Assert.Multiple(() => + { + Assert.Throws(() => reader.GetInt64(0)); + Assert.Throws(() => reader.GetDouble(1)); + Assert.Throws(() => reader.GetGuid(2)); + Assert.Throws(() => reader.GetBoolean(3)); + }); + } + + // GetString coerces rather than casts, and DBNull.Value.ToString() is "". Surprising, but pre-existing + // and load-bearing for anyone relying on it — which is why the fast path only covers non-nullable String. + [Test] + public async Task GetString_NullCell_ReturnsEmptyStringNotNull() + { + using var reader = await ReadOneAsync("CAST(NULL AS Nullable(String)) AS c"); + Assert.That(reader.GetString(0), Is.EqualTo(string.Empty)); + } + + // GetBoolean is the one accessor that widens; only an exact Bool column short-circuits, so the + // Convert.ToBoolean coercion has to survive for everything else. + [TestCase("toUInt8(1)", true)] + [TestCase("toUInt8(0)", false)] + [TestCase("toInt32(5)", true)] + [TestCase("toFloat64(0)", false)] + public async Task GetBoolean_NonBoolColumn_StillCoerces(string expression, bool expected) + { + using var reader = await ReadOneAsync($"{expression} AS c"); + Assert.That(reader.GetBoolean(0), Is.EqualTo(expected)); + } + + // GetString coerces too, and must keep doing so for columns that are not String at all. + [Test] + public async Task GetString_NonStringColumn_StillCoerces() + { + using var reader = await ReadOneAsync("toInt64(42) AS c"); + Assert.That(reader.GetString(0), Is.EqualTo("42")); + } + + // A Decimal column resolves to a decimal or a ClickHouseDecimal slot depending on UseCustomDecimals; + // GetDecimal has to reach the same decimal either way. + [TestCase(false)] + [TestCase(true)] + public async Task GetDecimal_UnderEitherDecimalRepresentation_ReturnsTheSameValue(bool useCustomDecimals) + { + var settings = TestUtilities.GetTestClickHouseClientSettings(); + settings = new ClickHouseClientSettings(settings) { UseCustomDecimals = useCustomDecimals }; + using var client = new ClickHouseClient(settings); + + using var reader = await client.ExecuteReaderAsync("SELECT toDecimal64(12.34, 2) AS c"); + Assert.That(reader.Read(), Is.True); + Assert.That(reader.GetDecimal(0), Is.EqualTo(12.34m)); + } + + // ---- Converter routing ---- + + // The typed accessors were `(T)GetValue(ordinal)`, so they saw ConvertValue(object, ...). De-boxing them + // would have switched them to ConvertValue, which is observable to a converter whose two overloads + // disagree — so with a converter configured they stay on the boxed route. GetFieldValue is unaffected: + // it already called ConvertValue. This pins both halves of that decision. + [Test] + public async Task WithConverter_TypedAccessorUsesObjectOverloadWhileGetFieldValueUsesGeneric() + { + var settings = TestUtilities.GetTestClickHouseClientSettings(); + settings = new ClickHouseClientSettings(settings) { ReadValueConverter = new ObjectOnlyDoublingConverter() }; + using var client = new ClickHouseClient(settings); + + using var reader = await client.ExecuteReaderAsync("SELECT toInt64(21) AS c"); + Assert.That(reader.Read(), Is.True); + + Assert.Multiple(() => + { + Assert.That(reader.GetInt64(0), Is.EqualTo(42L), "GetInt64 must still route through ConvertValue(object, ...)"); + Assert.That(reader.GetValue(0), Is.EqualTo(42L)); + Assert.That(reader.GetFieldValue(0), Is.EqualTo(21L), "GetFieldValue routes through ConvertValue, which this converter leaves alone"); + }); + } + + // Doubles longs in the object overload only, so which overload an accessor picks is directly observable. + private sealed class ObjectOnlyDoublingConverter : IReadValueConverter + { + public object ConvertValue(object value, string columnName, string clickHouseType) + => value is long l ? l * 2 : value; + + public T ConvertValue(T value, string columnName, string clickHouseType) => value; + } + // Reflection is the only way to parametrize over T. Unwraps TargetInvocationException so the assertions // above see the exception the caller would actually get. private static object GetFieldValueDynamic(ClickHouseDataReader reader, Type target) diff --git a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAllocationTests.cs b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAllocationTests.cs new file mode 100644 index 000000000..3d29e3cbd --- /dev/null +++ b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAllocationTests.cs @@ -0,0 +1,139 @@ +using System; +using System.IO; +using System.Net; +using System.Net.Http; +using System.Threading.Tasks; +using ClickHouse.Driver.ADO.Readers; +using ClickHouse.Driver.Formats; +using ClickHouse.Driver.Types; + +namespace ClickHouse.Driver.Tests.ADO; + +/// +/// The regression guard for the typed-column-slot reader. Every other test would still pass if boxing crept +/// back in — the values would be identical, only the garbage would return — so this is the one that fails. +/// +/// Deliberately server-free and synchronous: the payload is a pre-built RowBinaryWithNamesAndTypes +/// buffer served from a , so Read() and the accessors run entirely on +/// the test thread and is exact rather than a sample of +/// whatever else the process is doing. +/// +[TestFixture] +[NonParallelizable] +public class BoxFreeReaderAllocationTests +{ + private const int Rows = 5000; + private const int Columns = 4; + + // Four value-type columns, all with typed readers, none of whose decode allocates. That isolates the + // measurement to boxing: anything left is the thing under test. + private static readonly string[] ColumnTypes = ["Int64", "Float64", "Int32", "UInt64"]; + + private static byte[] BuildPayload() + { + var types = Array.ConvertAll(ColumnTypes, t => TypeConverter.ParseClickHouseType(t, TypeSettings.Default)); + + using var stream = new MemoryStream(); + using var writer = new ExtendedBinaryWriter(stream); + + writer.Write7BitEncodedInt(ColumnTypes.Length); + for (var i = 0; i < ColumnTypes.Length; i++) + writer.Write($"c{i}"); + foreach (var name in ColumnTypes) + writer.Write(name); + + for (var row = 0; row < Rows; row++) + { + types[0].Write(writer, (long)row); + types[1].Write(writer, (double)row); + types[2].Write(writer, row); + types[3].Write(writer, (ulong)row); + } + + writer.Flush(); + return stream.ToArray(); + } + + private static Task CreateReaderAsync(byte[] payload) + => ClickHouseDataReader.FromHttpResponseAsync( + new HttpResponseMessage(HttpStatusCode.OK) { Content = new ByteArrayContent(payload) }, + TypeSettings.Default); + + // Returns bytes allocated while draining the reader. Sums the values into `checksum` so nothing the + // accessors produce can be optimized away as dead. + private static long Measure(ClickHouseDataReader reader, Func readRow, out double checksum) + { + var sum = 0d; + var before = GC.GetAllocatedBytesForCurrentThread(); + var rows = 0; + while (reader.Read()) + { + sum += readRow(reader); + rows++; + } + var allocated = GC.GetAllocatedBytesForCurrentThread() - before; + + Assert.That(rows, Is.EqualTo(Rows), "the payload did not decode to the expected row count"); + checksum = sum; + return allocated; + } + + private static double Scan(ClickHouseDataReader reader) => 0d; + + private static double ReadTyped(ClickHouseDataReader reader) + => reader.GetInt64(0) + reader.GetDouble(1) + reader.GetInt32(2) + reader.GetUInt64(3); + + private static double ReadFieldValue(ClickHouseDataReader reader) + => reader.GetFieldValue(0) + reader.GetFieldValue(1) + + reader.GetFieldValue(2) + reader.GetFieldValue(3); + + private static double ReadBoxed(ClickHouseDataReader reader) + => (long)reader.GetValue(0) + (double)reader.GetValue(1) + + (int)reader.GetValue(2) + (ulong)reader.GetValue(3); + + [Test] + public async Task Read_ValueTypeColumns_AllocatesNothingPerRow() + { + var payload = BuildPayload(); + + // First pass per shape pays for JIT, generic instantiation and the slot factory's one-off reflection. + // None of that is per-row, and none of it should be attributed to the measurement. + foreach (var warmup in new[] { Scan, ReadTyped, ReadFieldValue, ReadBoxed }) + { + using var reader = await CreateReaderAsync(payload); + Measure(reader, warmup, out _); + } + + long scan, typed, fieldValue, boxed; + using (var reader = await CreateReaderAsync(payload)) + scan = Measure(reader, Scan, out _); + using (var reader = await CreateReaderAsync(payload)) + typed = Measure(reader, ReadTyped, out _); + using (var reader = await CreateReaderAsync(payload)) + fieldValue = Measure(reader, ReadFieldValue, out _); + using (var reader = await CreateReaderAsync(payload)) + boxed = Measure(reader, ReadBoxed, out _); + + TestContext.Out.WriteLine( + $"scan={PerRow(scan)} typed={PerRow(typed)} fieldValue={PerRow(fieldValue)} boxed={PerRow(boxed)} (bytes/row)"); + + // One box per value-type cell, and a box is 24 bytes on 64-bit. Requiring even half of that is a wide + // margin against allocation the reader does for other reasons, while still failing outright if any of + // the three de-boxed paths starts boxing again. + const long BoxedFloor = Rows * Columns * 12; + + Assert.Multiple(() => + { + // The headline: Read() used to box every cell of every row whether or not anyone looked at it. + Assert.That(scan, Is.LessThan(Rows), $"Read() must not allocate per row, saw {PerRow(scan)} B/row"); + Assert.That(typed, Is.LessThan(Rows), $"typed accessors must not box, saw {PerRow(typed)} B/row"); + Assert.That(fieldValue, Is.LessThan(Rows), $"GetFieldValue must not box, saw {PerRow(fieldValue)} B/row"); + + // And the control: the untyped path still boxes, so the comparison above is measuring something. + Assert.That(boxed, Is.GreaterThan(BoxedFloor), + $"GetValue is expected to still box; saw only {PerRow(boxed)} B/row, so this test is not measuring boxing"); + }); + } + + private static string PerRow(long allocated) => (allocated / (double)Rows).ToString("F1", System.Globalization.CultureInfo.InvariantCulture); +} diff --git a/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs b/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs index 6890cc0de..37fd61c69 100644 --- a/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs +++ b/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs @@ -160,30 +160,65 @@ internal ClickHouseType GetEffectiveClickHouseType(int ordinal) private protected ClickHouseType[] RawTypes { get; set; } - public override bool GetBoolean(int ordinal) => Convert.ToBoolean(GetValue(ordinal), CultureInfo.InvariantCulture); + /// + /// Shared body for the strict typed accessors — every one of which was (T)GetValue(ordinal) before + /// column slots, and keeps exactly that meaning here. This is the path compiled ORM mappers drive + /// (linq2db registers GetInt64/GetDouble/GetDateTime/… and inlines them per column + /// per row), so it is where the boxing elimination is worth the most. + /// + /// + /// With an configured the accessor keeps routing through + /// , so it still calls ConvertValue(object, …) exactly as it did before. + /// De-boxing here would mean calling ConvertValue<T> instead, which is observable to a + /// converter whose two overloads disagree — the generic one matching on typeof(T), the object one + /// on the value's runtime type. Not worth a silent semantic change for the rare converter case; + /// everyone else gets the fast path. is unaffected either way, because it + /// already called ConvertValue<T>. + /// + private T GetTypedValue(int ordinal) + => readValueConverter == null ? GetSlotValue(ordinal) : (T)GetValue(ordinal); + + // Unlike its neighbours this one coerces rather than casts, so only an exact Bool column can take the + // fast path; anything else keeps Convert.ToBoolean's widening (and its exception messages). + public override bool GetBoolean(int ordinal) + => readValueConverter == null && slots[ordinal] is ValueSlot boolSlot + ? boolSlot.Value + : Convert.ToBoolean(GetValue(ordinal), CultureInfo.InvariantCulture); - public override byte GetByte(int ordinal) => (byte)GetValue(ordinal); + public override byte GetByte(int ordinal) => GetTypedValue(ordinal); public override long GetBytes(int ordinal, long dataOffset, byte[] buffer, int bufferOffset, int length) => throw new NotImplementedException(); + // No ClickHouse type reads as char, so there is no slot to hit — left on the boxed cast. public override char GetChar(int ordinal) => (char)GetValue(ordinal); public override long GetChars(int ordinal, long dataOffset, char[] buffer, int bufferOffset, int length) => throw new NotImplementedException(); public override string GetDataTypeName(int ordinal) => GetClickHouseType(ordinal).ToString(); - public override DateTime GetDateTime(int ordinal) => (DateTime)GetValue(ordinal); + public override DateTime GetDateTime(int ordinal) => GetTypedValue(ordinal); + // Box-free by construction once GetDateTime is: CoerceToDateTimeOffset has a DateTime overload. public virtual DateTimeOffset GetDateTimeOffset(int ordinal) => GetEffectiveClickHouseType(ordinal) is AbstractDateTimeType adt ? adt.CoerceToDateTimeOffset(GetDateTime(ordinal)) : throw new InvalidCastException(); public override decimal GetDecimal(int ordinal) { + if (readValueConverter == null) + { + // Which of these two a Decimal column resolves to is the UseBigDecimal setting's doing; both + // reach the same decimal without a box. + if (slots[ordinal] is ValueSlot decimalSlot) + return decimalSlot.Value; + if (slots[ordinal] is ValueSlot bigDecimalSlot) + return bigDecimalSlot.Value.ToDecimal(CultureInfo.InvariantCulture); + } + var value = GetValue(ordinal); return value is ClickHouseDecimal clickHouseDecimal ? clickHouseDecimal.ToDecimal(CultureInfo.InvariantCulture) : (decimal)value; } - public override double GetDouble(int ordinal) => (double)GetValue(ordinal); + public override double GetDouble(int ordinal) => GetTypedValue(ordinal); public override Type GetFieldType(int ordinal) { @@ -191,15 +226,15 @@ public override Type GetFieldType(int ordinal) return rawType is NullableType nt ? nt.UnderlyingType.FrameworkType : rawType.FrameworkType; } - public override float GetFloat(int ordinal) => (float)GetValue(ordinal); + public override float GetFloat(int ordinal) => GetTypedValue(ordinal); - public override Guid GetGuid(int ordinal) => (Guid)GetValue(ordinal); + public override Guid GetGuid(int ordinal) => GetTypedValue(ordinal); - public override short GetInt16(int ordinal) => (short)GetValue(ordinal); + public override short GetInt16(int ordinal) => GetTypedValue(ordinal); - public override int GetInt32(int ordinal) => (int)GetValue(ordinal); + public override int GetInt32(int ordinal) => GetTypedValue(ordinal); - public override long GetInt64(int ordinal) => (long)GetValue(ordinal); + public override long GetInt64(int ordinal) => GetTypedValue(ordinal); public override string GetName(int ordinal) => FieldNames[ordinal]; @@ -214,7 +249,13 @@ public override int GetOrdinal(string name) return index; } - public override string GetString(int ordinal) => GetValue(ordinal)?.ToString(); + // Deliberately narrower than the other accessors: only a non-nullable String column short-circuits. Every + // other shape keeps ToString()'s coercion, including the quirk that a NULL cell yields "" rather than + // null, because DBNull.Value.ToString() is the empty string. + public override string GetString(int ordinal) + => readValueConverter == null && slots[ordinal] is ValueSlot stringSlot + ? stringSlot.Value + : GetValue(ordinal)?.ToString(); /// /// The one boxing entry point on the read path. Boxes lazily, per call, so a query that projects ten @@ -360,25 +401,25 @@ private static class FieldValueDispatcher public override Task NextResultAsync(CancellationToken cancellationToken) => Task.FromResult(false); // Custom extension - public ushort GetUInt16(int ordinal) => (ushort)GetValue(ordinal); + public ushort GetUInt16(int ordinal) => GetTypedValue(ordinal); // Custom extension - public uint GetUInt32(int ordinal) => (uint)GetValue(ordinal); + public uint GetUInt32(int ordinal) => GetTypedValue(ordinal); // Custom extension - public ulong GetUInt64(int ordinal) => (ulong)GetValue(ordinal); + public ulong GetUInt64(int ordinal) => GetTypedValue(ordinal); // Custom extension - public IPAddress GetIPAddress(int ordinal) => (IPAddress)GetValue(ordinal); + public IPAddress GetIPAddress(int ordinal) => GetTypedValue(ordinal); - // Custom extension + // Custom extension. Tuple columns have no typed slot, so this stays on the boxed cast. public ITuple GetTuple(int ordinal) => (ITuple)GetValue(ordinal); // Custom extension - public sbyte GetSByte(int ordinal) => (sbyte)GetValue(ordinal); + public sbyte GetSByte(int ordinal) => GetTypedValue(ordinal); // Custom extension - public BigInteger GetBigInteger(int ordinal) => (BigInteger)GetValue(ordinal); + public BigInteger GetBigInteger(int ordinal) => GetTypedValue(ordinal); /// /// Materializes the current row into a new instance of . From 3ee53de8ffae1383d118dbdf8142edecd44809df Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Sun, 2 Aug 2026 12:48:58 +0200 Subject: [PATCH 05/16] bench(ado): add the ADO read-path benchmark MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measures the four access patterns the slot change affects — Read()-only scan, typed accessors, GetFieldValue, and untyped GetValue — over a 10-column shape, so the allocation claim can be reproduced rather than taken on trust. Also covers the accessors and slot shapes the benchmark exercises. Co-Authored-By: Claude --- .../AdoReadPathBenchmark.cs | 133 ++++++++++++++++++ .../ADO/BoxFreeReaderAccessorTests.cs | 14 ++ .../ADO/ColumnSlotTests.cs | 31 ++++ 3 files changed, 178 insertions(+) create mode 100644 ClickHouse.Driver.Benchmark/AdoReadPathBenchmark.cs diff --git a/ClickHouse.Driver.Benchmark/AdoReadPathBenchmark.cs b/ClickHouse.Driver.Benchmark/AdoReadPathBenchmark.cs new file mode 100644 index 000000000..f9b37b4d8 --- /dev/null +++ b/ClickHouse.Driver.Benchmark/AdoReadPathBenchmark.cs @@ -0,0 +1,133 @@ +using System; +using System.Threading.Tasks; +using BenchmarkDotNet.Attributes; +using BenchmarkDotNet.Engines; +using ClickHouse.Driver.ADO; +using ClickHouse.Driver.ADO.Readers; +using ClickHouse.Driver.Utility; + +namespace ClickHouse.Driver.Benchmark; + +/// +/// The four ways a caller drives , over one realistic wide row. +/// +/// measures single-column reads with and without a converter, which +/// isolates per-accessor cost but hides the thing typed column slots actually changed: how much a row costs +/// to decode before anyone looks at it, and how that scales with the fraction of columns read. The +/// old object[] row buffer boxed every cell during Read(), so all five variants below allocated +/// identically — even , which reads nothing. +/// +/// +/// — the floor: decode cost with no accessor calls at all. +/// — the linq2db path. Its compiled mapper inlines +/// GetInt64/GetDouble/GetString/GetDateTime/GetGuid per column per row. +/// — hand-written GetFieldValue<T> code. +/// — the Dapper path. Its emitted IL calls the this[int] indexer, +/// i.e. GetValue, so it still boxes; this variant is the "must not regress" control. +/// — reads 2 of 10 columns, the case the old eager boxing +/// punished hardest. +/// +/// +[Config(typeof(ComparisonConfig))] +[MemoryDiagnoser(true)] +public class AdoReadPathBenchmark +{ + private readonly Consumer consumer = new(); + private ClickHouseConnection connection; + + [Params(200000)] + public int Count { get; set; } + + // Ten columns, eight of them value types — the shape that used to box eight times per row. + private string Sql => $@" +SELECT toInt64(number) AS c0, + toInt64(number * 2) AS c1, + toInt64(number * 3) AS c2, + toInt64(number * 5) AS c3, + toFloat64(number) * 0.5 AS c4, + toFloat64(number) * 1.5 AS c5, + concat('s', toString(number % 8)) AS c6, + concat('t', toString(number % 4)) AS c7, + toDateTime(1700000000 + (number % 65536), 'UTC') AS c8, + toUUID(concat('00000000-0000-0000-0000-', leftPad(toString(number % 1000), 12, '0'))) AS c9 +FROM system.numbers LIMIT {Count}"; + + [GlobalSetup] + public void Setup() + { + var connectionString = Environment.GetEnvironmentVariable("CLICKHOUSE_CONNECTION") ?? "Host=localhost"; + connection = new ClickHouseConnection(new ClickHouseClientSettings(connectionString)); + } + + [GlobalCleanup] + public void Cleanup() => connection?.Dispose(); + + [Benchmark(Baseline = true)] + public async Task UntypedAccessor() + { + using var reader = await connection.ExecuteReaderAsync(Sql); + while (reader.Read()) + { + for (var i = 0; i < 10; i++) + consumer.Consume(reader.GetValue(i)); + } + } + + [Benchmark] + public async Task Scan() + { + using var reader = await connection.ExecuteReaderAsync(Sql); + while (reader.Read()) + { + } + } + + [Benchmark] + public async Task TypedAccessors() + { + using var reader = (ClickHouseDataReader)await connection.ExecuteReaderAsync(Sql); + while (reader.Read()) + { + consumer.Consume(reader.GetInt64(0)); + consumer.Consume(reader.GetInt64(1)); + consumer.Consume(reader.GetInt64(2)); + consumer.Consume(reader.GetInt64(3)); + consumer.Consume(reader.GetDouble(4)); + consumer.Consume(reader.GetDouble(5)); + consumer.Consume(reader.GetString(6)); + consumer.Consume(reader.GetString(7)); + consumer.Consume(reader.GetDateTime(8)); + consumer.Consume(reader.GetGuid(9)); + } + } + + [Benchmark] + public async Task GenericAccessor() + { + using var reader = (ClickHouseDataReader)await connection.ExecuteReaderAsync(Sql); + while (reader.Read()) + { + consumer.Consume(reader.GetFieldValue(0)); + consumer.Consume(reader.GetFieldValue(1)); + consumer.Consume(reader.GetFieldValue(2)); + consumer.Consume(reader.GetFieldValue(3)); + consumer.Consume(reader.GetFieldValue(4)); + consumer.Consume(reader.GetFieldValue(5)); + consumer.Consume(reader.GetFieldValue(6)); + consumer.Consume(reader.GetFieldValue(7)); + consumer.Consume(reader.GetFieldValue(8)); + consumer.Consume(reader.GetFieldValue(9)); + } + } + + [Benchmark] + public async Task TypedAccessorsProjected() + { + using var reader = (ClickHouseDataReader)await connection.ExecuteReaderAsync(Sql); + while (reader.Read()) + { + consumer.Consume(reader.GetInt64(0)); + consumer.Consume(reader.GetString(6)); + } + } +} diff --git a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs index 0385590ad..270a7d520 100644 --- a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs +++ b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs @@ -358,6 +358,20 @@ public async Task WithConverter_TypedAccessorUsesObjectOverloadWhileGetFieldValu }); } + // GetDecimal's fast path is skipped when a converter is configured, so this exercises its boxed + // fallback — including the ClickHouseDecimal branch, which is what a Decimal column boxes as by default. + [Test] + public async Task GetDecimal_WithConverter_FallsBackToTheBoxedPath() + { + var settings = TestUtilities.GetTestClickHouseClientSettings(); + settings = new ClickHouseClientSettings(settings) { ReadValueConverter = new ObjectOnlyDoublingConverter() }; + using var client = new ClickHouseClient(settings); + + using var reader = await client.ExecuteReaderAsync("SELECT toDecimal64(12.34, 2) AS c"); + Assert.That(reader.Read(), Is.True); + Assert.That(reader.GetDecimal(0), Is.EqualTo(12.34m)); + } + // Doubles longs in the object overload only, so which overload an accessor picks is directly observable. private sealed class ObjectOnlyDoublingConverter : IReadValueConverter { diff --git a/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs b/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs index be7259c78..01b5047f8 100644 --- a/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs +++ b/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs @@ -231,6 +231,37 @@ public void Create_DecimalColumn_BindsToTheRepresentationTheSettingsSelect(bool Assert.That(ColumnSlotFactory.Create(Parse("Decimal(10, 2)", settings)), Is.TypeOf(expected)); } + // Object(...) is wire-transparent too, but the grammar never yields an ObjectType instance — + // ObjectType.Parse returns a SimpleAggregateFunctionType — so the only way to reach that unwrap branch is + // to construct one. Worth pinning: if Parse is ever corrected, this is already covered. + [Test] + public void Create_ObjectWrappedColumn_ResolvesToWrappedTypedSlot() + { + var type = new ObjectType { UnderlyingType = new Int64Type() }; + Assert.That(ColumnSlotFactory.Create(type), Is.TypeOf>()); + } + + // The factory's pre-filter finds the ITypedReader<> interfaces a class implements; the binding rule is + // narrower than that — the reader has to be for the column's own FrameworkType, or the slot's GetBoxed() + // would hand back a different CLR type than the boxed Read did. No shipped type is shaped this way, so + // only a purpose-built one can prove the factory declines rather than mis-binding. + [Test] + public void Create_TypeWhoseTypedReaderIsNotItsFrameworkType_FallsBackToBoxed() + => Assert.That(ColumnSlotFactory.Create(new MismatchedRepresentationType()), Is.TypeOf()); + + private sealed class MismatchedRepresentationType : ClickHouseType, ITypedReader + { + public override Type FrameworkType => typeof(string); + + public override object Read(ExtendedBinaryReader reader) => reader.ReadString(); + + public int ReadValue(ExtendedBinaryReader reader) => reader.ReadInt32(); + + public override void Write(ExtendedBinaryWriter writer, object value) => throw new NotSupportedException(); + + public override string ToString() => nameof(MismatchedRepresentationType); + } + // ---- IsNull: no boxed counterpart, so parity cannot cover it ---- [Test] From 049c0ab6d92b15e796681153e95a86cbb1580348 Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Sun, 2 Aug 2026 13:24:33 +0200 Subject: [PATCH 06/16] test(ado): guard the slot factory against evaluating FrameworkType too early MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AggregateFunctionType throws AggregateFunctionException from FrameworkType, Read and ToString, deliberately: you are meant to learn you need xMerge() when you read the value, not before. Slots are now built for every column when the reader is constructed, so ColumnSlotFactory.TryCreateTyped has to reach its no-typed-reader bail-out without ever touching FrameworkType — hoisting that read above the bail-out for readability would turn merely *selecting* an AggregateFunction column into a failure to open the reader at all. The current ordering is correct; this only documents why it is load-bearing and pins it, at both the factory level and end to end. Co-Authored-By: Claude --- .../ADO/BoxFreeReaderAccessorTests.cs | 15 +++++++++++++++ ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs | 13 +++++++++++++ .../ADO/Readers/ColumnSlotFactory.cs | 6 ++++++ 3 files changed, 34 insertions(+) diff --git a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs index 270a7d520..78b321d9b 100644 --- a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs +++ b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs @@ -2,6 +2,7 @@ using System.Threading.Tasks; using ClickHouse.Driver.ADO; using ClickHouse.Driver.ADO.Readers; +using ClickHouse.Driver.Types; using ClickHouse.Driver.Utility; namespace ClickHouse.Driver.Tests.ADO; @@ -116,6 +117,20 @@ public async Task GetFieldValue_CompositeColumn_ReadsThroughBoxedFallback() Assert.That(reader.GetFieldValue(0), Is.EqualTo(new[] { 1, 2, 3 })); } + // End-to-end counterpart to ColumnSlotTests' factory guard: an AggregateFunction column must still open a + // reader and fail only where it always did — on the value, with the message that tells you to use + // xMerge(). Building a slot per column at construction time is what puts that at risk. + [Test] + public async Task Read_AggregateFunctionColumn_OpensTheReaderAndFailsOnlyOnTheValue() + { + using var reader = (ClickHouseDataReader)await connection.ExecuteReaderAsync( + "SELECT quantileState(0.5)(number) AS c FROM numbers(10)"); + + Assert.That(reader.FieldCount, Is.EqualTo(1)); + var ex = Assert.Throws(() => reader.Read()); + Assert.That(ex.Message, Does.Contain("Merge()")); + } + // ---- IsDBNull now answers from the slot's presence flag rather than inspecting a boxed value ---- [Test] diff --git a/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs b/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs index 01b5047f8..784dc3c49 100644 --- a/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs +++ b/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs @@ -241,6 +241,19 @@ public void Create_ObjectWrappedColumn_ResolvesToWrappedTypedSlot() Assert.That(ColumnSlotFactory.Create(type), Is.TypeOf>()); } + // AggregateFunctionType throws from FrameworkType (and from Read and ToString) so that you learn you need + // xMerge() when you read the value. Slots are built for every column when the reader is constructed, so + // the factory must reach its no-typed-reader bail-out without ever evaluating FrameworkType — otherwise + // merely selecting such a column would fail to open the reader at all. Guards the ordering in + // TryCreateTyped, which is otherwise easy to "tidy up" into a regression. + [Test] + public void Create_AggregateFunctionColumn_FallsBackToBoxedWithoutEvaluatingFrameworkType() + { + var type = Parse("AggregateFunction(quantile(0.5), UInt64)"); + Assert.That(() => ColumnSlotFactory.Create(type), Throws.Nothing); + Assert.That(ColumnSlotFactory.Create(type), Is.TypeOf()); + } + // The factory's pre-filter finds the ITypedReader<> interfaces a class implements; the binding rule is // narrower than that — the reader has to be for the column's own FrameworkType, or the slot's GetBoxed() // would hand back a different CLR type than the boxed Read did. No shipped type is shaped this way, so diff --git a/ClickHouse.Driver/ADO/Readers/ColumnSlotFactory.cs b/ClickHouse.Driver/ADO/Readers/ColumnSlotFactory.cs index 23f4d7d95..208e595e6 100644 --- a/ClickHouse.Driver/ADO/Readers/ColumnSlotFactory.cs +++ b/ClickHouse.Driver/ADO/Readers/ColumnSlotFactory.cs @@ -58,6 +58,12 @@ public static ColumnSlot Create(ClickHouseType type) private static ColumnSlot TryCreateTyped(ClickHouseType type, bool nullable) { + // Order matters, and not only for speed: FrameworkType is not safe to evaluate on every column type. + // AggregateFunctionType throws AggregateFunctionException from it (deliberately — you are meant to + // learn you need xMerge() when you read the value, not when you open the reader), and the composite + // types build a fresh Type object on each call. Slots are created for every column in the ctor, so + // hoisting this read above the no-typed-reader bail-out would turn merely *selecting* an + // AggregateFunction column into a failure to construct the reader at all. var targets = TypedReadTargets.GetOrAdd(type.GetType(), FindTypedReadTargets); if (targets.Length == 0) return null; From bb8dde944cdb8a3c020c6ac8588fe5c8ec585166 Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Sun, 2 Aug 2026 13:51:19 +0200 Subject: [PATCH 07/16] fix(ado)!: throw when a column value is read with no current row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With the old object[] row buffer, reading before the first Read() returned null from GetValue, true from IsDBNull, and threw NullReferenceException from a typed accessor — accidental, but loud. Typed slots hold typed storage, so the same call would instead have returned a perfectly ordinary 0 / false / Guid.Empty for a non-nullable value column: a wrong answer indistinguishable from real data, where before there was a crash. Every value accessor now goes through one gate on the existing hasCurrentRow flag and throws InvalidOperationException: GetValue, both indexers, GetValues, GetFieldValue, IsDBNull and the typed accessors. This matches SqlClient and the rest of ADO.NET. Column metadata (FieldCount, GetName, GetOrdinal, GetFieldType, GetDataTypeName, GetSchemaTable) is deliberately not gated — a caller inspecting the shape of an empty result set needs it, and DataTable.Load asks for it before its first Read(). The gate also covers reads after Read() has returned false, which previously kept serving the last row. That is the one place this is stricter than both the old and the intermediate behaviour, and it caught a real defect in the test harness: GetEnsureSingleRow built its "unexpected extra row" message inline as an argument to IsFalse, so it read the row after Read() had already returned false — on every single call, allocating a joined string of every column value purely to discard it. Now built only when there is an extra row. That was the only call site in the solution. Verified across net6.0/8.0/9.0/10.0 (9906-9923 passed, 0 failed) plus the linq2db integration test. Allocation is unchanged: still 0 B/row for Read(), the typed accessors and GetFieldValue. Co-Authored-By: Claude --- .../ADO/BoxFreeReaderAccessorTests.cs | 66 +++++++++++++++++++ .../Utilities/TestUtilities.cs | 7 +- .../ADO/Readers/ClickHouseDataReader.cs | 51 +++++++++++--- 3 files changed, 115 insertions(+), 9 deletions(-) diff --git a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs index 78b321d9b..7f0cb80dc 100644 --- a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs +++ b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Threading.Tasks; using ClickHouse.Driver.ADO; using ClickHouse.Driver.ADO.Readers; @@ -131,6 +132,71 @@ public async Task Read_AggregateFunctionColumn_OpensTheReaderAndFailsOnlyOnTheVa Assert.That(ex.Message, Does.Contain("Merge()")); } + // ---- No current row ---- + // + // Slots hold typed storage, so without this guard a non-nullable value column would read back as a + // perfectly plausible 0 / false / Guid.Empty before the first Read() — data-shaped, and indistinguishable + // from a real value. (The old object[] buffer started all-null, so GetValue returned null and a typed + // accessor threw NullReferenceException.) Every value accessor now reports the mistake instead. + + private static IEnumerable ValueAccessors() + { + yield return Accessor("GetValue", r => r.GetValue(0)); + yield return Accessor("Indexer", r => r[0]); + yield return Accessor("IndexerByName", r => r["a"]); + yield return Accessor("GetValues", r => r.GetValues(new object[3])); + yield return Accessor("GetFieldValue", r => r.GetFieldValue(0)); + yield return Accessor("IsDBNull", r => r.IsDBNull(0)); + yield return Accessor("GetInt64", r => r.GetInt64(0)); + yield return Accessor("GetString", r => r.GetString(1)); + yield return Accessor("GetBoolean", r => r.GetBoolean(0)); + yield return Accessor("GetDecimal", r => r.GetDecimal(2)); + yield return Accessor("GetDateTime", r => r.GetDateTime(0)); + + static TestCaseData Accessor(string name, Func read) + => new TestCaseData(read).SetArgDisplayNames(name); + } + + private const string ThreeColumns = "toInt64(1) AS a, 's' AS b, toDecimal64(1.5, 2) AS c"; + + [TestCaseSource(nameof(ValueAccessors))] + public async Task ValueAccessor_BeforeFirstRead_ThrowsInvalidOperation(Func read) + { + using var reader = (ClickHouseDataReader)await connection.ExecuteReaderAsync($"SELECT {ThreeColumns}"); + + var ex = Assert.Throws(() => read(reader)); + Assert.That(ex.Message, Does.Contain("Read()")); + } + + [TestCaseSource(nameof(ValueAccessors))] + public async Task ValueAccessor_AfterReadReturnsFalse_ThrowsInvalidOperation(Func read) + { + using var reader = (ClickHouseDataReader)await connection.ExecuteReaderAsync($"SELECT {ThreeColumns}"); + Assert.That(reader.Read(), Is.True); + Assert.That(reader.Read(), Is.False); + + Assert.Throws(() => read(reader)); + } + + // Column metadata does not depend on a row and must stay reachable — this is what a caller inspecting the + // shape of an empty result set needs, and what DataTable.Load asks for before its first Read(). + [Test] + public async Task ColumnMetadata_WithNoCurrentRow_IsStillAvailable() + { + using var reader = (ClickHouseDataReader)await connection.ExecuteReaderAsync( + $"SELECT {ThreeColumns} FROM system.numbers WHERE 0"); + + Assert.Multiple(() => + { + Assert.That(reader.FieldCount, Is.EqualTo(3)); + Assert.That(reader.GetName(0), Is.EqualTo("a")); + Assert.That(reader.GetOrdinal("b"), Is.EqualTo(1)); + Assert.That(reader.GetFieldType(0), Is.EqualTo(typeof(long))); + Assert.That(reader.GetDataTypeName(0), Is.EqualTo("Int64")); + Assert.That(reader.GetSchemaTable().Rows, Has.Count.EqualTo(3)); + }); + } + // ---- IsDBNull now answers from the slot's presence flag rather than inspecting a boxed value ---- [Test] diff --git a/ClickHouse.Driver.Tests/Utilities/TestUtilities.cs b/ClickHouse.Driver.Tests/Utilities/TestUtilities.cs index 8a3b19097..4243014a1 100644 --- a/ClickHouse.Driver.Tests/Utilities/TestUtilities.cs +++ b/ClickHouse.Driver.Tests/Utilities/TestUtilities.cs @@ -375,7 +375,12 @@ public static object[] GetEnsureSingleRow(this DbDataReader reader) var data = reader.GetFieldValues(); - ClassicAssert.IsFalse(reader.Read(), "Unexpected extra row: " + string.Join(",", reader.GetFieldValues())); + // Read the extra row's values only once there is one. The argument to IsFalse is evaluated eagerly, + // so inlining GetFieldValues() there read the row *after* Read() had returned false — which the + // reader now rejects — and allocated a joined string of every column on every successful call just + // to discard it. + if (reader.Read()) + Assert.Fail("Unexpected extra row: " + string.Join(",", reader.GetFieldValues())); return data; } diff --git a/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs b/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs index 37fd61c69..6b6ad4908 100644 --- a/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs +++ b/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs @@ -181,7 +181,7 @@ private T GetTypedValue(int ordinal) // Unlike its neighbours this one coerces rather than casts, so only an exact Bool column can take the // fast path; anything else keeps Convert.ToBoolean's widening (and its exception messages). public override bool GetBoolean(int ordinal) - => readValueConverter == null && slots[ordinal] is ValueSlot boolSlot + => readValueConverter == null && Slot(ordinal) is ValueSlot boolSlot ? boolSlot.Value : Convert.ToBoolean(GetValue(ordinal), CultureInfo.InvariantCulture); @@ -208,7 +208,7 @@ public override decimal GetDecimal(int ordinal) { // Which of these two a Decimal column resolves to is the UseBigDecimal setting's doing; both // reach the same decimal without a box. - if (slots[ordinal] is ValueSlot decimalSlot) + if (Slot(ordinal) is ValueSlot decimalSlot) return decimalSlot.Value; if (slots[ordinal] is ValueSlot bigDecimalSlot) return bigDecimalSlot.Value.ToDecimal(CultureInfo.InvariantCulture); @@ -253,7 +253,7 @@ public override int GetOrdinal(string name) // other shape keeps ToString()'s coercion, including the quirk that a NULL cell yields "" rather than // null, because DBNull.Value.ToString() is the empty string. public override string GetString(int ordinal) - => readValueConverter == null && slots[ordinal] is ValueSlot stringSlot + => readValueConverter == null && Slot(ordinal) is ValueSlot stringSlot ? stringSlot.Value : GetValue(ordinal)?.ToString(); @@ -268,12 +268,18 @@ public override string GetString(int ordinal) /// (the ADO.NET-relevant comparison) but no longer by . /// public override object GetValue(int ordinal) - => readValueConverter == null - ? slots[ordinal].GetBoxed() - : readValueConverter.ConvertValue(slots[ordinal].GetBoxed(), FieldNames[ordinal], columnTypeNames[ordinal]); + { + var value = Slot(ordinal).GetBoxed(); + return readValueConverter == null + ? value + : readValueConverter.ConvertValue(value, FieldNames[ordinal], columnTypeNames[ordinal]); + } public override int GetValues(object[] values) { + if (!hasCurrentRow) + ThrowNoCurrentRow(); + var count = Math.Min(slots.Length, values.Length); if (readValueConverter != null) @@ -294,7 +300,36 @@ public override bool IsDBNull(int ordinal) // Asks the slot directly rather than going through GetValue, for two reasons: a configured // IReadValueConverter must not run during a null check (it could throw, do expensive work, or // change the nullness of the result), and a null check has no business materializing a box. - => slots[ordinal].IsNull; + => Slot(ordinal).IsNull; + + /// + /// The single gate every value accessor passes through. Column metadata (, + /// , , , …) is available + /// without a current row and is deliberately not gated. + /// + /// + /// Slots hold typed storage, so before the first a non-nullable value column would + /// otherwise read back as a perfectly plausible 0 / false / Guid.Empty rather than + /// as nothing. Answering a question the reader cannot yet answer, with a value indistinguishable from + /// real data, is the one failure mode worth spending a branch to prevent — so this reports the mistake + /// instead, matching what SqlClient and the rest of ADO.NET do. + /// + private ColumnSlot Slot(int ordinal) + { + if (!hasCurrentRow) + ThrowNoCurrentRow(); + + return slots[ordinal]; + } + + // Separate and non-inlined so the check above stays small enough for the JIT to inline into the hot + // accessors. + [MethodImpl(MethodImplOptions.NoInlining)] + private static void ThrowNoCurrentRow() + => throw new InvalidOperationException( + "The reader has no current row. Call Read() and check that it returned true before reading " + + "column values. Column metadata (FieldCount, GetName, GetFieldType, GetSchemaTable) is " + + "available without a current row."); public override bool NextResult() => false; @@ -367,7 +402,7 @@ public override T GetFieldValue(int ordinal) /// private T GetSlotValue(int ordinal) { - var slot = slots[ordinal]; + var slot = Slot(ordinal); if (slot is ValueSlot valueSlot) return valueSlot.Value; From be6e988bdcc0de6b19a348089100aa720d8767af Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Sun, 2 Aug 2026 14:04:36 +0200 Subject: [PATCH 08/16] refactor(ado): select column slots without runtime generic construction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ColumnSlotFactory reached its slot types through MakeGenericMethod + CreateDelegate, cached per CLR type. That works, but NativeAOT cannot construct generic instantiations over value types that were not rooted at compile time, and this sits on the read path of every scalar column — so the change widened the driver's existing AOT exposure from composite columns to all of them. Replaced with a static table of ValueSlot/NullableSlot constructors, one entry per CLR type any ITypedReader is implemented for. Every instantiation is now emitted by the compiler, so trimming and AOT can see the whole set. The interface-list walk that decided "does this type read box-free at all?" is gone too: ITypedReader now derives from a non-generic ITypedReader marker, so the question is a plain type test. That leaves the factory with no reflection at all, and drops both ConcurrentDictionary caches — the marker test replaces the per-class cache, and the static table replaces the per-CLR-type one. It also makes the ordering constraint self-evident rather than incidental: the marker test is what keeps FrameworkType (which AggregateFunctionType throws from) out of reach until we know the column could match. The cost is that the table no longer maintains itself. Adding an ITypedReader for a new T and forgetting the entry would silently demote that column to the boxed path — correct values, allocation quietly back, and nothing else in the suite would notice. Binders_CoverEveryTypedReadTarget reflects over every ClickHouseType in the assembly and fails with the missing type named. Verified by deleting the Guid entry: it reports "< >". A few table entries are unreachable today — DateTimeOffset, DateOnly and the native Int128/UInt128 are alternative read representations offered alongside a type's FrameworkType, never as it. Listed anyway so the table is exactly "every ITypedReader target" and the completeness check stays mechanical. Over-inclusion is inert; a missing entry is the failure mode that matters. net6.0/8.0/9.0/10.0 green (9907-9924 passed, 0 failed), integration tests green, allocation unchanged at 0 B/row. Co-Authored-By: Claude --- .../ADO/ColumnSlotTests.cs | 29 +++++ .../ADO/Readers/ColumnSlotFactory.cs | 118 +++++++++--------- ClickHouse.Driver/Types/ITypedReader.cs | 12 +- 3 files changed, 100 insertions(+), 59 deletions(-) diff --git a/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs b/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs index 784dc3c49..1baf00324 100644 --- a/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs +++ b/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs @@ -1,9 +1,11 @@ using System; +using System.Collections; using System.Collections.Generic; using System.IO; using System.Linq; using System.Net; using System.Numerics; +using System.Reflection; using ClickHouse.Driver.ADO.Readers; using ClickHouse.Driver.Formats; using ClickHouse.Driver.Numerics; @@ -241,6 +243,33 @@ public void Create_ObjectWrappedColumn_ResolvesToWrappedTypedSlot() Assert.That(ColumnSlotFactory.Create(type), Is.TypeOf>()); } + // The factory dispatches through a hand-written table of ValueSlot/NullableSlot constructors rather + // than MakeGenericMethod, so that NativeAOT and trimming can see every instantiation the reader needs. + // The cost of giving up runtime generic construction is that the table no longer maintains itself: adding + // an ITypedReader for a new T and forgetting the entry would silently demote that column to the boxed + // path — values still correct, allocation quietly back. Nothing else would catch it, so this does. + [Test] + public void Binders_CoverEveryTypedReadTarget() + { + var declared = typeof(ClickHouseType).Assembly + .GetTypes() + .Where(t => typeof(ClickHouseType).IsAssignableFrom(t)) + .SelectMany(t => t.GetInterfaces()) + .Where(i => i.IsGenericType && i.GetGenericTypeDefinition() == typeof(ITypedReader<>)) + .Select(i => i.GetGenericArguments()[0]) + .Distinct() + .ToArray(); + + Assert.That(declared, Is.Not.Empty, "found no ITypedReader implementations at all — check the query"); + + var bound = (IDictionary)typeof(ColumnSlotFactory) + .GetField("Binders", BindingFlags.NonPublic | BindingFlags.Static) + .GetValue(null); + + Assert.That(declared.Where(t => !bound.Contains(t)), Is.Empty, + "every CLR type some ClickHouseType can read box-free needs a ColumnSlotFactory.Binders entry"); + } + // AggregateFunctionType throws from FrameworkType (and from Read and ToString) so that you learn you need // xMerge() when you read the value. Slots are built for every column when the reader is constructed, so // the factory must reach its no-typed-reader bail-out without ever evaluating FrameworkType — otherwise diff --git a/ClickHouse.Driver/ADO/Readers/ColumnSlotFactory.cs b/ClickHouse.Driver/ADO/Readers/ColumnSlotFactory.cs index 208e595e6..3afa59149 100644 --- a/ClickHouse.Driver/ADO/Readers/ColumnSlotFactory.cs +++ b/ClickHouse.Driver/ADO/Readers/ColumnSlotFactory.cs @@ -1,7 +1,8 @@ using System; -using System.Collections.Concurrent; -using System.Linq; -using System.Reflection; +using System.Collections.Generic; +using System.Net; +using System.Numerics; +using ClickHouse.Driver.Numerics; using ClickHouse.Driver.Types; namespace ClickHouse.Driver.ADO.Readers; @@ -17,28 +18,56 @@ namespace ClickHouse.Driver.ADO.Readers; /// path always produce the same value from the same bytes. Anything else gets a . /// /// Slots are built from the resolved type instance, never from a cached shape, so settings that -/// change FrameworkType — ReadStringsAsByteArrays (string vs byte[]), UseBigDecimal +/// change FrameworkType — ReadStringsAsByteArrays (string vs byte[]), UseCustomDecimals /// (decimal vs ClickHouseDecimal) — are handled without any of the cache-key hazards a per-query-shape cache /// would have. /// internal static class ColumnSlotFactory { - private static readonly MethodInfo CreateValueSlotMethod = - typeof(ColumnSlotFactory).GetMethod(nameof(CreateValueSlot), BindingFlags.NonPublic | BindingFlags.Static); - - private static readonly MethodInfo CreateNullableSlotMethod = - typeof(ColumnSlotFactory).GetMethod(nameof(CreateNullableSlot), BindingFlags.NonPublic | BindingFlags.Static); - - // Concrete ClickHouseType class -> the CLR types it can read box-free. Keyed on the class, not the - // instance, because the implemented interface list is fixed per class. Bounded by the number of - // ClickHouseType subclasses (~60), and it is what keeps SlotConstructors below bounded too: without it, - // a composite column's FrameworkType (long[], Dictionary, Tuple<...>, ...) would be an unbounded - // family of keys, none of which could ever produce a typed slot. - private static readonly ConcurrentDictionary TypedReadTargets = new(); - - // CLR type -> closed-generic slot constructors. Bounded by the union of every ITypedReader's T - // (~25 types), because a key only ever gets here after passing the TypedReadTargets check. - private static readonly ConcurrentDictionary ConstructorCache = new(); + /// + /// CLR type → the slot constructor for it. Every entry is a static generic instantiation the compiler + /// emits, rather than a MakeGenericMethod built at runtime, so NativeAOT and trimming can see + /// every ValueSlot<T>/NullableSlot<T> the reader will ever need. Runtime generic + /// construction over value types is exactly what NativeAOT cannot satisfy, and this sits on the read path + /// of every scalar column, so it is worth spelling out. + /// + /// + /// A few entries are unreachable today — , and the + /// native Int128/UInt128 are alternative read representations offered alongside a + /// type's FrameworkType, never as it. They are listed anyway so the table is exactly "every + /// ITypedReader<T> target", which + /// ColumnSlotTests.Binders_CoverEveryTypedReadTarget can then check mechanically. Over-inclusion + /// is inert; a missing entry would silently demote a column to the boxed path. + /// + private static readonly Dictionary> Binders = new() + { + [typeof(sbyte)] = Bind, + [typeof(short)] = Bind, + [typeof(int)] = Bind, + [typeof(long)] = Bind, + [typeof(byte)] = Bind, + [typeof(ushort)] = Bind, + [typeof(uint)] = Bind, + [typeof(ulong)] = Bind, + [typeof(BigInteger)] = Bind, + [typeof(float)] = Bind, + [typeof(double)] = Bind, + [typeof(decimal)] = Bind, + [typeof(ClickHouseDecimal)] = Bind, + [typeof(bool)] = Bind, + [typeof(string)] = Bind, + [typeof(byte[])] = Bind, + [typeof(Guid)] = Bind, + [typeof(IPAddress)] = Bind, + [typeof(DateTime)] = Bind, + [typeof(DateTimeOffset)] = Bind, + [typeof(DateOnly)] = Bind, + [typeof(TimeSpan)] = Bind, +#if NET8_0_OR_GREATER + [typeof(Int128)] = Bind, + [typeof(UInt128)] = Bind, +#endif + }; /// /// Returns the slot for . Never null: falls back to a @@ -62,46 +91,19 @@ private static ColumnSlot TryCreateTyped(ClickHouseType type, bool nullable) // AggregateFunctionType throws AggregateFunctionException from it (deliberately — you are meant to // learn you need xMerge() when you read the value, not when you open the reader), and the composite // types build a fresh Type object on each call. Slots are created for every column in the ctor, so - // hoisting this read above the no-typed-reader bail-out would turn merely *selecting* an - // AggregateFunction column into a failure to construct the reader at all. - var targets = TypedReadTargets.GetOrAdd(type.GetType(), FindTypedReadTargets); - if (targets.Length == 0) - return null; - - var clrType = type.FrameworkType; - if (Array.IndexOf(targets, clrType) < 0) + // reading FrameworkType before this bail-out would turn merely *selecting* an AggregateFunction + // column into a failure to construct the reader at all. + if (type is not ITypedReader) return null; - var constructors = ConstructorCache.GetOrAdd(clrType, BuildSlotConstructors); - return nullable ? constructors.Nullable(type) : constructors.Value(type); + return Binders.TryGetValue(type.FrameworkType, out var bind) ? bind(type, nullable) : null; } - private static Type[] FindTypedReadTargets(Type clickHouseTypeClass) => clickHouseTypeClass - .GetInterfaces() - .Where(i => i.IsGenericType && i.GetGenericTypeDefinition() == typeof(ITypedReader<>)) - .Select(i => i.GetGenericArguments()[0]) - .ToArray(); - - private static SlotConstructors BuildSlotConstructors(Type clrType) => new( - (Func)CreateValueSlotMethod - .MakeGenericMethod(clrType) - .CreateDelegate(typeof(Func)), - (Func)CreateNullableSlotMethod - .MakeGenericMethod(clrType) - .CreateDelegate(typeof(Func))); - - // The `is ITypedReader` here is the real check; TypedReadTargets is only a cheap pre-filter that keeps - // the constructor cache bounded, so these still return null rather than assuming the cast succeeds. - private static ColumnSlot CreateValueSlot(ClickHouseType type) - => type is ITypedReader typedReader ? new ValueSlot(typedReader) : null; - - private static ColumnSlot CreateNullableSlot(ClickHouseType type) - => type is ITypedReader typedReader ? new NullableSlot(typedReader) : null; - - private readonly struct SlotConstructors(Func value, Func nullable) - { - public Func Value { get; } = value; - - public Func Nullable { get; } = nullable; - } + // Binds only when the type's typed reader is for its *own* FrameworkType — the CLR type its boxed Read + // returns. A type offering extra representations (a DateTime column also readable as DateTimeOffset, a + // Decimal column as either decimal or ClickHouseDecimal) must not have a slot bound to one the boxed path + // would not have produced, or GetValue would start handing back a different CLR type. + private static ColumnSlot Bind(ClickHouseType type, bool nullable) + => type is not ITypedReader typedReader ? null + : nullable ? new NullableSlot(typedReader) : new ValueSlot(typedReader); } diff --git a/ClickHouse.Driver/Types/ITypedReader.cs b/ClickHouse.Driver/Types/ITypedReader.cs index 424dfb357..32ddfca2c 100644 --- a/ClickHouse.Driver/Types/ITypedReader.cs +++ b/ClickHouse.Driver/Types/ITypedReader.cs @@ -14,7 +14,17 @@ namespace ClickHouse.Driver.Types; /// representation, so every is byte-identical to it by construction. /// /// The exact CLR type this type can read without boxing (e.g. ). -internal interface ITypedReader +internal interface ITypedReader : ITypedReader { T ReadValue(ExtendedBinaryReader reader); } + +/// +/// Non-generic base of , so "can this type read anything box-free?" is a plain +/// type test rather than an interface-list walk. +/// asks that question for every column of every reader, and has to ask it before touching +/// . +/// +internal interface ITypedReader +{ +} From ed246f583ff06dee3d47670778126ceb3f6b1610 Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Mon, 3 Aug 2026 11:24:25 +0200 Subject: [PATCH 09/16] perf(ado): stop boxing populated nullable Bool and Decimal cells MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GetBoolean and GetDecimal coerce rather than cast, so neither can use the shared GetSlotValue body and both match their slot kinds by hand. Both recognised only the non-nullable ValueSlot, so a populated Nullable(Bool) or Nullable(Decimal) cell fell through to GetValue and boxed — on column types the feature already claims to cover. Add the populated-NullableSlot branches for bool, decimal and ClickHouseDecimal. A NULL cell still falls through to the boxed path, so Convert.ToBoolean(DBNull.Value) and the (decimal)DBNull cast keep throwing exactly as before, and a configured IReadValueConverter still bypasses the whole block. The new allocation test measures differentially against the identical non-nullable shape rather than against zero: under useBigDecimal the ClickHouseDecimal-to-decimal conversion allocates two byte arrays per call (~72 B/row) whatever the reader does, and that cost is common to both sides. Without the fix the nullable column measures identical to the boxed control (136 B/row for ClickHouseDecimal, 56 for decimal); with it, identical to the non-nullable twin. Also adds the value coverage the branches needed — GetDecimal had no nullable assertion anywhere, and Nullable(Bool) was only ever asserted at true. Co-Authored-By: Claude --- .../ADO/BoxFreeReaderAccessorTests.cs | 21 ++- .../ADO/BoxFreeReaderAllocationTests.cs | 122 ++++++++++++++++-- .../ADO/Readers/ClickHouseDataReader.cs | 32 +++-- 3 files changed, 149 insertions(+), 26 deletions(-) diff --git a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs index 7f0cb80dc..b6554cb51 100644 --- a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs +++ b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs @@ -343,14 +343,18 @@ public async Task TypedAccessors_NullableColumnsWithValues_ReturnUnderlyingValue { using var reader = await ReadOneAsync( "toInt64OrNull('7') AS a, toFloat64OrNull('1.5') AS b, " + - "CAST('s' AS Nullable(String)) AS c, CAST(true AS Nullable(Bool)) AS d"); + "CAST('s' AS Nullable(String)) AS c, CAST(true AS Nullable(Bool)) AS d, " + + "CAST(false AS Nullable(Bool)) AS e"); Assert.Multiple(() => { Assert.That(reader.GetInt64(0), Is.EqualTo(7L)); Assert.That(reader.GetDouble(1), Is.EqualTo(1.5d)); Assert.That(reader.GetString(2), Is.EqualTo("s")); + + // Both polarities, so the assertion cannot be satisfied by a branch that hard-codes one. Assert.That(reader.GetBoolean(3), Is.True); + Assert.That(reader.GetBoolean(4), Is.False); }); } @@ -401,7 +405,8 @@ public async Task GetString_NonStringColumn_StillCoerces() } // A Decimal column resolves to a decimal or a ClickHouseDecimal slot depending on UseCustomDecimals; - // GetDecimal has to reach the same decimal either way. + // GetDecimal has to reach the same decimal either way. Nullable() doubles that again — it is a different + // slot kind, so a different branch — and a NULL cell has to keep throwing off the boxed fallback. [TestCase(false)] [TestCase(true)] public async Task GetDecimal_UnderEitherDecimalRepresentation_ReturnsTheSameValue(bool useCustomDecimals) @@ -410,9 +415,17 @@ public async Task GetDecimal_UnderEitherDecimalRepresentation_ReturnsTheSameValu settings = new ClickHouseClientSettings(settings) { UseCustomDecimals = useCustomDecimals }; using var client = new ClickHouseClient(settings); - using var reader = await client.ExecuteReaderAsync("SELECT toDecimal64(12.34, 2) AS c"); + using var reader = await client.ExecuteReaderAsync( + "SELECT toDecimal64(12.34, 2) AS c, toDecimal64OrNull('56.78', 2) AS n, " + + "CAST(NULL AS Nullable(Decimal64(2))) AS z"); Assert.That(reader.Read(), Is.True); - Assert.That(reader.GetDecimal(0), Is.EqualTo(12.34m)); + + Assert.Multiple(() => + { + Assert.That(reader.GetDecimal(0), Is.EqualTo(12.34m)); + Assert.That(reader.GetDecimal(1), Is.EqualTo(56.78m)); + Assert.Throws(() => reader.GetDecimal(2)); + }); } // ---- Converter routing ---- diff --git a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAllocationTests.cs b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAllocationTests.cs index 3d29e3cbd..03e052a17 100644 --- a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAllocationTests.cs +++ b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAllocationTests.cs @@ -5,6 +5,7 @@ using System.Threading.Tasks; using ClickHouse.Driver.ADO.Readers; using ClickHouse.Driver.Formats; +using ClickHouse.Driver.Numerics; using ClickHouse.Driver.Types; namespace ClickHouse.Driver.Tests.ADO; @@ -29,35 +30,42 @@ public class BoxFreeReaderAllocationTests // measurement to boxing: anything left is the thing under test. private static readonly string[] ColumnTypes = ["Int64", "Float64", "Int32", "UInt64"]; - private static byte[] BuildPayload() + private static byte[] BuildPayload() => BuildPayload(ColumnTypes, TypeSettings.Default, WriteNumericRow); + + private static void WriteNumericRow(ExtendedBinaryWriter writer, ClickHouseType[] types, int row) + { + types[0].Write(writer, (long)row); + types[1].Write(writer, (double)row); + types[2].Write(writer, row); + types[3].Write(writer, (ulong)row); + } + + private static byte[] BuildPayload(string[] columnTypes, TypeSettings settings, Action writeRow, int rows = Rows) { - var types = Array.ConvertAll(ColumnTypes, t => TypeConverter.ParseClickHouseType(t, TypeSettings.Default)); + var types = Array.ConvertAll(columnTypes, t => TypeConverter.ParseClickHouseType(t, settings)); using var stream = new MemoryStream(); using var writer = new ExtendedBinaryWriter(stream); - writer.Write7BitEncodedInt(ColumnTypes.Length); - for (var i = 0; i < ColumnTypes.Length; i++) + writer.Write7BitEncodedInt(columnTypes.Length); + for (var i = 0; i < columnTypes.Length; i++) writer.Write($"c{i}"); - foreach (var name in ColumnTypes) + foreach (var name in columnTypes) writer.Write(name); - for (var row = 0; row < Rows; row++) - { - types[0].Write(writer, (long)row); - types[1].Write(writer, (double)row); - types[2].Write(writer, row); - types[3].Write(writer, (ulong)row); - } + for (var row = 0; row < rows; row++) + writeRow(writer, types, row); writer.Flush(); return stream.ToArray(); } - private static Task CreateReaderAsync(byte[] payload) + private static Task CreateReaderAsync(byte[] payload) => CreateReaderAsync(payload, TypeSettings.Default); + + private static Task CreateReaderAsync(byte[] payload, TypeSettings settings) => ClickHouseDataReader.FromHttpResponseAsync( new HttpResponseMessage(HttpStatusCode.OK) { Content = new ByteArrayContent(payload) }, - TypeSettings.Default); + settings); // Returns bytes allocated while draining the reader. Sums the values into `checksum` so nothing the // accessors produce can be optimized away as dead. @@ -135,5 +143,91 @@ public async Task Read_ValueTypeColumns_AllocatesNothingPerRow() }); } + // Bool and Decimal(10,2), read through the two accessors that coerce rather than cast. + private static readonly string[] CoercedColumnTypes = ["Bool", "Decimal(10,2)"]; + private static readonly string[] NullableCoercedColumnTypes = ["Nullable(Bool)", "Nullable(Decimal(10,2))"]; + + // Every cell is populated: a NULL costs nothing on either path (DBNull.Value is a singleton), so it is + // the present value that can be left boxing. + private static void WriteCoercedRow(ExtendedBinaryWriter writer, ClickHouseType[] types, int row) + { + types[0].Write(writer, row % 2 == 0); + types[1].Write(writer, row / 100m); + } + + private static double ReadCoerced(ClickHouseDataReader reader) + => (reader.GetBoolean(0) ? 1d : 0d) + (double)reader.GetDecimal(1); + + // The control has to do the same *work* as ReadCoerced, not merely touch the same cells, or it measures + // the decimal conversion rather than the box. This is the pre-slot body of both accessors verbatim, so + // the difference between the two is exactly the two boxes. + private static double ReadCoercedBoxed(ClickHouseDataReader reader) + { + var flag = Convert.ToBoolean(reader.GetValue(0), System.Globalization.CultureInfo.InvariantCulture); + var raw = reader.GetValue(1); + var value = raw is ClickHouseDecimal chd + ? chd.ToDecimal(System.Globalization.CultureInfo.InvariantCulture) + : (decimal)raw; + return (flag ? 1d : 0d) + (double)value; + } + + /// + /// and coerce + /// rather than cast, so neither can use the shared GetSlotValue<T> body and both match their + /// slot kinds by hand — which is how they came to recognise only the non-nullable ValueSlot<T> + /// and box every populated Nullable(Bool)/Nullable(Decimal) cell, on column types the + /// feature claims to cover. + /// + /// + /// Measured against the identical non-nullable shape rather than against zero, because zero is not the + /// right answer here: under useBigDecimal the ClickHouseDecimal-to-decimal conversion + /// allocates two byte arrays per call (~72 B/row) whatever the reader does, and that cost is common to + /// both sides. The difference isolates exactly the property under test — a Nullable column must + /// cost no more than its non-nullable twin. Both decimal representations are covered because + /// useBigDecimal picks between two different slots reached by two different branches. + /// + [TestCase(true)] + [TestCase(false)] + public async Task GetBooleanAndGetDecimal_PopulatedNullableCells_AllocateNoMoreThanNonNullable(bool useBigDecimal) + { + var settings = TypeSettings.Default with { useBigDecimal = useBigDecimal }; + var plain = BuildPayload(CoercedColumnTypes, settings, WriteCoercedRow); + var nullable = BuildPayload(NullableCoercedColumnTypes, settings, WriteCoercedRow); + + foreach (var payload in new[] { plain, nullable, nullable }) + { + using var warmup = await CreateReaderAsync(payload, settings); + Measure(warmup, ReadCoerced, out _); + using var warmupBoxed = await CreateReaderAsync(payload, settings); + Measure(warmupBoxed, ReadCoercedBoxed, out _); + } + + long plainAllocated, nullableAllocated, nullableBoxed; + using (var reader = await CreateReaderAsync(plain, settings)) + plainAllocated = Measure(reader, ReadCoerced, out _); + using (var reader = await CreateReaderAsync(nullable, settings)) + nullableAllocated = Measure(reader, ReadCoerced, out _); + using (var reader = await CreateReaderAsync(nullable, settings)) + nullableBoxed = Measure(reader, ReadCoercedBoxed, out _); + + TestContext.Out.WriteLine( + $"useBigDecimal={useBigDecimal} plain={PerRow(plainAllocated)} nullable={PerRow(nullableAllocated)} " + + $"nullableBoxed={PerRow(nullableBoxed)} (bytes/row)"); + + Assert.Multiple(() => + { + // One byte per row of slack, which is far below the 24-byte box either accessor would take. + Assert.That(nullableAllocated, Is.LessThanOrEqualTo(plainAllocated + Rows), + $"a populated Nullable cell must cost no more than a non-nullable one; saw " + + $"{PerRow(nullableAllocated)} B/row against {PerRow(plainAllocated)} B/row"); + + // The control: the same two cells through GetValue do box, so the comparison above is measuring + // something rather than two equally-zero numbers. + Assert.That(nullableBoxed, Is.GreaterThan(nullableAllocated + (Rows * 12)), + $"GetValue is expected to box both cells; saw only {PerRow(nullableBoxed)} B/row, so this " + + $"test is not measuring boxing"); + }); + } + private static string PerRow(long allocated) => (allocated / (double)Rows).ToString("F1", System.Globalization.CultureInfo.InvariantCulture); } diff --git a/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs b/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs index 6b6ad4908..b2cdd19b3 100644 --- a/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs +++ b/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs @@ -179,11 +179,21 @@ private T GetTypedValue(int ordinal) => readValueConverter == null ? GetSlotValue(ordinal) : (T)GetValue(ordinal); // Unlike its neighbours this one coerces rather than casts, so only an exact Bool column can take the - // fast path; anything else keeps Convert.ToBoolean's widening (and its exception messages). + // fast path; anything else keeps Convert.ToBoolean's widening (and its exception messages). A NULL cell + // falls through as well, so Convert.ToBoolean(DBNull.Value) still throws exactly as it did. public override bool GetBoolean(int ordinal) - => readValueConverter == null && Slot(ordinal) is ValueSlot boolSlot - ? boolSlot.Value - : Convert.ToBoolean(GetValue(ordinal), CultureInfo.InvariantCulture); + { + if (readValueConverter == null) + { + var slot = Slot(ordinal); + if (slot is ValueSlot boolSlot) + return boolSlot.Value; + if (slot is NullableSlot nullableSlot && nullableSlot.HasValue) + return nullableSlot.Value; + } + + return Convert.ToBoolean(GetValue(ordinal), CultureInfo.InvariantCulture); + } public override byte GetByte(int ordinal) => GetTypedValue(ordinal); @@ -206,12 +216,18 @@ public override decimal GetDecimal(int ordinal) { if (readValueConverter == null) { - // Which of these two a Decimal column resolves to is the UseBigDecimal setting's doing; both - // reach the same decimal without a box. - if (Slot(ordinal) is ValueSlot decimalSlot) + // Which of these two representations a Decimal column resolves to is the UseBigDecimal setting's + // doing; both reach the same decimal without a box, nullable or not. A NULL cell falls through to + // the boxed path below, where casting DBNull.Value throws exactly as it did. + var slot = Slot(ordinal); + if (slot is ValueSlot decimalSlot) return decimalSlot.Value; - if (slots[ordinal] is ValueSlot bigDecimalSlot) + if (slot is NullableSlot nullableDecimalSlot && nullableDecimalSlot.HasValue) + return nullableDecimalSlot.Value; + if (slot is ValueSlot bigDecimalSlot) return bigDecimalSlot.Value.ToDecimal(CultureInfo.InvariantCulture); + if (slot is NullableSlot nullableBigDecimalSlot && nullableBigDecimalSlot.HasValue) + return nullableBigDecimalSlot.Value.ToDecimal(CultureInfo.InvariantCulture); } var value = GetValue(ordinal); From f6c38487f02ad8ce818c8bb389d7b7be8cecb65c Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Mon, 3 Aug 2026 11:25:48 +0200 Subject: [PATCH 10/16] perf(ado): build column slots on the first Read() rather than in the ctor The reader constructed one ColumnSlot per selected column eagerly, but QueryAsync's box-free POCO path materializes straight from the stream through TryMaterializeNextRow and never reads a slot. Every one of them was dead allocation on the driver's primary read API, and an empty result set or a reader opened only for its column metadata paid the same. Move construction into the first Read(). The safety argument is unchanged in substance: slots are built once and never nulled, so hasCurrentRow implies non-null, and that is the condition every value accessor already establishes through Slot(). GetValues is the one accessor that indexes the array directly, and it carries its own copy of the guard. Also corrects the comments and changelog wording the slot work left stale: several still described the removed object[] row buffer, one credited the slot factory with runtime reflection that a later commit deleted, and the changelog understated the untyped path by omitting the per-column slot and the repeated- GetValue box. One consequence worth recording: ColumnSlotFactory's bail-out ordering can no longer be guarded end to end. With slot construction inside Read(), an ordering regression raises the same AggregateFunctionException, with the same message, from the same call. The unit test in ColumnSlotTests remains a real guard; Read_AggregateFunctionColumn_OpensTheReaderAndFailsOnlyOnTheValue no longer is, and its comment now says so rather than implying otherwise. Co-Authored-By: Claude --- .../ADO/BoxFreeReaderAccessorTests.cs | 11 +++- .../ADO/BoxFreeReaderAllocationTests.cs | 55 ++++++++++++++++++- .../ADO/ColumnSlotTests.cs | 11 ++-- .../ADO/Readers/ClickHouseDataReader.cs | 44 +++++++++++---- .../ADO/Readers/ColumnSlotFactory.cs | 12 ++-- ClickHouse.Driver/ClickHouseClient.cs | 2 +- .../Poco/PocoReadExpressionFactory.cs | 9 ++- 7 files changed, 114 insertions(+), 30 deletions(-) diff --git a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs index b6554cb51..daa595251 100644 --- a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs +++ b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs @@ -118,9 +118,14 @@ public async Task GetFieldValue_CompositeColumn_ReadsThroughBoxedFallback() Assert.That(reader.GetFieldValue(0), Is.EqualTo(new[] { 1, 2, 3 })); } - // End-to-end counterpart to ColumnSlotTests' factory guard: an AggregateFunction column must still open a - // reader and fail only where it always did — on the value, with the message that tells you to use - // xMerge(). Building a slot per column at construction time is what puts that at risk. + // Pins the user-visible contract for an AggregateFunction column: the reader opens, FieldCount answers, + // and the failure arrives on the row with the message that tells you to use xMerge(). + // + // Deliberately not the guard on ColumnSlotFactory's bail-out ordering, though it reads like one. Slots are + // built on the first Read(), so an ordering regression would throw the same exception with the same + // message from this same call; only the unit test + // (ColumnSlotTests.Create_AggregateFunctionColumn_FallsBackToBoxedWithoutEvaluatingFrameworkType) can + // distinguish them. [Test] public async Task Read_AggregateFunctionColumn_OpensTheReaderAndFailsOnlyOnTheValue() { diff --git a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAllocationTests.cs b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAllocationTests.cs index 03e052a17..1dca8db16 100644 --- a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAllocationTests.cs +++ b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAllocationTests.cs @@ -2,6 +2,7 @@ using System.IO; using System.Net; using System.Net.Http; +using System.Reflection; using System.Threading.Tasks; using ClickHouse.Driver.ADO.Readers; using ClickHouse.Driver.Formats; @@ -104,8 +105,8 @@ public async Task Read_ValueTypeColumns_AllocatesNothingPerRow() { var payload = BuildPayload(); - // First pass per shape pays for JIT, generic instantiation and the slot factory's one-off reflection. - // None of that is per-row, and none of it should be attributed to the measurement. + // First pass per shape pays for JIT and generic instantiation. Neither is per-row, and neither should + // be attributed to the measurement. foreach (var warmup in new[] { Scan, ReadTyped, ReadFieldValue, ReadBoxed }) { using var reader = await CreateReaderAsync(payload); @@ -229,5 +230,55 @@ public async Task GetBooleanAndGetDecimal_PopulatedNullableCells_AllocateNoMoreT }); } + /// + /// The column slots are the reader's only per-column allocation, and QueryAsync<T>'s box-free + /// POCO path never reads one — it materializes straight from the stream. Building them in the constructor + /// would put one permanently dead object per column on the driver's primary read API, and on every empty + /// or metadata-only reader besides. + /// + /// + /// Reached by reflection because the effect under test is an absence. No public surface reports whether + /// the storage exists, and measuring it in bytes would mean resolving a few hundred bytes against the row + /// materialization it is supposed to be dwarfed by. + /// + [Test] + public async Task Read_BeforeFirstRow_HasNotBuiltColumnSlots() + { + var slotsField = SlotsField(); + + using var reader = await CreateReaderAsync(BuildPayload()); + Assert.That(slotsField.GetValue(reader), Is.Null, "column slots must not be built before the first Read()"); + + Assert.That(reader.Read(), Is.True); + Assert.That(slotsField.GetValue(reader), Is.Not.Null, "the first Read() must build the column slots"); + } + + [Test] + public async Task Read_EmptyResult_BuildsNoColumnSlotsAndKeepsMetadata() + { + var slotsField = SlotsField(); + var payload = BuildPayload(ColumnTypes, TypeSettings.Default, WriteNumericRow, rows: 0); + + using var reader = await CreateReaderAsync(payload); + Assert.That(reader.Read(), Is.False); + + Assert.Multiple(() => + { + Assert.That(slotsField.GetValue(reader), Is.Null, "an empty result must not build column slots"); + + // Metadata stays available with no row, which is what makes skipping the storage safe. + Assert.That(reader.FieldCount, Is.EqualTo(Columns)); + Assert.That(reader.GetName(0), Is.EqualTo("c0")); + Assert.That(reader.GetFieldType(0), Is.EqualTo(typeof(long))); + }); + } + + private static FieldInfo SlotsField() + { + var field = typeof(ClickHouseDataReader).GetField("slots", BindingFlags.Instance | BindingFlags.NonPublic); + Assert.That(field, Is.Not.Null, "the reader's column-slot storage was renamed; update these tests"); + return field; + } + private static string PerRow(long allocated) => (allocated / (double)Rows).ToString("F1", System.Globalization.CultureInfo.InvariantCulture); } diff --git a/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs b/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs index 1baf00324..a96dc259b 100644 --- a/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs +++ b/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs @@ -271,10 +271,13 @@ public void Binders_CoverEveryTypedReadTarget() } // AggregateFunctionType throws from FrameworkType (and from Read and ToString) so that you learn you need - // xMerge() when you read the value. Slots are built for every column when the reader is constructed, so - // the factory must reach its no-typed-reader bail-out without ever evaluating FrameworkType — otherwise - // merely selecting such a column would fail to open the reader at all. Guards the ordering in - // TryCreateTyped, which is otherwise easy to "tidy up" into a regression. + // xMerge() when you read the value. Slots are built for every column of the row on the reader's first + // Read(), so the factory must reach its no-typed-reader bail-out without ever evaluating FrameworkType — + // otherwise building the slots would throw before a single column had been decoded. Guards the ordering + // in TryCreateTyped, which is otherwise easy to "tidy up" into a regression. + // + // This is the only test that can guard it. End to end the two orderings are indistinguishable: both raise + // AggregateFunctionException, with the same message, from the same Read() call. [Test] public void Create_AggregateFunctionColumn_FallsBackToBoxedWithoutEvaluatingFrameworkType() { diff --git a/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs b/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs index b2cdd19b3..4487dbb07 100644 --- a/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs +++ b/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs @@ -37,9 +37,20 @@ public class ClickHouseDataReader : DbDataReader, IEnumerator, IEnu private readonly PocoTypeRegistry pocoRegistry; private readonly Dictionary bindingPlanCache = new(); - // Per-column typed storage for the current row; replaces the old shared object[] buffer. Built once per - // reader, mutated in place by every Read(). Always non-null and always FieldNames.Length long. - private readonly ColumnSlot[] slots; + // Per-column typed storage for the current row; replaces the old shared object[] buffer. Built on the + // first Read() and mutated in place by every one after it. + // + // Deliberately not built in the constructor. QueryAsync's box-free POCO path materializes straight + // from the stream through TryMaterializeNextRow and never touches a slot, so constructing them eagerly + // would allocate one permanently dead object per column on the driver's primary read API. Empty result + // sets and readers opened only for their metadata likewise never pay for storage they do not use. + // + // Built once and never nulled again, so `hasCurrentRow` implies non-null. That is the whole safety + // argument, and every value accessor establishes it by going through Slot(). GetValues is the one + // exception — it indexes this array directly and carries its own copy of the guard, so if that guard is + // ever "simplified" away it produces a NullReferenceException rather than the intended + // InvalidOperationException. + private ColumnSlot[] slots; private bool hasCurrentRow; private ClickHouseDataReader(HttpResponseMessage httpResponse, ExtendedBinaryReader reader, PooledReadBufferStream pooledReadBuffer, string[] names, ClickHouseType[] types, string[] rawTypeNames, PocoTypeRegistry pocoRegistry, ExceptionTagAwareStream exceptionTagStream = null, IReadValueConverter readValueConverter = null, Stream decompressor = null) @@ -57,10 +68,6 @@ private ClickHouseDataReader(HttpResponseMessage httpResponse, ExtendedBinaryRea RawTypes = types; FieldNames = names; columnTypeNames = rawTypeNames; - - slots = new ColumnSlot[types.Length]; - for (var i = 0; i < types.Length; i++) - slots[i] = ColumnSlotFactory.Create(types[i]); } internal static Task FromHttpResponseAsync(HttpResponseMessage httpResponse, TypeSettings settings) @@ -568,8 +575,8 @@ internal bool TryGetRowMaterializer(out RowColumnReader[] materializers, o } /// - /// Reads the next row straight from the stream via the fast-path delegates, bypassing the shared - /// object[] row buffer and its per-value boxing. Returns false at end of stream, and mirrors + /// Reads the next row straight from the stream via the fast-path delegates, bypassing the reader's column + /// slots, which this path never allocates. Returns false at end of stream, and mirrors /// 's mid-stream server-exception handling. The delegates consume every wire column in /// order, so the stream stays aligned even for columns the POCO does not map. /// @@ -657,9 +664,13 @@ public override bool Read() if (reader.PeekChar() == -1) return false; // End of stream reached - for (var i = 0; i < slots.Length; i++) + // Built on the first row rather than in the ctor: the POCO fast path materializes straight + // from the stream and never touches a slot, so eager construction would allocate a + // permanently dead object per column on the primary read API. An empty result never gets here. + var columns = slots ??= CreateSlots(); + for (var i = 0; i < columns.Length; i++) { - slots[i].Read(reader); + columns[i].Read(reader); } hasCurrentRow = true; return true; @@ -678,6 +689,17 @@ public override bool Read() } } + // Runs at most once per reader, so it is kept out of Read() to leave that method small enough for the + // JIT to treat the slot loop as the hot path it is. + [MethodImpl(MethodImplOptions.NoInlining)] + private ColumnSlot[] CreateSlots() + { + var created = new ColumnSlot[RawTypes.Length]; + for (var i = 0; i < created.Length; i++) + created[i] = ColumnSlotFactory.Create(RawTypes[i]); + return created; + } + #pragma warning disable CA2215 // Dispose methods should call base class dispose protected override void Dispose(bool disposing) { diff --git a/ClickHouse.Driver/ADO/Readers/ColumnSlotFactory.cs b/ClickHouse.Driver/ADO/Readers/ColumnSlotFactory.cs index 3afa59149..8c9d8e922 100644 --- a/ClickHouse.Driver/ADO/Readers/ColumnSlotFactory.cs +++ b/ClickHouse.Driver/ADO/Readers/ColumnSlotFactory.cs @@ -89,10 +89,14 @@ private static ColumnSlot TryCreateTyped(ClickHouseType type, bool nullable) { // Order matters, and not only for speed: FrameworkType is not safe to evaluate on every column type. // AggregateFunctionType throws AggregateFunctionException from it (deliberately — you are meant to - // learn you need xMerge() when you read the value, not when you open the reader), and the composite - // types build a fresh Type object on each call. Slots are created for every column in the ctor, so - // reading FrameworkType before this bail-out would turn merely *selecting* an AggregateFunction - // column into a failure to construct the reader at all. + // learn you need xMerge() when you read the value), and the composite types build a fresh Type object + // on each call. Testing the marker first is what keeps slot construction total: it has to work for + // every column of every row the reader is handed, and so may depend only on the FrameworkType of + // types that advertise a typed reader in the first place. + // + // ColumnSlotTests.Create_AggregateFunctionColumn_FallsBackToBoxedWithoutEvaluatingFrameworkType is + // the guard on this ordering. It has to be a unit test: end to end both orderings raise the same + // exception from the same Read() call, so no reader-level test can tell them apart. if (type is not ITypedReader) return null; diff --git a/ClickHouse.Driver/ClickHouseClient.cs b/ClickHouse.Driver/ClickHouseClient.cs index 72e807a34..3ada0106a 100644 --- a/ClickHouse.Driver/ClickHouseClient.cs +++ b/ClickHouse.Driver/ClickHouseClient.cs @@ -445,7 +445,7 @@ public async IAsyncEnumerable QueryAsync( // the underlying HTTP stream is buffered, so per-row reads do not perform real I/O. if (reader.TryGetRowMaterializer(out var materializers, out var constructor)) { - // Bypasses the shared object[] row buffer and the boxing/unboxing MapTo setter. + // Bypasses the reader's column slots and the boxing/unboxing MapTo setter. while (reader.TryMaterializeNextRow(materializers, constructor, out var row)) { cancellationToken.ThrowIfCancellationRequested(); diff --git a/ClickHouse.Driver/Poco/PocoReadExpressionFactory.cs b/ClickHouse.Driver/Poco/PocoReadExpressionFactory.cs index 688dcfc1b..e4e2f5afa 100644 --- a/ClickHouse.Driver/Poco/PocoReadExpressionFactory.cs +++ b/ClickHouse.Driver/Poco/PocoReadExpressionFactory.cs @@ -10,11 +10,10 @@ namespace ClickHouse.Driver.Poco; /// /// Builds box-free read expressions for the POCO read (materialization) fast path. /// -/// The default path boxes every value-type column through -/// into the reader's shared object[] row -/// buffer, then unboxes it in a compiled Action<T,object> setter (MapTo<T>). Where a -/// type can decode straight into the target CLR type, this compiles a fused read-and-assign delegate -/// instead, dropping both the box and the unbox. +/// The fallback path decodes each column into the reader's per-column slot, boxes it on the way out through +/// GetValue, then unboxes it in a compiled Action<T,object> setter (MapTo<T>). +/// Where a type can decode straight into the target CLR type, this compiles a fused read-and-assign delegate +/// instead, dropping both the box and the unbox and bypassing the slots. /// /// Dispatch is driven by : a column type takes the fast path for a property CLR /// type iff it implements ITypedReader<thatType>, which also lets one column offer several From 23c22428fd685525f349659e4d9b8fa068c73edd Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Mon, 3 Aug 2026 11:26:09 +0200 Subject: [PATCH 11/16] test(poco): materialize the SimpleAggregateFunction column, don't just plan it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QueryAsync_SimpleAggregateFunctionColumn_ReadsUnderlyingBoxFree opened a reader over the raw wrapped column only to assert a materializer plan could be built, then ran QueryAsync against SELECT Name, sum(Total) — whose Total projects to an ordinary UInt64. The wrapped column's bytes never reached the fast path, so the test would have passed with no wrapper read support at all. Read the raw column instead, through FINAL so the two 'a' parts are merged whether or not the background merge has run, while the column stays declared — and so encoded on the wire — as SimpleAggregateFunction(sum, UInt64). Assert the wire type as well, so a future rewrite cannot quietly stop covering the wrapper. The aggregate-result case is kept alongside it. Co-Authored-By: Claude --- .../Copy/PocoReadFastPathTests.cs | 29 +++++++++++++++---- 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/ClickHouse.Driver.Tests/Copy/PocoReadFastPathTests.cs b/ClickHouse.Driver.Tests/Copy/PocoReadFastPathTests.cs index 64e12e7e0..bacdafcc7 100644 --- a/ClickHouse.Driver.Tests/Copy/PocoReadFastPathTests.cs +++ b/ClickHouse.Driver.Tests/Copy/PocoReadFastPathTests.cs @@ -406,17 +406,34 @@ await client.ExecuteNonQueryAsync( "ENGINE AggregatingMergeTree ORDER BY Name"); await client.ExecuteNonQueryAsync($"INSERT INTO {table} VALUES ('a', 3), ('a', 4), ('b', 10)"); - var sql = $"SELECT Name, sum(Total) AS Total FROM {table} GROUP BY Name ORDER BY Name"; + // FINAL, so the two 'a' parts are merged whether or not the background merge has run yet, while the + // column stays declared — and therefore encoded on the wire — as SimpleAggregateFunction(sum, UInt64). + // This is the query that actually exercises the wrapper: sum(Total) below projects to a plain UInt64 + // and would pass with no wrapper support at all. + var rawSql = $"SELECT Name, Total FROM {table} FINAL ORDER BY Name"; - using (var reader = (ClickHouseDataReader)await client.ExecuteReaderAsync($"SELECT Name, Total FROM {table}")) + using (var reader = (ClickHouseDataReader)await client.ExecuteReaderAsync(rawSql)) + { + Assert.That(reader.GetDataTypeName(1), Does.StartWith("SimpleAggregateFunction("), + "the wrapper must survive to the wire, or this test is not covering it"); Assert.That(reader.TryGetRowMaterializer(out _, out _), Is.True, "SimpleAggregateFunction is wire-transparent and must reach the wrapped typed reader"); + } - var rows = new List(); - await foreach (var row in client.QueryAsync(sql)) - rows.Add(row); + var raw = new List(); + await foreach (var row in client.QueryAsync(rawSql)) + raw.Add(row); + + Assert.That(raw.Select(r => (r.Name, r.Total)), Is.EqualTo(new[] { ("a", 7ul), ("b", 10ul) }), + "the wrapped column must materialize through the fast path"); + + // And separately the aggregate result, whose Total is an ordinary UInt64. + var aggregated = new List(); + await foreach (var row in client.QueryAsync( + $"SELECT Name, sum(Total) AS Total FROM {table} GROUP BY Name ORDER BY Name")) + aggregated.Add(row); - Assert.That(rows.Select(r => (r.Name, r.Total)), Is.EqualTo(new[] { ("a", 7ul), ("b", 10ul) })); + Assert.That(aggregated.Select(r => (r.Name, r.Total)), Is.EqualTo(new[] { ("a", 7ul), ("b", 10ul) })); } public class NullablePropPoco From 9119b3902113c6ed0551646a79f67ef976573db1 Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Tue, 4 Aug 2026 13:34:12 +0200 Subject: [PATCH 12/16] docs: say which converter overload each read path uses The remark and test comment named both IReadValueConverter overloads without saying which path calls which, so "rather than switching to ConvertValue" read as if nothing calls the generic one. GetFieldValue does. Co-Authored-By: Claude --- .../ADO/BoxFreeReaderAccessorTests.cs | 9 +++++---- ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs | 5 +++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs index daa595251..98033dd86 100644 --- a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs +++ b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs @@ -435,10 +435,11 @@ public async Task GetDecimal_UnderEitherDecimalRepresentation_ReturnsTheSameValu // ---- Converter routing ---- - // The typed accessors were `(T)GetValue(ordinal)`, so they saw ConvertValue(object, ...). De-boxing them - // would have switched them to ConvertValue, which is observable to a converter whose two overloads - // disagree — so with a converter configured they stay on the boxed route. GetFieldValue is unaffected: - // it already called ConvertValue. This pins both halves of that decision. + // Two paths, two overloads, both unchanged by column slots: the typed accessors were + // `(T)GetValue(ordinal)` and so saw ConvertValue(object, ...), while GetFieldValue called + // ConvertValue. De-boxing the accessors would have switched them onto ConvertValue, which is + // observable to a converter whose two overloads disagree, so with a converter configured they stay on the + // boxed route. This pins both halves of that decision. [Test] public async Task WithConverter_TypedAccessorUsesObjectOverloadWhileGetFieldValueUsesGeneric() { diff --git a/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs b/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs index 4487dbb07..45c14bbb1 100644 --- a/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs +++ b/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs @@ -179,8 +179,9 @@ internal ClickHouseType GetEffectiveClickHouseType(int ordinal) /// De-boxing here would mean calling ConvertValue<T> instead, which is observable to a /// converter whose two overloads disagree — the generic one matching on typeof(T), the object one /// on the value's runtime type. Not worth a silent semantic change for the rare converter case; - /// everyone else gets the fast path. is unaffected either way, because it - /// already called ConvertValue<T>. + /// everyone else gets the fast path. keeps its own routing and stays on + /// ConvertValue<T>, which is the overload it already called — so between the two paths every + /// converter overload is still reached, exactly as before. /// private T GetTypedValue(int ordinal) => readValueConverter == null ? GetSlotValue(ordinal) : (T)GetValue(ordinal); From 33f9938f0c91aeee2482cf62f6b63344b0066e8c Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Tue, 4 Aug 2026 16:40:31 +0200 Subject: [PATCH 13/16] docs: trim the slot commentary to what the code does not already say The running commentary across ColumnSlot, ColumnSlotFactory, ITypedReader and the reader had drifted into restating the mechanism. Cut to the parts a reader cannot recover from the code: why a slot binds only to a type's own FrameworkType, why the generics are unconstrained, and why Nullable is excluded from TransparentWrapper. Co-Authored-By: Claude --- .../ADO/BoxFreeReaderAccessorTests.cs | 10 +- .../ADO/ColumnSlotTests.cs | 18 ++-- .../ADO/Readers/ClickHouseDataReader.cs | 101 ++++++++---------- ClickHouse.Driver/ADO/Readers/ColumnSlot.cs | 48 ++++----- .../ADO/Readers/ColumnSlotFactory.cs | 54 ++++------ ClickHouse.Driver/Types/ITypedReader.cs | 24 ++--- ClickHouse.Driver/Types/TransparentWrapper.cs | 11 +- 7 files changed, 117 insertions(+), 149 deletions(-) diff --git a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs index 98033dd86..db50dda65 100644 --- a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs +++ b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs @@ -11,11 +11,11 @@ namespace ClickHouse.Driver.Tests.ADO; /// /// Pins the accessor semantics of the typed-column-slot reader against a live server. /// -/// proves a slot decodes the same bytes to the same value as the boxed -/// reader. What it cannot see is the layer above: which slot each accessor reaches for, and — more -/// importantly — that the cases the fast path deliberately declines still fail in exactly the way they used -/// to. Widening, reading a NULL as a non-nullable target, and T = U? all fall through to the boxed -/// cast, and their s are part of the ADO.NET contract callers rely on. +/// proves a slot decodes the same bytes to the same value as the boxed reader. +/// What it cannot see is the layer above: which slot each accessor reaches for, and — more importantly — that the +/// cases the fast path deliberately declines still fail exactly as they used to. Widening, reading a NULL as a +/// non-nullable target, and T = U? all fall through to the boxed cast, and their +/// s are part of the ADO.NET contract callers rely on. /// [TestFixture] public class BoxFreeReaderAccessorTests : AbstractConnectionTestFixture diff --git a/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs b/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs index a96dc259b..8296075ef 100644 --- a/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs +++ b/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs @@ -17,16 +17,14 @@ namespace ClickHouse.Driver.Tests.ADO; /// Server-free tests for the typed column slots that replaced ClickHouseDataReader's shared /// object[] row buffer. /// -/// The whole design rests on one invariant: a slot must be observationally identical to the -/// boxed path it replaced. must consume exactly the bytes -/// would have, and -/// must return exactly the value it would have returned — same CLR type, and for a -/// NULL. asserts precisely that, differentially, -/// against the real boxed reader rather than against hand-written expectations. -/// -/// The rest pins the things parity alone cannot see: which slot kind a column resolves to (a -/// silent demotion to is invisible — values stay correct, only the allocation -/// disappears), and , which has no boxed counterpart to compare against. +/// The design rests on one invariant: a slot must be observationally identical to the boxed path it +/// replaced — consuming exactly the bytes +/// would, and returning +/// exactly the value it would (same CLR type, for a NULL). +/// asserts that differentially against the real +/// boxed reader. The rest pins what parity cannot see: which slot kind a column resolves to (a silent +/// demotion to keeps values correct and only loses the allocation win), and +/// , which has no boxed counterpart to compare against. /// [TestFixture] public class ColumnSlotTests diff --git a/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs b/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs index 45c14bbb1..9cd9bbdd1 100644 --- a/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs +++ b/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs @@ -37,19 +37,17 @@ public class ClickHouseDataReader : DbDataReader, IEnumerator, IEnu private readonly PocoTypeRegistry pocoRegistry; private readonly Dictionary bindingPlanCache = new(); - // Per-column typed storage for the current row; replaces the old shared object[] buffer. Built on the - // first Read() and mutated in place by every one after it. + // Per-column typed storage for the current row; replaces the old shared object[] buffer. Built on the first + // Read() and mutated in place by every one after it. // - // Deliberately not built in the constructor. QueryAsync's box-free POCO path materializes straight - // from the stream through TryMaterializeNextRow and never touches a slot, so constructing them eagerly - // would allocate one permanently dead object per column on the driver's primary read API. Empty result - // sets and readers opened only for their metadata likewise never pay for storage they do not use. + // Not built in the constructor: QueryAsync's box-free POCO path materializes straight from the stream and + // never touches a slot, so eager construction would allocate one permanently dead object per column on the + // primary read API. Empty result sets and metadata-only readers likewise pay nothing. // - // Built once and never nulled again, so `hasCurrentRow` implies non-null. That is the whole safety - // argument, and every value accessor establishes it by going through Slot(). GetValues is the one - // exception — it indexes this array directly and carries its own copy of the guard, so if that guard is - // ever "simplified" away it produces a NullReferenceException rather than the intended - // InvalidOperationException. + // Built once and never nulled again, so `hasCurrentRow` implies non-null — the whole safety argument, which + // every value accessor establishes by going through Slot(). GetValues is the one exception: it indexes this + // array directly and carries its own copy of the guard, so "simplifying" that guard away yields a + // NullReferenceException instead of the intended InvalidOperationException. private ColumnSlot[] slots; private bool hasCurrentRow; @@ -169,26 +167,24 @@ internal ClickHouseType GetEffectiveClickHouseType(int ordinal) /// /// Shared body for the strict typed accessors — every one of which was (T)GetValue(ordinal) before - /// column slots, and keeps exactly that meaning here. This is the path compiled ORM mappers drive - /// (linq2db registers GetInt64/GetDouble/GetDateTime/… and inlines them per column - /// per row), so it is where the boxing elimination is worth the most. + /// column slots and keeps exactly that meaning here. This is the path compiled ORM mappers drive (linq2db + /// inlines GetInt64/GetDouble/GetDateTime/… per column per row), so it is where the + /// boxing elimination is worth the most. /// /// /// With an configured the accessor keeps routing through - /// , so it still calls ConvertValue(object, …) exactly as it did before. - /// De-boxing here would mean calling ConvertValue<T> instead, which is observable to a - /// converter whose two overloads disagree — the generic one matching on typeof(T), the object one - /// on the value's runtime type. Not worth a silent semantic change for the rare converter case; - /// everyone else gets the fast path. keeps its own routing and stays on - /// ConvertValue<T>, which is the overload it already called — so between the two paths every - /// converter overload is still reached, exactly as before. + /// , so it still calls ConvertValue(object, …) as before. De-boxing here would + /// switch it to ConvertValue<T>, which is observable to a converter whose two overloads disagree + /// (the generic one matching on typeof(T), the object one on the value's runtime type) — not worth a + /// silent semantic change for the rare converter case. stays on + /// ConvertValue<T>, the overload it already called, so both overloads are still reached. /// private T GetTypedValue(int ordinal) => readValueConverter == null ? GetSlotValue(ordinal) : (T)GetValue(ordinal); - // Unlike its neighbours this one coerces rather than casts, so only an exact Bool column can take the - // fast path; anything else keeps Convert.ToBoolean's widening (and its exception messages). A NULL cell - // falls through as well, so Convert.ToBoolean(DBNull.Value) still throws exactly as it did. + // Unlike its neighbours this one coerces rather than casts, so only an exact Bool column takes the fast path; + // anything else keeps Convert.ToBoolean's widening and its exception messages. A NULL cell falls through too, + // so Convert.ToBoolean(DBNull.Value) still throws exactly as it did. public override bool GetBoolean(int ordinal) { if (readValueConverter == null) @@ -224,9 +220,9 @@ public override decimal GetDecimal(int ordinal) { if (readValueConverter == null) { - // Which of these two representations a Decimal column resolves to is the UseBigDecimal setting's - // doing; both reach the same decimal without a box, nullable or not. A NULL cell falls through to - // the boxed path below, where casting DBNull.Value throws exactly as it did. + // Which of the two representations a Decimal column resolves to is the UseBigDecimal setting's doing; + // both reach the same decimal without a box, nullable or not. A NULL cell falls through to the boxed + // path below, where casting DBNull.Value throws exactly as it did. var slot = Slot(ordinal); if (slot is ValueSlot decimalSlot) return decimalSlot.Value; @@ -273,23 +269,23 @@ public override int GetOrdinal(string name) return index; } - // Deliberately narrower than the other accessors: only a non-nullable String column short-circuits. Every - // other shape keeps ToString()'s coercion, including the quirk that a NULL cell yields "" rather than - // null, because DBNull.Value.ToString() is the empty string. + // Deliberately narrower than the other accessors: only a non-nullable String column short-circuits. Every other + // shape keeps ToString()'s coercion, including the quirk that a NULL cell yields "" rather than null, because + // DBNull.Value.ToString() is the empty string. public override string GetString(int ordinal) => readValueConverter == null && Slot(ordinal) is ValueSlot stringSlot ? stringSlot.Value : GetValue(ordinal)?.ToString(); /// - /// The one boxing entry point on the read path. Boxes lazily, per call, so a query that projects ten - /// columns and reads two pays for two — where the old object[] buffer boxed all ten during - /// regardless. + /// The one boxing entry point on the read path. Boxes lazily, per call, so a query that projects ten columns + /// and reads two pays for two — where the old object[] buffer boxed all ten during + /// regardless. /// /// - /// Consequence of boxing per call rather than once per row: two GetValue(i) calls on the same - /// value-type cell now return two distinct boxes. They compare equal by - /// (the ADO.NET-relevant comparison) but no longer by . + /// Consequence of boxing per call rather than once per row: two GetValue(i) calls on the same value-type + /// cell return two distinct boxes. They still compare equal by — the + /// ADO.NET-relevant comparison — but no longer by . /// public override object GetValue(int ordinal) { @@ -321,9 +317,9 @@ public override int GetValues(object[] values) } public override bool IsDBNull(int ordinal) - // Asks the slot directly rather than going through GetValue, for two reasons: a configured - // IReadValueConverter must not run during a null check (it could throw, do expensive work, or - // change the nullness of the result), and a null check has no business materializing a box. + // Asks the slot directly rather than going through GetValue: a configured IReadValueConverter must not run + // during a null check (it could throw, do expensive work, or change the result's nullness), and a null + // check has no business materializing a box. => Slot(ordinal).IsNull; /// @@ -332,11 +328,10 @@ public override bool IsDBNull(int ordinal) /// without a current row and is deliberately not gated. /// /// - /// Slots hold typed storage, so before the first a non-nullable value column would - /// otherwise read back as a perfectly plausible 0 / false / Guid.Empty rather than - /// as nothing. Answering a question the reader cannot yet answer, with a value indistinguishable from - /// real data, is the one failure mode worth spending a branch to prevent — so this reports the mistake - /// instead, matching what SqlClient and the rest of ADO.NET do. + /// Slots hold typed storage, so without this a non-nullable value column would read back before the first + /// as a perfectly plausible 0/false/Guid.Empty — a value + /// indistinguishable from real data. Worth a branch to report the mistake instead, as SqlClient and the + /// rest of ADO.NET do. /// private ColumnSlot Slot(int ordinal) { @@ -412,17 +407,15 @@ public override T GetFieldValue(int ordinal) /// already holds exactly that type. /// /// - /// The two sealed-class checks are ordered by cost. For a value-typed the - /// runtime JITs a dedicated instantiation, so each is a plain isinst against a known method table — - /// measured at 1.5–2.4x the cost of the unbox it replaces, against roughly 10 ns per column of decode. - /// A generic IValueGetter<T> interface implemented twice would let one slot serve both - /// long and long?, but it goes through the shared-generics dictionary and measured - /// 3.5–5.5x instead — so T = U? is deliberately left to the boxed fallback. + /// Both checks are sealed-class type tests, so for a value-typed each is a plain + /// isinst against a known method table. A generic IValueGetter<T> implemented twice would let + /// one slot serve both long and long?, but it goes through the shared-generics dictionary and + /// measured several times slower — so T = U? is deliberately left to the boxed fallback. /// - /// The fallback is the pre-slot expression verbatim, which is what preserves the exact-type - /// strictness callers depend on: GetFieldValue<long> over an Int32 column throws, it - /// does not widen, and reading a NULL as a non-nullable throws the runtime's - /// own "cannot cast DBNull" exactly as before. + /// That fallback is the pre-slot expression verbatim, which preserves the exact-type strictness callers + /// depend on: GetFieldValue<long> over an Int32 column throws rather than widening, and + /// reading a NULL as a non-nullable throws the runtime's own "cannot cast DBNull" + /// as before. /// private T GetSlotValue(int ordinal) { diff --git a/ClickHouse.Driver/ADO/Readers/ColumnSlot.cs b/ClickHouse.Driver/ADO/Readers/ColumnSlot.cs index ad35fba68..f055d4d10 100644 --- a/ClickHouse.Driver/ADO/Readers/ColumnSlot.cs +++ b/ClickHouse.Driver/ADO/Readers/ColumnSlot.cs @@ -5,18 +5,15 @@ namespace ClickHouse.Driver.ADO.Readers; /// -/// Per-column storage for the reader's current row. One instance per wire column, allocated once per -/// and overwritten in place on every . +/// Per-column storage for the reader's current row, replacing the shared object[] row buffer that boxed +/// every value-type cell of every row whether the caller asked for it or not. One instance per wire column, +/// allocated once per and overwritten in place on every +/// ; the box now happens only in . /// -/// This replaces the shared object[] row buffer, whose every value-type cell was boxed once per -/// value per row by — whether or not the caller ever -/// asked for that column. A slot decodes into strongly-typed storage instead, so the box happens only when -/// someone actually calls an untyped accessor (), and never at all for the typed ones. -/// -/// Every slot must be observationally identical to the boxed path it replaces: -/// has to return exactly what would have returned for -/// the same bytes, including for a SQL NULL, and has to consume -/// exactly the same bytes. +/// Every slot must be observationally identical to the boxed path it replaces: has to +/// consume exactly the bytes would, and +/// has to return exactly what it would have returned — same CLR type, and +/// for a SQL NULL. /// internal abstract class ColumnSlot { @@ -34,15 +31,14 @@ internal abstract class ColumnSlot } /// -/// A non-nullable column whose type can decode straight into . -/// is always the column type's , so the -/// stored value is exactly what the boxed produces. +/// A non-nullable column whose type can decode straight into , which is always the +/// column's — so the stored value is exactly what the boxed +/// produces. /// internal sealed class ValueSlot : ColumnSlot { - // typeof(T) is a JIT-time constant per closed generic, so this static folds to a constant load (the same - // trick ClickHouseDataReader.FieldValueDispatcher uses). For a value-typed T the IsNull check below - // then folds to `false` outright and the boxing conversion in it is never reached. + // typeof(T) is a JIT-time constant per closed generic, so this folds to a constant load; for a value-typed + // T the IsNull check below then folds to `false` and never reaches its boxing conversion. private static readonly bool CanBeNull = !typeof(T).IsValueType; private readonly ITypedReader typedReader; @@ -55,16 +51,14 @@ internal sealed class ValueSlot : ColumnSlot public override object GetBoxed() => Value; - // A non-nullable column has no null marker on the wire, so the only way this can be null is a - // reference-typed reader handing one back — none currently do, but the check keeps GetBoxed and IsNull - // agreeing with the boxed path if one ever did. + // A non-nullable column has no wire null marker, so this can only be null if a reference-typed reader hands + // one back. None currently do; the check keeps IsNull agreeing with GetBoxed if one ever did. public override bool IsNull => CanBeNull && (object)Value is null; } /// -/// A Nullable(T) column: the decoded value plus a presence flag, so a NULL costs no object at all -/// (the boxed path allocated nothing for it either — it returned the singleton — -/// but it did box every non-null cell). +/// A Nullable(T) column: the decoded value plus a presence flag, so a non-null cell no longer boxes +/// (a NULL never allocated — the boxed path returned the singleton). /// internal sealed class NullableSlot : ColumnSlot { @@ -78,8 +72,8 @@ internal sealed class NullableSlot : ColumnSlot public override void Read(ExtendedBinaryReader reader) { - // Byte-identical to NullableType.Read: a marker > 0 means NULL, and in that case the underlying type - // wrote nothing, so nothing more is consumed. + // Byte-identical to NullableType.Read: marker > 0 means NULL, and the underlying type then wrote + // nothing, so nothing more is consumed. if (reader.ReadByte() > 0) { HasValue = false; @@ -101,8 +95,8 @@ public override void Read(ExtendedBinaryReader reader) /// /// Fallback for any column with no for its own -/// — composites (Array, Tuple, Map, Nested), the polymorphic types -/// (Variant, Dynamic, JSON), geo types, and so on. Byte-for-byte and value-for-value the pre-slot behaviour. +/// — composites, the polymorphic types, geo, and so on. Byte-for-byte +/// and value-for-value the pre-slot behaviour. /// internal sealed class BoxedSlot : ColumnSlot { diff --git a/ClickHouse.Driver/ADO/Readers/ColumnSlotFactory.cs b/ClickHouse.Driver/ADO/Readers/ColumnSlotFactory.cs index 8c9d8e922..a774af13b 100644 --- a/ClickHouse.Driver/ADO/Readers/ColumnSlotFactory.cs +++ b/ClickHouse.Driver/ADO/Readers/ColumnSlotFactory.cs @@ -10,34 +10,27 @@ namespace ClickHouse.Driver.ADO.Readers; /// /// Builds the for a resolved column type. /// -/// A column gets a typed slot iff its type implements ITypedReader<FrameworkType> — that -/// is, iff it can decode straight into the very CLR type its boxed -/// would have returned. Every current -/// implementor satisfies that by construction (its Read body is the typed read, or picks -/// between typed reads by the same setting that picks FrameworkType), so a typed slot and the boxed -/// path always produce the same value from the same bytes. Anything else gets a . +/// A column gets a typed slot iff its type implements ITypedReader<FrameworkType> — iff it can +/// decode straight into the very CLR type its boxed Read would have returned, so that slot and boxed path +/// always produce the same value from the same bytes. Anything else gets a . /// /// Slots are built from the resolved type instance, never from a cached shape, so settings that -/// change FrameworkType — ReadStringsAsByteArrays (string vs byte[]), UseCustomDecimals -/// (decimal vs ClickHouseDecimal) — are handled without any of the cache-key hazards a per-query-shape cache -/// would have. +/// change FrameworkType (ReadStringsAsByteArrays, UseCustomDecimals) are handled without the +/// cache-key hazards a per-query-shape cache would have. /// internal static class ColumnSlotFactory { /// - /// CLR type → the slot constructor for it. Every entry is a static generic instantiation the compiler - /// emits, rather than a MakeGenericMethod built at runtime, so NativeAOT and trimming can see - /// every ValueSlot<T>/NullableSlot<T> the reader will ever need. Runtime generic - /// construction over value types is exactly what NativeAOT cannot satisfy, and this sits on the read path - /// of every scalar column, so it is worth spelling out. + /// CLR type → the slot constructor for it. Every entry is a static generic instantiation the compiler emits + /// rather than a runtime MakeGenericMethod — which NativeAOT cannot satisfy over value types — so every + /// ValueSlot<T>/NullableSlot<T> the reader needs is visible to AOT and trimming. /// /// - /// A few entries are unreachable today — , and the - /// native Int128/UInt128 are alternative read representations offered alongside a - /// type's FrameworkType, never as it. They are listed anyway so the table is exactly "every - /// ITypedReader<T> target", which - /// ColumnSlotTests.Binders_CoverEveryTypedReadTarget can then check mechanically. Over-inclusion - /// is inert; a missing entry would silently demote a column to the boxed path. + /// A few entries are unreachable today (, , native + /// Int128/UInt128): those are alternative representations offered alongside a type's + /// FrameworkType, never as it. They are listed anyway so the table is exactly "every + /// ITypedReader<T> target" and ColumnSlotTests.Binders_CoverEveryTypedReadTarget can check + /// it mechanically. Over-inclusion is inert; a missing entry silently demotes a column to the boxed path. /// private static readonly Dictionary> Binders = new() { @@ -88,25 +81,20 @@ public static ColumnSlot Create(ClickHouseType type) private static ColumnSlot TryCreateTyped(ClickHouseType type, bool nullable) { // Order matters, and not only for speed: FrameworkType is not safe to evaluate on every column type. - // AggregateFunctionType throws AggregateFunctionException from it (deliberately — you are meant to - // learn you need xMerge() when you read the value), and the composite types build a fresh Type object - // on each call. Testing the marker first is what keeps slot construction total: it has to work for - // every column of every row the reader is handed, and so may depend only on the FrameworkType of - // types that advertise a typed reader in the first place. - // - // ColumnSlotTests.Create_AggregateFunctionColumn_FallsBackToBoxedWithoutEvaluatingFrameworkType is - // the guard on this ordering. It has to be a unit test: end to end both orderings raise the same - // exception from the same Read() call, so no reader-level test can tell them apart. + // AggregateFunctionType throws from it (deliberately — you are meant to learn you need xMerge()), and the + // composite types build a fresh Type object per call. Slot construction has to succeed for every column + // of every reader, so it may only touch the FrameworkType of types that advertise a typed reader. + // Guarded by ColumnSlotTests.Create_AggregateFunctionColumn_FallsBackToBoxedWithoutEvaluatingFrameworkType, + // which has to be a unit test — end to end both orderings throw the same exception from the same call. if (type is not ITypedReader) return null; return Binders.TryGetValue(type.FrameworkType, out var bind) ? bind(type, nullable) : null; } - // Binds only when the type's typed reader is for its *own* FrameworkType — the CLR type its boxed Read - // returns. A type offering extra representations (a DateTime column also readable as DateTimeOffset, a - // Decimal column as either decimal or ClickHouseDecimal) must not have a slot bound to one the boxed path - // would not have produced, or GetValue would start handing back a different CLR type. + // Binds only when the type's typed reader is for its *own* FrameworkType. A type offering extra + // representations (a DateTime column also readable as DateTimeOffset) must not get a slot bound to one the + // boxed path would not have produced, or GetValue would start handing back a different CLR type. private static ColumnSlot Bind(ClickHouseType type, bool nullable) => type is not ITypedReader typedReader ? null : nullable ? new NullableSlot(typedReader) : new ValueSlot(typedReader); diff --git a/ClickHouse.Driver/Types/ITypedReader.cs b/ClickHouse.Driver/Types/ITypedReader.cs index 32ddfca2c..6b88910b1 100644 --- a/ClickHouse.Driver/Types/ITypedReader.cs +++ b/ClickHouse.Driver/Types/ITypedReader.cs @@ -3,27 +3,25 @@ namespace ClickHouse.Driver.Types; /// -/// Implemented by a that can deserialize a value of the CLR type -/// without boxing, for the POCO read (materialization) fast path. +/// Implemented by a that can deserialize without +/// boxing, for the box-free read paths (POCO materialization and the reader's column slots). /// -/// A type may implement this for more than one when it can produce several CLR -/// representations of the same column (e.g. a DateTime column as , -/// or ). Such implementations are explicit, -/// since they differ only by return type. The boxed -/// reads the same bytes and returns the canonical -/// representation, so every is byte-identical to it by construction. +/// A type may implement this for several when one column has more than one CLR +/// representation — a DateTime column as // +/// , a String column as /byte[]. Such implementations +/// are explicit, since they differ only by return type. Every must consume exactly +/// the bytes would. /// -/// The exact CLR type this type can read without boxing (e.g. ). +/// The exact CLR type read without boxing. internal interface ITypedReader : ITypedReader { T ReadValue(ExtendedBinaryReader reader); } /// -/// Non-generic base of , so "can this type read anything box-free?" is a plain -/// type test rather than an interface-list walk. -/// asks that question for every column of every reader, and has to ask it before touching -/// . +/// Non-generic base, so "can this type read box-free at all?" is a plain type test rather than an +/// interface-list walk. asks that for every +/// column, and must ask it before touching . /// internal interface ITypedReader { diff --git a/ClickHouse.Driver/Types/TransparentWrapper.cs b/ClickHouse.Driver/Types/TransparentWrapper.cs index b3c03eb16..a82c568fc 100644 --- a/ClickHouse.Driver/Types/TransparentWrapper.cs +++ b/ClickHouse.Driver/Types/TransparentWrapper.cs @@ -1,13 +1,10 @@ namespace ClickHouse.Driver.Types; /// -/// Column types that are pass-through on the RowBinary wire: their -/// and -/// delegate straight to the -/// wrapped type, and they report the wrapped type's . -/// -/// Any fast path that dispatches on the concrete column type has to look through these, or a wrapped -/// column silently falls back to the boxed path even though it decodes identically to a bare one. +/// Column types that are pass-through on the RowBinary wire: their Read/Write delegate straight +/// to the wrapped type and they report its . Any fast path +/// dispatching on the concrete column type has to look through these, or a wrapped column silently falls back +/// to the boxed path despite decoding identically to a bare one. /// internal static class TransparentWrapper { From 45015c6bb5ebd3a202c0019158dae7ddcf83114d Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Thu, 6 Aug 2026 15:28:50 +0200 Subject: [PATCH 14/16] docs: add the changelog fragments for the typed column slots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same conversion as the parent branch: #501 replaced direct Unreleased edits with changelog.d/ fragments, so the CHANGELOG.md/RELEASENOTES.md edits were dropped during the rebase and come back as files. The no-current-row throw becomes its own `breaking` fragment rather than a sub-bullet under Improvements. It is the one behaviour this stack does not preserve, the category now exists, and burying an exception change under a performance entry is how people miss it. Also amends the parent's POCO read fragment, which claimed the ADO accessors were unchanged — true until this branch. Co-Authored-By: Claude --- changelog.d/449-poco-read-boxfree.improvements.md | 2 +- changelog.d/499-ado-read-with-no-current-row.breaking.md | 1 + changelog.d/499-ado-typed-column-slots.improvements.md | 2 ++ .../499-simpleaggregatefunction-poco-read.improvements.md | 1 + 4 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 changelog.d/499-ado-read-with-no-current-row.breaking.md create mode 100644 changelog.d/499-ado-typed-column-slots.improvements.md create mode 100644 changelog.d/499-simpleaggregatefunction-poco-read.improvements.md diff --git a/changelog.d/449-poco-read-boxfree.improvements.md b/changelog.d/449-poco-read-boxfree.improvements.md index b90df4389..ff074bae8 100644 --- a/changelog.d/449-poco-read-boxfree.improvements.md +++ b/changelog.d/449-poco-read-boxfree.improvements.md @@ -1 +1 @@ -* Reduced allocations in POCO reads (`client.QueryAsync(...)`): scalar, `String` and `FixedString` columns are now materialized straight into the target property instead of through a boxed `object[]` row buffer, removing one box and one unbox per value-type property per row. A 500k-row read of a 3-column POCO (`Int64`, `String`, `Float64`) allocates about 38% less. Composite columns (`Array`, `Tuple`, `Map`, `Nested`, `Variant`, `Dynamic`, `JSON`) fall back per-column to the previous path. Values and their CLR types are unchanged, as are `MapTo` and the ADO accessors. +* Reduced allocations in POCO reads (`client.QueryAsync(...)`): scalar, `String` and `FixedString` columns are now materialized straight into the target property instead of through a boxed `object[]` row buffer, removing one box and one unbox per value-type property per row. A 500k-row read of a 3-column POCO (`Int64`, `String`, `Float64`) allocates about 38% less. Composite columns (`Array`, `Tuple`, `Map`, `Nested`, `Variant`, `Dynamic`, `JSON`) fall back per-column to the previous path. Values and their CLR types are unchanged, as is `MapTo`; the ADO accessors get their own box-free path (see below). diff --git a/changelog.d/499-ado-read-with-no-current-row.breaking.md b/changelog.d/499-ado-read-with-no-current-row.breaking.md new file mode 100644 index 000000000..29f4b22f1 --- /dev/null +++ b/changelog.d/499-ado-read-with-no-current-row.breaking.md @@ -0,0 +1 @@ +* **Reading a column value from `ClickHouseDataReader` with no current row now throws `InvalidOperationException`** — that is, before the first `Read()` or after `Read()` has returned `false`. It covers `GetValue`, the indexers, `GetValues`, `GetFieldValue`, `IsDBNull` and the typed accessors. Previously the result depended on which one you called: `GetValue` returned `null`, `IsDBNull` returned `true`, and a typed accessor threw `NullReferenceException`. Column metadata (`FieldCount`, `GetName`, `GetOrdinal`, `GetFieldType`, `GetDataTypeName`, `GetSchemaTable`) is still available without a row, and code that checks `Read()`'s return value is unaffected. diff --git a/changelog.d/499-ado-typed-column-slots.improvements.md b/changelog.d/499-ado-typed-column-slots.improvements.md new file mode 100644 index 000000000..6ba652092 --- /dev/null +++ b/changelog.d/499-ado-typed-column-slots.improvements.md @@ -0,0 +1,2 @@ +* Reduced allocations when reading through `ClickHouseDataReader`, and therefore through ORMs built on it such as Dapper and linq2db. The typed accessors (`GetInt64`, `GetDouble`, `GetDateTime`, `GetGuid`, …), `GetFieldValue` and `IsDBNull` no longer box each value, and columns you never read are no longer decoded at all. Allocations drop by roughly two thirds on a typical multi-column read, and to zero per row for all-numeric columns. Scalar columns benefit; composite columns (`Array`, `Tuple`, `Map`, `Variant`, `Dynamic`, `JSON`, geo) are unchanged. Values and their CLR types are unchanged, including exact-type strictness and the `DBNull.Value` representation of NULL. + - `GetValue`, `GetValues` and the indexers return `object` and so still box, but now only for the columns you actually ask for. Because they box per call, two `GetValue(i)` calls on the same value-type cell now return two distinct boxes — still equal by `Equals`, but no longer by `ReferenceEquals`. diff --git a/changelog.d/499-simpleaggregatefunction-poco-read.improvements.md b/changelog.d/499-simpleaggregatefunction-poco-read.improvements.md new file mode 100644 index 000000000..c30e68c86 --- /dev/null +++ b/changelog.d/499-simpleaggregatefunction-poco-read.improvements.md @@ -0,0 +1 @@ +* `SimpleAggregateFunction(f, T)` columns now take the box-free fast path in `QueryAsync`, as `LowCardinality(T)` already did. Previously they fell back to the slower boxed read. From 3223c0295c2a06c57a68e0b810e4d7abfcd9bc66 Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Fri, 7 Aug 2026 12:11:28 +0200 Subject: [PATCH 15/16] docs(ado): clarify slot allocation tradeoffs --- ClickHouse.Driver.Benchmark/AdoReadPathBenchmark.cs | 6 ++++-- changelog.d/499-ado-read-with-no-current-row.breaking.md | 2 +- changelog.d/499-ado-typed-column-slots.improvements.md | 4 ++-- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/ClickHouse.Driver.Benchmark/AdoReadPathBenchmark.cs b/ClickHouse.Driver.Benchmark/AdoReadPathBenchmark.cs index f9b37b4d8..2535b1df8 100644 --- a/ClickHouse.Driver.Benchmark/AdoReadPathBenchmark.cs +++ b/ClickHouse.Driver.Benchmark/AdoReadPathBenchmark.cs @@ -23,7 +23,8 @@ namespace ClickHouse.Driver.Benchmark; /// GetInt64/GetDouble/GetString/GetDateTime/GetGuid per column per row. /// — hand-written GetFieldValue<T> code. /// — the Dapper path. Its emitted IL calls the this[int] indexer, -/// i.e. GetValue, so it still boxes; this variant is the "must not regress" control. +/// i.e. GetValue, so it still boxes and pays one fixed slot allocation per returned column; this +/// variant is the "must not regress" control. /// — reads 2 of 10 columns, the case the old eager boxing /// punished hardest. /// @@ -35,7 +36,8 @@ public class AdoReadPathBenchmark private readonly Consumer consumer = new(); private ClickHouseConnection connection; - [Params(200000)] + // The short cases expose the fixed per-reader slot cost that a 200k-row allocation total rounds away. + [Params(1, 10, 200000)] public int Count { get; set; } // Ten columns, eight of them value types — the shape that used to box eight times per row. diff --git a/changelog.d/499-ado-read-with-no-current-row.breaking.md b/changelog.d/499-ado-read-with-no-current-row.breaking.md index 29f4b22f1..b3e8ec1d9 100644 --- a/changelog.d/499-ado-read-with-no-current-row.breaking.md +++ b/changelog.d/499-ado-read-with-no-current-row.breaking.md @@ -1 +1 @@ -* **Reading a column value from `ClickHouseDataReader` with no current row now throws `InvalidOperationException`** — that is, before the first `Read()` or after `Read()` has returned `false`. It covers `GetValue`, the indexers, `GetValues`, `GetFieldValue`, `IsDBNull` and the typed accessors. Previously the result depended on which one you called: `GetValue` returned `null`, `IsDBNull` returned `true`, and a typed accessor threw `NullReferenceException`. Column metadata (`FieldCount`, `GetName`, `GetOrdinal`, `GetFieldType`, `GetDataTypeName`, `GetSchemaTable`) is still available without a row, and code that checks `Read()`'s return value is unaffected. +* **Reading a column value from `ClickHouseDataReader` with no current row now throws `InvalidOperationException`** — that is, before the first `Read()` or after `Read()` has returned `false`; this covers `GetValue`, the indexers, `GetValues`, `GetFieldValue`, `IsDBNull` and the typed accessors. Before the first row the previous result depended on the accessor (`GetValue` returned `null`, `IsDBNull` returned `true`, and a typed accessor threw `NullReferenceException`), while after a non-empty result ended accessors continued exposing the last row; column metadata (`FieldCount`, `GetName`, `GetOrdinal`, `GetFieldType`, `GetDataTypeName`, `GetSchemaTable`) remains available without a row. diff --git a/changelog.d/499-ado-typed-column-slots.improvements.md b/changelog.d/499-ado-typed-column-slots.improvements.md index 6ba652092..2cba1db2e 100644 --- a/changelog.d/499-ado-typed-column-slots.improvements.md +++ b/changelog.d/499-ado-typed-column-slots.improvements.md @@ -1,2 +1,2 @@ -* Reduced allocations when reading through `ClickHouseDataReader`, and therefore through ORMs built on it such as Dapper and linq2db. The typed accessors (`GetInt64`, `GetDouble`, `GetDateTime`, `GetGuid`, …), `GetFieldValue` and `IsDBNull` no longer box each value, and columns you never read are no longer decoded at all. Allocations drop by roughly two thirds on a typical multi-column read, and to zero per row for all-numeric columns. Scalar columns benefit; composite columns (`Array`, `Tuple`, `Map`, `Variant`, `Dynamic`, `JSON`, geo) are unchanged. Values and their CLR types are unchanged, including exact-type strictness and the `DBNull.Value` representation of NULL. - - `GetValue`, `GetValues` and the indexers return `object` and so still box, but now only for the columns you actually ask for. Because they box per call, two `GetValue(i)` calls on the same value-type cell now return two distinct boxes — still equal by `Equals`, but no longer by `ReferenceEquals`. +* Reduced per-row allocations for `ClickHouseDataReader` typed accessors (`GetInt64`, `GetDouble`, `GetDateTime`, `GetGuid`, …), `GetFieldValue`, `IsDBNull` and ORMs such as linq2db by decoding scalar columns into reusable typed slots instead of eagerly boxing every value-type cell. Allocations drop by roughly two thirds on a typical multi-column typed read and to zero per row for all-numeric typed reads; composite columns (`Array`, `Tuple`, `Map`, `Variant`, `Dynamic`, `JSON`, geo), values, CLR types, exact-type strictness and the `DBNull.Value` representation of NULL are unchanged. + - `GetValue`, `GetValues` and the indexers — including Dapper's default materialization path — still box each requested value and pay a small fixed slot allocation per returned column. Because they box per call, two `GetValue(i)` calls on the same value-type cell now return two distinct boxes — still equal by `Equals`, but no longer by `ReferenceEquals`. From ba8461c69316305bc6f0269689b41bfec64b0c85 Mon Sep 17 00:00:00 2001 From: Alex Soffronow-Pagonidis Date: Wed, 12 Aug 2026 17:16:40 +0200 Subject: [PATCH 16/16] perf(ado): convert typed accessor reads on the generic overload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The typed accessors now read their slot unboxed even with an IReadValueConverter configured, and convert through ConvertValue — the overload GetFieldValue uses for a typed read. GetValue, GetValues and the coercing fall-throughs keep the boxed ConvertValue, so which overload runs follows how the value was read rather than whether a converter exists. This matches what QueryAsync does per column. GetBoolean, GetDecimal and GetString had their own converter guards; all three now take the slot path too. GetDecimal narrows a ClickHouseDecimal column to decimal before converting, so the converter sees the value the accessor returns. Retargets the two tests that pinned the previous decision, and adds a converter that transforms only in the generic overload so the decimal path is observable. 10664 tests pass on net9.0. --- .../ADO/BoxFreeReaderAccessorTests.cs | 45 +++++++---- .../ADO/ColumnSlotTests.cs | 2 +- .../ADO/Readers/ClickHouseDataReader.cs | 75 ++++++++++--------- ...9-typed-accessor-converter.improvements.md | 1 + 4 files changed, 72 insertions(+), 51 deletions(-) create mode 100644 changelog.d/499-typed-accessor-converter.improvements.md diff --git a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs index db50dda65..517e0e1d2 100644 --- a/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs +++ b/ClickHouse.Driver.Tests/ADO/BoxFreeReaderAccessorTests.cs @@ -435,13 +435,12 @@ public async Task GetDecimal_UnderEitherDecimalRepresentation_ReturnsTheSameValu // ---- Converter routing ---- - // Two paths, two overloads, both unchanged by column slots: the typed accessors were - // `(T)GetValue(ordinal)` and so saw ConvertValue(object, ...), while GetFieldValue called - // ConvertValue. De-boxing the accessors would have switched them onto ConvertValue, which is - // observable to a converter whose two overloads disagree, so with a converter configured they stay on the - // boxed route. This pins both halves of that decision. + // Which overload a read takes follows how it read, not whether a converter is configured: a value pulled + // unboxed out of its slot converts through ConvertValue, and only a genuinely boxed read uses + // ConvertValue(object, ...). This pins all three cases against a converter that doubles in the object + // overload alone, so the choice is directly observable. [Test] - public async Task WithConverter_TypedAccessorUsesObjectOverloadWhileGetFieldValueUsesGeneric() + public async Task WithConverter_TypedReadsUseTheGenericOverloadAndBoxedReadsUseTheObjectOverload() { var settings = TestUtilities.GetTestClickHouseClientSettings(); settings = new ClickHouseClientSettings(settings) { ReadValueConverter = new ObjectOnlyDoublingConverter() }; @@ -452,24 +451,31 @@ public async Task WithConverter_TypedAccessorUsesObjectOverloadWhileGetFieldValu Assert.Multiple(() => { - Assert.That(reader.GetInt64(0), Is.EqualTo(42L), "GetInt64 must still route through ConvertValue(object, ...)"); - Assert.That(reader.GetValue(0), Is.EqualTo(42L)); - Assert.That(reader.GetFieldValue(0), Is.EqualTo(21L), "GetFieldValue routes through ConvertValue, which this converter leaves alone"); + Assert.That(reader.GetInt64(0), Is.EqualTo(21L), "GetInt64 reads its slot unboxed, so it converts through ConvertValue"); + Assert.That(reader.GetFieldValue(0), Is.EqualTo(21L), "GetFieldValue converts through ConvertValue"); + Assert.That(reader.GetValue(0), Is.EqualTo(42L), "GetValue is a boxed read, so it stays on ConvertValue(object, ...)"); }); } - // GetDecimal's fast path is skipped when a converter is configured, so this exercises its boxed - // fallback — including the ClickHouseDecimal branch, which is what a Decimal column boxes as by default. + // A converter does not take GetDecimal off its slot path: a ClickHouseDecimal column is narrowed to decimal + // first, so the converter sees the decimal the accessor returns rather than the column's own + // representation. A NULL cell falls through to the boxed path and throws there. [Test] - public async Task GetDecimal_WithConverter_FallsBackToTheBoxedPath() + public async Task GetDecimal_WithConverter_ReadsTheSlotAndConvertsOnTheGenericOverload() { var settings = TestUtilities.GetTestClickHouseClientSettings(); - settings = new ClickHouseClientSettings(settings) { ReadValueConverter = new ObjectOnlyDoublingConverter() }; + settings = new ClickHouseClientSettings(settings) { ReadValueConverter = new GenericOnlyDecimalDoublingConverter() }; using var client = new ClickHouseClient(settings); - using var reader = await client.ExecuteReaderAsync("SELECT toDecimal64(12.34, 2) AS c"); + using var reader = await client.ExecuteReaderAsync( + "SELECT toDecimal64(12.34, 2) AS c, CAST(NULL AS Nullable(Decimal64(2))) AS z"); Assert.That(reader.Read(), Is.True); - Assert.That(reader.GetDecimal(0), Is.EqualTo(12.34m)); + + Assert.Multiple(() => + { + Assert.That(reader.GetDecimal(0), Is.EqualTo(24.68m)); + Assert.Throws(() => reader.GetDecimal(1)); + }); } // Doubles longs in the object overload only, so which overload an accessor picks is directly observable. @@ -481,6 +487,15 @@ public object ConvertValue(object value, string columnName, string clickHouseTyp public T ConvertValue(T value, string columnName, string clickHouseType) => value; } + // The mirror image, for the accessors whose slot value is a decimal. + private sealed class GenericOnlyDecimalDoublingConverter : IReadValueConverter + { + public object ConvertValue(object value, string columnName, string clickHouseType) => value; + + public T ConvertValue(T value, string columnName, string clickHouseType) + => value is decimal d ? (T)(object)(d * 2) : value; + } + // Reflection is the only way to parametrize over T. Unwraps TargetInvocationException so the assertions // above see the exception the caller would actually get. private static object GetFieldValueDynamic(ClickHouseDataReader reader, Type target) diff --git a/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs b/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs index 8296075ef..40100bbb1 100644 --- a/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs +++ b/ClickHouse.Driver.Tests/ADO/ColumnSlotTests.cs @@ -243,7 +243,7 @@ public void Create_ObjectWrappedColumn_ResolvesToWrappedTypedSlot() // The factory dispatches through a hand-written table of ValueSlot/NullableSlot constructors rather // than MakeGenericMethod, so that NativeAOT and trimming can see every instantiation the reader needs. - // The cost of giving up runtime generic construction is that the table no longer maintains itself: adding + // The cost of giving up runtime generic construction is that the table does not maintain itself: adding // an ITypedReader for a new T and forgetting the entry would silently demote that column to the boxed // path — values still correct, allocation quietly back. Nothing else would catch it, so this does. [Test] diff --git a/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs b/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs index 9cd9bbdd1..2095d2a04 100644 --- a/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs +++ b/ClickHouse.Driver/ADO/Readers/ClickHouseDataReader.cs @@ -166,35 +166,38 @@ internal ClickHouseType GetEffectiveClickHouseType(int ordinal) private protected ClickHouseType[] RawTypes { get; set; } /// - /// Shared body for the strict typed accessors — every one of which was (T)GetValue(ordinal) before - /// column slots and keeps exactly that meaning here. This is the path compiled ORM mappers drive (linq2db - /// inlines GetInt64/GetDouble/GetDateTime/… per column per row), so it is where the - /// boxing elimination is worth the most. + /// Shared body for the strict typed accessors, which cast rather than coerce: the value must already be + /// the requested . This is the path compiled ORM mappers drive (linq2db inlines + /// GetInt64/GetDouble/GetDateTime/… per column per row), so it is where the boxing + /// elimination is worth the most. /// /// - /// With an configured the accessor keeps routing through - /// , so it still calls ConvertValue(object, …) as before. De-boxing here would - /// switch it to ConvertValue<T>, which is observable to a converter whose two overloads disagree - /// (the generic one matching on typeof(T), the object one on the value's runtime type) — not worth a - /// silent semantic change for the rare converter case. stays on - /// ConvertValue<T>, the overload it already called, so both overloads are still reached. + /// An does not force the accessor onto the boxed path. The value comes + /// out of the slot unboxed and converts through ConvertValue<T>, the overload + /// uses for a typed read. Only genuinely boxed reads — + /// , , and the coercing fall-throughs below — use + /// ConvertValue(object, …). /// private T GetTypedValue(int ordinal) - => readValueConverter == null ? GetSlotValue(ordinal) : (T)GetValue(ordinal); + { + var value = GetSlotValue(ordinal); + return readValueConverter == null ? value : ConvertTyped(ordinal, value); + } + + // Applies the converter on the typed overload to a value already read out of its slot. + private T ConvertTyped(int ordinal, T value) + => readValueConverter.ConvertValue(value, FieldNames[ordinal], columnTypeNames[ordinal]); // Unlike its neighbours this one coerces rather than casts, so only an exact Bool column takes the fast path; // anything else keeps Convert.ToBoolean's widening and its exception messages. A NULL cell falls through too, // so Convert.ToBoolean(DBNull.Value) still throws exactly as it did. public override bool GetBoolean(int ordinal) { - if (readValueConverter == null) - { - var slot = Slot(ordinal); - if (slot is ValueSlot boolSlot) - return boolSlot.Value; - if (slot is NullableSlot nullableSlot && nullableSlot.HasValue) - return nullableSlot.Value; - } + var slot = Slot(ordinal); + if (slot is ValueSlot boolSlot) + return readValueConverter == null ? boolSlot.Value : ConvertTyped(ordinal, boolSlot.Value); + if (slot is NullableSlot nullableSlot && nullableSlot.HasValue) + return readValueConverter == null ? nullableSlot.Value : ConvertTyped(ordinal, nullableSlot.Value); return Convert.ToBoolean(GetValue(ordinal), CultureInfo.InvariantCulture); } @@ -218,21 +221,23 @@ public virtual DateTimeOffset GetDateTimeOffset(int ordinal) => GetEffectiveClic public override decimal GetDecimal(int ordinal) { - if (readValueConverter == null) + // Which of the two representations a Decimal column resolves to is the UseBigDecimal setting's doing; + // both reach the same decimal without a box, nullable or not. A NULL cell falls through to the boxed + // path below, where casting DBNull.Value throws exactly as it did. + var slot = Slot(ordinal); + decimal? unboxed = slot switch { - // Which of the two representations a Decimal column resolves to is the UseBigDecimal setting's doing; - // both reach the same decimal without a box, nullable or not. A NULL cell falls through to the boxed - // path below, where casting DBNull.Value throws exactly as it did. - var slot = Slot(ordinal); - if (slot is ValueSlot decimalSlot) - return decimalSlot.Value; - if (slot is NullableSlot nullableDecimalSlot && nullableDecimalSlot.HasValue) - return nullableDecimalSlot.Value; - if (slot is ValueSlot bigDecimalSlot) - return bigDecimalSlot.Value.ToDecimal(CultureInfo.InvariantCulture); - if (slot is NullableSlot nullableBigDecimalSlot && nullableBigDecimalSlot.HasValue) - return nullableBigDecimalSlot.Value.ToDecimal(CultureInfo.InvariantCulture); - } + ValueSlot decimalSlot => decimalSlot.Value, + NullableSlot n when n.HasValue => n.Value, + ValueSlot bigDecimalSlot => bigDecimalSlot.Value.ToDecimal(CultureInfo.InvariantCulture), + NullableSlot nb when nb.HasValue => nb.Value.ToDecimal(CultureInfo.InvariantCulture), + _ => null, + }; + + // A ClickHouseDecimal column is narrowed first, so the converter sees the decimal this accessor + // returns rather than the column's own representation. + if (unboxed.HasValue) + return readValueConverter == null ? unboxed.Value : ConvertTyped(ordinal, unboxed.Value); var value = GetValue(ordinal); return value is ClickHouseDecimal clickHouseDecimal ? clickHouseDecimal.ToDecimal(CultureInfo.InvariantCulture) : (decimal)value; @@ -273,8 +278,8 @@ public override int GetOrdinal(string name) // shape keeps ToString()'s coercion, including the quirk that a NULL cell yields "" rather than null, because // DBNull.Value.ToString() is the empty string. public override string GetString(int ordinal) - => readValueConverter == null && Slot(ordinal) is ValueSlot stringSlot - ? stringSlot.Value + => Slot(ordinal) is ValueSlot stringSlot + ? (readValueConverter == null ? stringSlot.Value : ConvertTyped(ordinal, stringSlot.Value)) : GetValue(ordinal)?.ToString(); /// diff --git a/changelog.d/499-typed-accessor-converter.improvements.md b/changelog.d/499-typed-accessor-converter.improvements.md new file mode 100644 index 000000000..dda066f31 --- /dev/null +++ b/changelog.d/499-typed-accessor-converter.improvements.md @@ -0,0 +1 @@ +* With an `IReadValueConverter` configured, `ClickHouseDataReader`'s typed accessors (`GetInt64`, `GetDouble`, `GetDecimal`, `GetString`, …) keep their reduced allocations: each reads its typed slot and converts through `ConvertValue`, the overload `GetFieldValue` uses. These accessors previously routed through `GetValue` and so through the boxed `ConvertValue`, so a converter whose two overloads return different results for the same cell now gets the typed result from them. `GetValue`, `GetValues` and the coercing fall-throughs still use the boxed overload.