diff --git a/ClickHouse.Driver.Tests/Types/DynamicTests.cs b/ClickHouse.Driver.Tests/Types/DynamicTests.cs index d0f05b6a0..ddda9f2df 100644 --- a/ClickHouse.Driver.Tests/Types/DynamicTests.cs +++ b/ClickHouse.Driver.Tests/Types/DynamicTests.cs @@ -373,6 +373,55 @@ await connection.ExecuteStatementAsync( Assert.That(result, Is.EqualTo(new ClickHouseDecimal(123.456789m))); } + // Regression coverage for issue #466: a decimal written to a Dynamic column was inferred from + // the .NET type via a per-Type cache that hardcoded Decimal128(38, 9), so any value with scale + // greater than 9 was silently truncated on write. These cases exercise scales the fixed scale + // could not represent, spanning the Decimal32/64/128/256 widths, plus a low-scale contrast case + // that must keep round-tripping unchanged. + public static IEnumerable DynamicDecimalRoundTripCases + { + get + { + yield return new TestCaseData(0.0123456789012345m, "scale16").SetName("Write_DynamicDecimal_Scale16"); + yield return new TestCaseData(0.0000000001m, "tiny_scale10").SetName("Write_DynamicDecimal_TinyScale10"); + yield return new TestCaseData(12345.6789012345m, "intpart_scale10").SetName("Write_DynamicDecimal_IntegerPartScale10"); + // 28 significant digits with scale 10: the width must be chosen by the digit count + // (forces Decimal128), not the scale alone (which would only need Decimal64). + yield return new TestCaseData(123456789012345678.0123456789m, "digits28_scale10").SetName("Write_DynamicDecimal_DigitsDominatedScale10"); + yield return new TestCaseData(-0.0123456789012345m, "negative_scale16").SetName("Write_DynamicDecimal_NegativeScale16"); + yield return new TestCaseData(0.1234567890123456789012345678m, "scale28").SetName("Write_DynamicDecimal_MaxDecimalScale28"); + // Scale 40 is beyond System.Decimal's 28-digit limit, so it can only arrive as a + // ClickHouseDecimal; it must widen to Decimal256. + yield return new TestCaseData( + new ClickHouseDecimal(BigInteger.Parse("123456789012345678901234567890"), 40), "scale40") + .SetName("Write_DynamicClickHouseDecimal_Scale40"); + // Contrast: scale 6 already fit inside the old fixed scale of 9 and round-tripped; it must + // keep round-tripping (now via a narrower Decimal32) with the same value. + yield return new TestCaseData(123.456789m, "contrast_scale6").SetName("Write_DynamicDecimal_Scale6_Contrast"); + } + } + + [Test] + [RequiredFeature(Feature.Dynamic)] + [TestCaseSource(typeof(DynamicTests), nameof(DynamicDecimalRoundTripCases))] + public async Task Write_DecimalWithScaleAbove9_ShouldRoundTripWithoutTruncation(object value, string caseId) + { + var targetTable = CreateTableName($"dynamic_write_decimal_{caseId}"); + await connection.ExecuteStatementAsync( + $"CREATE OR REPLACE TABLE {targetTable} (id UInt32, value Dynamic) ENGINE = Memory"); + + var expected = value is ClickHouseDecimal chd ? chd : new ClickHouseDecimal((decimal)value); + + using var bulkCopy = new ClickHouseBulkCopy(connection) { DestinationTableName = targetTable }; + await bulkCopy.WriteToServerAsync([new object[] { 1u, value }]); + + using var reader = await connection.ExecuteReaderAsync($"SELECT value FROM {targetTable}"); + ClassicAssert.IsTrue(reader.Read()); + var result = (ClickHouseDecimal)reader.GetValue(0); + Assert.That(result, Is.EqualTo(expected)); + ClassicAssert.IsFalse(reader.Read()); + } + [Test] [RequiredFeature(Feature.Dynamic)] public async Task Write_IntArray_ShouldRoundTrip() diff --git a/ClickHouse.Driver.Tests/Types/TypeMappingTests.cs b/ClickHouse.Driver.Tests/Types/TypeMappingTests.cs index 349c5b810..945ae8e83 100644 --- a/ClickHouse.Driver.Tests/Types/TypeMappingTests.cs +++ b/ClickHouse.Driver.Tests/Types/TypeMappingTests.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Linq; using System.Net; +using System.Numerics; using ClickHouse.Driver.ADO.Parameters; using ClickHouse.Driver.Formats; using ClickHouse.Driver.Numerics; @@ -196,6 +197,45 @@ private static IEnumerable ValueToClickHouseTypeCases() [TestCaseSource(nameof(ValueToClickHouseTypeCases))] public string ShouldConvertValueToClickHouseType(object value) => TypeConverter.ToClickHouseType(value).ToString(); + private static IEnumerable InferDecimalTypeCases() + { + // InferDecimalType chooses the narrowest Decimal width whose precision (the max of the + // value's significant digits and its scale) represents the value, and carries the value's + // own scale so no fractional digits are lost. Cases drive precision by scale and by digit + // count and sit on both edges of every width threshold, so an off-by-one in a `<=` bound — + // or a dropped BigInteger.Abs — would fail. Args: (mantissa, scale) => "Decimal()". + yield return new TestCaseData(BigInteger.Zero, 0).Returns("Decimal32(0)"); // precision 1: the Math.Max(..., 1) floor + yield return new TestCaseData(BigInteger.Parse("123456789"), 4).Returns("Decimal32(4)"); // precision 9: Decimal32 upper edge, digit-driven + yield return new TestCaseData(BigInteger.One, 10).Returns("Decimal64(10)"); // precision 10: Decimal64 lower edge, scale-driven (the #466 0.0000000001 case) + yield return new TestCaseData(BigInteger.One, 18).Returns("Decimal64(18)"); // precision 18: Decimal64 upper edge + yield return new TestCaseData(BigInteger.Parse("1234567890123456789"), 0).Returns("Decimal128(0)"); // precision 19: Decimal128 lower edge, digit-driven + yield return new TestCaseData(BigInteger.One, 38).Returns("Decimal128(38)"); // precision 38: Decimal128 upper edge + yield return new TestCaseData(BigInteger.One, 39).Returns("Decimal256(39)"); // precision 39: Decimal256 lower edge + yield return new TestCaseData(BigInteger.One, 76).Returns("Decimal256(76)"); // precision 76: Decimal256 upper edge + yield return new TestCaseData(BigInteger.Parse("-123456789"), 6).Returns("Decimal32(6)"); // negative: the sign is not a digit (|mantissa| has 9 digits -> Decimal32) + } + + [Test] + [TestCaseSource(nameof(InferDecimalTypeCases))] + public string InferDecimalType_ForValueScale_SelectsNarrowestWidthCarryingValueScale(BigInteger mantissa, int scale) + => TypeConverter.InferDecimalType(new ClickHouseDecimal(mantissa, scale)).ToString(); + + private static IEnumerable InferDecimalTypeOverflowCases() + { + // A precision above 76 exceeds the capacity of the widest ClickHouse Decimal (Decimal256), + // reachable either through the scale alone or through the significant-digit count. + yield return new TestCaseData(BigInteger.One, 77).SetName("InferDecimalType_ScaleAbove76_Throws"); + yield return new TestCaseData(BigInteger.Parse(new string('9', 77)), 0).SetName("InferDecimalType_DigitsAbove76_Throws"); + } + + [Test] + [TestCaseSource(nameof(InferDecimalTypeOverflowCases))] + public void InferDecimalType_ForPrecisionAbove76_ThrowsArgumentOutOfRangeException(BigInteger mantissa, int scale) + { + var value = new ClickHouseDecimal(mantissa, scale); + Assert.Throws(() => TypeConverter.InferDecimalType(value)); + } + private static IEnumerable NonZeroBoundMultidimCases() { // Rank 2, single non-zero lower bound diff --git a/ClickHouse.Driver/Types/DynamicType.cs b/ClickHouse.Driver/Types/DynamicType.cs index 7f03678de..cfc7993cd 100644 --- a/ClickHouse.Driver/Types/DynamicType.cs +++ b/ClickHouse.Driver/Types/DynamicType.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Concurrent; using ClickHouse.Driver.Formats; +using ClickHouse.Driver.Numerics; namespace ClickHouse.Driver.Types; @@ -24,7 +25,8 @@ public override object Read(ExtendedBinaryReader reader) => /// /// Writes a value with its type header for dynamic type encoding. - /// The type is inferred from the value's .NET type and cached. + /// The type is inferred from the value's .NET type and cached, except for decimals, whose + /// ClickHouse scale/width depend on the value itself and so are inferred per value. /// public override void Write(ExtendedBinaryWriter writer, object value) { @@ -33,7 +35,18 @@ public override void Write(ExtendedBinaryWriter writer, object value) writer.Write(BinaryTypeIndex.Nothing); return; } - var inferredType = GetCachedInferredType(value.GetType()); + + // Decimals must be inferred from the value, not just its .NET type: the ClickHouse scale is + // derived from the value's own scale, so the per-Type cache (which cannot vary by value) + // would truncate any value whose scale exceeds the cached type's fixed scale (issue #466). + ClickHouseType inferredType; + if (value is ClickHouseDecimal chd) + inferredType = TypeConverter.InferDecimalType(chd); + else if (value is decimal dec) + inferredType = TypeConverter.InferDecimalType(dec); // implicit decimal -> ClickHouseDecimal + else + inferredType = GetCachedInferredType(value.GetType()); + BinaryTypeDescriptionWriter.WriteTypeHeader(writer, inferredType); inferredType.Write(writer, value); } diff --git a/ClickHouse.Driver/Types/TypeConverter.cs b/ClickHouse.Driver/Types/TypeConverter.cs index d417fd7b5..7c8af1211 100644 --- a/ClickHouse.Driver/Types/TypeConverter.cs +++ b/ClickHouse.Driver/Types/TypeConverter.cs @@ -1,8 +1,10 @@ using System; using System.Collections.Generic; +using System.Globalization; using System.Linq; using System.Net; using System.Net.Sockets; +using System.Numerics; using System.Runtime.CompilerServices; using System.Text.Json.Nodes; using ClickHouse.Driver.Numerics; @@ -498,6 +500,43 @@ public static ClickHouseType ToClickHouseType(object value) return ToClickHouseType(type); } + /// + /// Infers the narrowest ClickHouse Decimal type that represents + /// without losing precision, deriving the scale from the value's own scale. + /// + /// Unlike the type-based mapping (which fixes the scale at a constant), this is value-aware and + /// is required wherever the target type is chosen from the value rather than a column definition + /// — e.g. writing into a Dynamic column. A fixed scale silently truncates any value whose + /// scale exceeds it (issue #466). + /// + /// + /// The decimal value to infer a type for. + /// A whose equals the value's scale. + /// + /// The value needs more than 76 significant digits, which exceeds the capacity of ClickHouse's widest Decimal256. + /// + internal static ClickHouseType InferDecimalType(ClickHouseDecimal value) + { + var scale = value.Scale; + // Significant digits of the mantissa == the digits stored when the ClickHouse scale equals + // the value's own scale. The precision must cover those digits, and can never be smaller + // than the scale (ClickHouse requires scale <= precision). + var digits = BigInteger.Abs(value.Mantissa).ToString(CultureInfo.InvariantCulture).Length; + var precision = Math.Max(Math.Max(digits, scale), 1); + + return precision switch + { + <= 9 => new Decimal32Type { Scale = scale }, + <= 18 => new Decimal64Type { Scale = scale }, + <= 38 => new Decimal128Type { Scale = scale }, + <= 76 => new Decimal256Type { Scale = scale }, + _ => throw new ArgumentOutOfRangeException( + nameof(value), + value, + $"Decimal value requires a precision of {precision} digits, which exceeds the maximum of 76 supported by ClickHouse (Decimal256)."), + }; + } + private static bool IsKeyValuePairType(Type type) => type.IsGenericType && type.GetGenericTypeDefinition() == typeof(KeyValuePair<,>); diff --git a/changelog.d/467-dynamic-decimal-scale.fixes.md b/changelog.d/467-dynamic-decimal-scale.fixes.md new file mode 100644 index 000000000..b50a8a5e5 --- /dev/null +++ b/changelog.d/467-dynamic-decimal-scale.fixes.md @@ -0,0 +1 @@ +* Fixed silent decimal precision loss when writing a `decimal` or `ClickHouseDecimal` into a `Dynamic` column. The ClickHouse type was inferred from the .NET type via a per-type cache that hardcoded `Decimal128(38, 9)`, so any value with a scale above 9 was truncated toward zero on write (e.g. `0.0000000001` was stored as `0`). The `Dynamic` write path now derives the scale from the value itself and selects the narrowest Decimal width (`Decimal32`/`Decimal64`/`Decimal128`/`Decimal256`) that preserves it, throwing only when the value needs more than the 76 digits ClickHouse supports (issue #466).