Repository navigation
Fix Dynamic: store decimals inside collections without truncating them to scale 9 - #640
Open
polyglotAI-bot wants to merge 2 commits into
Open
polyglotAI-bot wants to merge 2 commits into
polyglotAI-bot wants to merge 2 commits into
Conversation
…m to scale 9 A collection written as one Dynamic value (an array, a list, a dictionary or a tuple of decimal or ClickHouseDecimal) took its type from the cache keyed by the .NET type, whose decimal is fixed at Decimal128(9). ArrayType, MapType and TupleType write every element with that one element type, so each element with more than 9 fractional digits was truncated toward zero. DynamicType.Write now re-infers each decimal position of such a type from the values: TypeConverter.InferCommonDecimalType applies the scalar policy of #467 to the largest integer-digit count and the largest scale of the non-null values at that position. A single element gets the scalar type. Positions with no non-null value keep Decimal(38, 9). The value-dependent type is never cached; other types still come from the cache unchanged. When no Decimal256 holds all the values, the write throws instead of truncating. The 76-digit check ignores trailing fractional zeros, which a smaller scale holds exactly, for scalars too. Fixes: #636 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
polyglotAI-bot
requested review from
alex-clickhouse and
mzitnik
as code owners
October 1, 2026 12:18
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Decimal array inference introduces per-element boxing allocations in the binary-insert hot path.
Review effort: Balanced
Findings: 1
What changed in this PR
Fixes decimal precision loss in HTTP-client Dynamic collections and unhinted binary JSON paths.
Changes:
- Adds value-aware decimal type inference for nested collections.
- Expands real-server regression coverage.
- Updates documentation and changelog.
| File | Description |
|---|---|
ClickHouse.Driver/Types/TypeConverter.cs |
Infers common lossless decimal types. |
ClickHouse.Driver/Types/DynamicType.cs |
Applies inference across composite values. |
ClickHouse.Driver.Tests/Types/DynamicTests.cs |
Adds collection regression coverage. |
ClickHouse.Driver.Tests/Types/JsonTypeTests.cs |
Covers unhinted JSON decimal arrays. |
docs/http.mdx |
Documents inference behavior. |
changelog.d/636-dynamic-decimal-collections.fixes.md |
Records the fix. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ArrayType writes a decimal[] or ClickHouseDecimal[] without boxing its elements, but the type inference added for #636 enumerated them as object (one box per element), formatted each mantissa to a string to count its digits, and converted each decimal to ClickHouseDecimal (a BigInteger allocation once the mantissa exceeds 31 bits). - Array(Decimal) positions read decimal[] and ClickHouseDecimal[] elements through typed iterators. A position that holds only decimal[] uses a new InferCommonDecimalType(IEnumerable<decimal>) overload, which reads the scale and mantissa from the decimal bits. - Digit counts use a binary search over cached powers of ten instead of BigInteger.ToString().Length. Inference now allocates a constant ~0.5 KB per array instead of 88-113 B per element. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 1, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Summary
Fixes #636. Follow-up to #467.
A collection written as one
Dynamicvalue (decimal[],List<ClickHouseDecimal>,Dictionary<string, decimal>, a tuple, nested forms) took its type from the cache keyed by the .NET type. That type has a fixedDecimal(38, 9)element.ArrayType/MapType/TupleTypewrite every element with it, soScaleMantissa(value, 9)truncated each element with more than 9 fractional digits. Reproduced onmain(server 26.9):new[] { 0.0123456789012345m, 0.0000000001m }was stored asArray(Decimal(38, 9)).The fix follows the policy in the issue. For each decimal position in the type, the driver applies the #467 scalar policy to all the non-null values at that position together: the largest integer-digit count and the largest scale select the narrowest width with precision P, at scale
P - integerDigits. The example above is nowArray(Decimal(18, 18)), and[1.2m],[1.23m]and[1.2345m]are allArray(Decimal(9, 8)).Decimal(38, 9), the type it had before.Decimal256holds all the values at a position, the write throwsArgumentOutOfRangeException, wrapped byInsertBinaryAsync. It does not truncate.bool.Changes
Types/DynamicType.cs: the cache also stores whether the type contains a decimal. If it does,InferDecimalTypesrebuilds the type by walkingNullable/Array/Map/Tuplein parallel with the value. It covers jagged and multi-dimensional arrays,IDictionaryandList<KeyValuePair<,>>maps,TupleandValueTuple, and decimal map keys.Types/TypeConverter.cs:InferCommonDecimalType. It shares the width selection withInferDecimalType.{10^28, 1.000…0 (scale 48)}would be rejected, althoughmainwrites it exactly and it fitsDecimal(76, 47). The scalar check has the same change, so[x]andxalways agree. This only affects values that Fix Dynamic: store decimals with the client-v2 scale policy instead of truncating to scale 9 #467 (not yet released) rejected.JSONpaths inJsonWriteMode.Binarywrite throughDynamicType.Write, so they are fixed too.docs/http.mdxdescribes the collection rule, the fallback and the failure. A changelog fragment is added.No public API change. The TCP client is not affected: it rejects
decimalinside Dynamic composites and does not truncate them. Query parameters withdecimal[]take a different path, which #637 tracks.Test
New tests use
InsertBinaryAsyncagainst the real server and assertdynamicType()plus the numeric round trip:InsertBinaryAsync_DecimalCollectionToDynamic_StoresCommonTypeAndPreservesValues, which is parametrized:decimalandClickHouseDecimal, including negative and tiny high-scale values, and the largest scale first or last;Decimal256and values beyondSystem.Decimal;Tuple/ValueTuple, and an array of tuples.InsertBinaryAsync_DecimalArraysOfOneClrTypeToDynamic_InfersTypeForEachCell: successive cells of one .NET type with different types, which catches cache contamination.Read_DecimalCollectionFromDynamicWithoutCustomDecimals_ReturnsSystemDecimals:UseCustomDecimals=falsereads stored scales above 28 and mantissas above 96 bits.InsertBinaryAsync_DecimalCollectionWithNoCommonDecimal256TypeToDynamic_Throws: each value fits alone, but together they need 77 digits. Nothing is inserted.InsertBinaryAsync_WithUnhintedDecimalArrayScaleAbove9_ShouldPreserveAllFractionalDigits: the JSON path.1at scale 77 is stored asDecimal(76, 75).BoxFreeWriteAllocationTests.Write_DecimalArrayToDynamic_AllocatesNothingPerElementToInferType: writes a 1000-elementdecimal[]andClickHouseDecimal[]toDynamic, and subtracts the allocations of writing the same array as its inferred type. Before the second commit: 112.5 and 88.5 B per element. Now: a constant ~0.5 KB per array.TypeMappingTests.InferDecimalType_IntegerAtDigitBoundary_KeepsEveryIntegerDigitandInferCommonDecimalType_DecimalAtDigitBoundary_MatchesClickHouseDecimal: the digit count at 10^n - 1, 10^n, 2^n - 1 and 2^n (up to 76 digits), and thedecimaloverload against theClickHouseDecimalone.On
main, all the new real-server cases fail except the two fallback cases, which pin the unchangedDecimal(38, 9). With the fix, the full HTTP suite passes on net10.0: 11121 passed, 0 failed, 142 skipped by version gates.Performance
Array(Decimal)writes adecimal[]orClickHouseDecimal[]without boxing its elements, so the inference reads them the same way. A position that holds onlydecimal[]reads scale and mantissa from thedecimalbits, so noBigIntegeris built. Digits are counted with a binary search over cached powers of ten, notToString().Length. Other collections (List<T>, maps, tuples,decimal?[], multi-dimensional arrays) already box on their write paths, and they keep the generic path.BenchmarkDotNet,
InsertBinaryAsyncof 100 rows × 10,000-element arrays (1M elements per operation) into aNull-engine table. The values have 5 integer and 16 fractional digits. The typed-column rows insert the same arrays intoArray(Decimal(38, 33)), the type that Dynamic now infers, so no inference runs:main(truncates to scale 9)decimal[]main(truncates to scale 9)ClickHouseDecimal[]decimal[]ClickHouseDecimal[]decimal[]ClickHouseDecimal[]The difference between
mainand the typed column is the write at the correct scale:ScaleMantissamultiplies by 10^17 instead of dividing by 10^7. In-process, the inference allocates 416–440 B per 10,000-element array. Its CPU cost is 2–3% of the write (best of 9: 7.00 → 7.23 ms fordecimal[]and 6.80 → 6.92 ms forClickHouseDecimal[]per array). The end-to-end mean times on the shared host had a StdDev of 75–260 ms, so I do not quote them.Benchmark source (not committed)
The typed-column rows used two extra methods that insert the same row lists into
test.benchmark_typed_decimal_array (value Array(Decimal(38, 33))) ENGINE Null.Checklist
changelog.d/636-dynamic-decimal-collections.fixes.md)docs/http.mdx)🤖 Generated with Claude Code