diff --git a/CHANGELOG.md b/CHANGELOG.md index 31243a46d..014a7c639 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,12 @@ ### Bug Fixes +- **[client-v2]** Fixed `BigDecimal` values written into a `Dynamic` column being silently truncated when the + value's scale exceeded the inferred width's maximum scale, and throwing an overflow error when the value + carried an integer part (e.g. `19.99`). The `Dynamic` type inference now sizes the `Decimal` width to hold + both the integer digits and the value's scale, keeps the scale as wide as the width allows without stealing + room from the integer part, and writes the actual column scale into the `Dynamic` type tag. Values that + already round-tripped losslessly are unchanged. (https://github.com/ClickHouse/clickhouse-java/issues/2966) - **[client-v2]** Fixed the `RowBinary` writer throwing `UnsupportedOperationException: Unsupported data type: SimpleAggregateFunction` when inserting into a `SimpleAggregateFunction(func, T)` column (the reader already supported these columns). The value is now diff --git a/client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java b/client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java index 4b6cc5a03..b4b3b2aa0 100644 --- a/client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java +++ b/client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java @@ -209,23 +209,43 @@ public static ClickHouseColumn valueToColumnForDynamicType(Object value) { column = ClickHouseColumn.of("v", "DateTime64(9, " + ZoneId.systemDefault().getId() + ")"); } else if (value instanceof BigDecimal) { BigDecimal d = (BigDecimal) value; - String decType; - int scale; - if (d.precision() > ClickHouseDataType.Decimal128.getMaxScale()) { - decType = "Decimal256"; - scale = ClickHouseDataType.Decimal256.getMaxScale(); - } else if (d.precision() > ClickHouseDataType.Decimal64.getMaxScale()) { - decType = "Decimal128"; - scale = ClickHouseDataType.Decimal128.getMaxScale(); - } else if (d.precision() > ClickHouseDataType.Decimal32.getMaxScale()) { - decType = "Decimal64"; - scale = ClickHouseDataType.Decimal64.getMaxScale(); + // A DecimalN(S) column is Decimal(P, S) with P fixed to the width's precision + // (Decimal32=9, Decimal64=18, Decimal128=38, Decimal256=76) and 0 <= S <= P, so it can + // hold at most P - S integer digits. Size the width to fit both the integer digits and + // the value's own scale, then keep S as wide as the width allows without stealing room + // from the integer part. Keying the width off precision() alone and forcing S to the + // width maximum truncated values whose scale exceeded that maximum, and overflowed + // values that carried an integer part. + int valueScale = Math.max(d.scale(), 0); + int integerDigits = Math.max(d.precision() - d.scale(), 0); + int requiredPrecision = integerDigits + valueScale; + if (requiredPrecision > ClickHouseDataType.Decimal256.getMaxPrecision()) { + if (d.signum() != 0) { + throw new ClientException("Unable to serialize BigDecimal into a Dynamic column: it needs " + + requiredPrecision + " digits of precision, exceeding the maximum supported Decimal256 precision of " + + ClickHouseDataType.Decimal256.getMaxPrecision()); + } + // A numerically-zero value rounds to zero at any scale with no data loss, so it fits + // any Decimal width regardless of the scale/exponent implied by precision()/scale() + // (e.g. 0E-77 implies scale 77, 0E+77 implies 78 integer digits). Store it in the + // widest band rather than rejecting a value ClickHouse can represent exactly; cap the + // integer digits so the emitted scale (maxScale - integerDigits) stays non-negative. + integerDigits = Math.min(integerDigits, ClickHouseDataType.Decimal256.getMaxScale()); + requiredPrecision = ClickHouseDataType.Decimal256.getMaxPrecision(); + } + ClickHouseDataType decType; + if (requiredPrecision > ClickHouseDataType.Decimal128.getMaxPrecision()) { + decType = ClickHouseDataType.Decimal256; + } else if (requiredPrecision > ClickHouseDataType.Decimal64.getMaxPrecision()) { + decType = ClickHouseDataType.Decimal128; + } else if (requiredPrecision > ClickHouseDataType.Decimal32.getMaxPrecision()) { + decType = ClickHouseDataType.Decimal64; } else { - decType = "Decimal32"; - scale = ClickHouseDataType.Decimal32.getMaxScale(); + decType = ClickHouseDataType.Decimal32; } + int scale = decType.getMaxScale() - integerDigits; - column = ClickHouseColumn.of("v", decType + "(" + scale + ")"); + column = ClickHouseColumn.of("v", decType.name() + "(" + scale + ")"); } else if (value instanceof Map) { Map map = (Map) value; // TODO: handle empty map? @@ -382,7 +402,7 @@ public static void writeDynamicTypeTag(OutputStream stream, ClickHouseColumn typ case Decimal256: stream.write(binTag); BinaryStreamUtils.writeUnsignedInt8(stream, dt.getMaxPrecision()); - BinaryStreamUtils.writeUnsignedInt8(stream, dt.getMaxScale()); + BinaryStreamUtils.writeUnsignedInt8(stream, typeColumn.getScale()); break; case IntervalNanosecond: case IntervalMillisecond: diff --git a/client-v2/src/test/java/com/clickhouse/client/api/data_formats/internal/SerializerUtilsTest.java b/client-v2/src/test/java/com/clickhouse/client/api/data_formats/internal/SerializerUtilsTest.java index 2175d8ffd..1d807884f 100644 --- a/client-v2/src/test/java/com/clickhouse/client/api/data_formats/internal/SerializerUtilsTest.java +++ b/client-v2/src/test/java/com/clickhouse/client/api/data_formats/internal/SerializerUtilsTest.java @@ -1,5 +1,6 @@ package com.clickhouse.client.api.data_formats.internal; +import com.clickhouse.client.api.ClientException; import com.clickhouse.data.ClickHouseColumn; import com.clickhouse.data.ClickHouseDataType; import com.clickhouse.data.value.ClickHouseGeoPolygonValue; @@ -144,6 +145,54 @@ public void testGeometrySerializationRejectsMalformedList() { ClickHouseColumn.of("v", "Geometry"))); } + @DataProvider(name = "dynamicDecimalTypeInference") + public Object[][] dynamicDecimalTypeInference() { + return new Object[][]{ + // value, inferred width, inferred scale. The width must hold integerDigits + scale, + // and the scale is kept as wide as the width allows (maxScale - integerDigits). + {new BigDecimal("0.5"), ClickHouseDataType.Decimal32, 9}, // sub-1, unchanged + {new BigDecimal("19.99"), ClickHouseDataType.Decimal32, 7}, // integer part 2 + {new BigDecimal("-19.99"), ClickHouseDataType.Decimal32, 7}, // sign is irrelevant + {new BigDecimal("1000"), ClickHouseDataType.Decimal32, 5}, // integer, scale 0 + {new BigDecimal("1E3"), ClickHouseDataType.Decimal32, 5}, // negative scale (-3) + {new BigDecimal("0"), ClickHouseDataType.Decimal32, 8}, + {new BigDecimal("1.23456789"), ClickHouseDataType.Decimal32, 8}, // required precision == 9 boundary + {new BigDecimal("0.0123456789"), ClickHouseDataType.Decimal64, 18}, // scale 10 > Decimal32 max + {new BigDecimal("123456789.123456789"), ClickHouseDataType.Decimal64, 9}, + {new BigDecimal("0.00012345678901234567"), ClickHouseDataType.Decimal128, 38}, // scale 20 > Decimal64 max + {new BigDecimal("12345678901234567890.12345678901234567890"), ClickHouseDataType.Decimal256, 56}, // 20 int + 20 frac + {new BigDecimal("0.12345678901234567890123456789012345678901"), ClickHouseDataType.Decimal256, 76}, // scale 41 + // Numerically-zero values whose implied width exceeds Decimal256 still fit: zero rounds + // to zero at any scale with no loss, so they map to the widest band, not a rejection. + {new BigDecimal("0E-77"), ClickHouseDataType.Decimal256, 76}, // zero, scale 77 > max scale + {new BigDecimal("0E+100"), ClickHouseDataType.Decimal256, 0}, // zero, 101 integer digits > max + {new BigDecimal(BigInteger.ZERO, Integer.MAX_VALUE), ClickHouseDataType.Decimal256, 76}, // zero, maximal scale + }; + } + + @Test(dataProvider = "dynamicDecimalTypeInference") + public void testValueToColumnForDynamicTypeSizesDecimal(BigDecimal value, ClickHouseDataType expectedType, int expectedScale) { + ClickHouseColumn column = SerializerUtils.valueToColumnForDynamicType(value); + Assert.assertEquals(column.getDataType(), expectedType); + Assert.assertEquals(column.getScale(), expectedScale); + } + + @DataProvider(name = "oversizedDecimalValues") + public Object[][] oversizedDecimalValues() { + return new Object[][]{ + // Non-zero values that genuinely exceed Decimal256: fail loudly, never truncate. The + // zero-fits-any-width relaxation must NOT let these through — they carry real digits. + {new BigDecimal(BigInteger.TEN.pow(76))}, // 10^76: 77 integer digits, one past the width + {new BigDecimal(BigInteger.ONE, 77)}, // 1E-77: scale 77, one past the max scale + }; + } + + @Test(dataProvider = "oversizedDecimalValues") + public void testValueToColumnForDynamicTypeRejectsOversizedDecimal(BigDecimal value) { + Assert.assertThrows(ClientException.class, + () -> SerializerUtils.valueToColumnForDynamicType(value)); + } + @Test(dataProvider = "nonNullableEnumTypes") public void testNullIntoNonNullableEnumThrowsIllegalArgument(String typeName) { ClickHouseColumn column = ClickHouseColumn.of("bs_flag", typeName); diff --git a/client-v2/src/test/java/com/clickhouse/client/datatypes/DataTypeTests.java b/client-v2/src/test/java/com/clickhouse/client/datatypes/DataTypeTests.java index 3fc39ac46..27e5891be 100644 --- a/client-v2/src/test/java/com/clickhouse/client/datatypes/DataTypeTests.java +++ b/client-v2/src/test/java/com/clickhouse/client/datatypes/DataTypeTests.java @@ -981,6 +981,77 @@ public void testDynamicWithPrimitives() throws Exception { } } + @DataProvider(name = "dynamicDecimalValues") + public Object[][] dynamicDecimalValues() { + return new Object[][]{ + // Low-precision values whose scale exceeds the inferred width's maximum scale: + // previously silently truncated on write. + {new BigDecimal("0.0123456789")}, // scale 10 > Decimal32 max scale 9 + {new BigDecimal("0.00012345678901234567")}, // scale 20 > Decimal64 max scale 18 + {new BigDecimal("0.12345678901234567890123456789012345678901")}, // scale 41 -> Decimal256 + // Values carrying an integer part: previously overflowed on insert because the + // scale was forced to the full width precision, leaving no room for integer digits. + {new BigDecimal("19.99")}, + {new BigDecimal("-19.99")}, + {new BigDecimal("1000")}, + {new BigDecimal("1E3")}, // negative scale (-3) + {new BigDecimal("123456789.123456789")}, + {new BigDecimal("12345678901234567890.12345678901234567890")}, // 20 int + 20 frac -> Decimal256 + {new BigDecimal("0")}, + // A numerically-zero value whose scale exceeds any width: it fits (zero rounds to zero + // with no loss) and must round-trip rather than being rejected on insert. + {new BigDecimal("0E-77")}, // zero, scale 77 > Decimal256 max scale + }; + } + + @Test(groups = {"integration"}, dataProvider = "dynamicDecimalValues") + public void testDynamicColumnPreservesBigDecimalValue(BigDecimal value) throws Exception { + if (isVersionMatch("(,24.8]")) { + return; + } + final String table = "test_dynamic_decimal_roundtrip"; + final int tail = 987654321; + client.execute("DROP TABLE IF EXISTS " + table).get(); + client.execute(tableDefinition(table, "rowId Int32", "field Dynamic", "tail Int32"), + (CommandSettings) new CommandSettings().serverSetting("allow_experimental_dynamic_type", "1")).get(); + client.register(DTOForDynamicDecimalTests.class, client.getTableSchema(table)); + + client.insert(table, Collections.singletonList(new DTOForDynamicDecimalTests(1, value, tail))).get().close(); + + List rows = client.queryAll("SELECT field, tail FROM " + table); + Assert.assertEquals(rows.size(), 1); + GenericRecord row = rows.get(0); + Assert.assertEquals(row.getBigDecimal("field").compareTo(value), 0, + "Dynamic Decimal round-trip must not lose data for " + value.toPlainString()); + // The trailing fixed-width column stays intact only if the Decimal width written matches the + // type tag; a wrong width would shift the following bytes and corrupt it. + Assert.assertEquals(row.getInteger("tail"), tail); + } + + @Test(groups = {"integration"}) + public void testDynamicColumnFractionalDecimalRepresentationUnchanged() throws Exception { + if (isVersionMatch("(,24.8]")) { + return; + } + final String table = "test_dynamic_decimal_unchanged"; + final int tail = 123456789; + client.execute("DROP TABLE IF EXISTS " + table).get(); + client.execute(tableDefinition(table, "rowId Int32", "field Dynamic", "tail Int32"), + (CommandSettings) new CommandSettings().serverSetting("allow_experimental_dynamic_type", "1")).get(); + client.register(DTOForDynamicDecimalTests.class, client.getTableSchema(table)); + + // A sub-integer value (no integer part) whose scale fits the smallest width was already + // stored correctly, as Decimal32 with the scale padded to the width maximum. The fix must + // leave this representation byte-for-byte unchanged. + client.insert(table, Collections.singletonList(new DTOForDynamicDecimalTests(1, new BigDecimal("0.5"), tail))).get().close(); + + GenericRecord row = client.queryAll("SELECT field, tail FROM " + table).get(0); + BigDecimal readBack = row.getBigDecimal("field"); + Assert.assertEquals(readBack, new BigDecimal("0.500000000")); + Assert.assertEquals(readBack.scale(), 9); + Assert.assertEquals(row.getInteger("tail"), tail); + } + @Test(groups = {"integration"}) public void testDynamicWithArrays() throws Exception { testDynamicWith("arrays", @@ -1244,6 +1315,14 @@ public static class DTOForDynamicDefaultsTests { private String extra; } + @Data + @AllArgsConstructor + public static class DTOForDynamicDecimalTests { + private int rowId; + private Object field; + private int tail; + } + @Test(groups = {"integration"}) public void testAllDataTypesKnown() { List dbTypes = client.queryAll("SELECT * FROM system.data_type_families");