Repository navigation
[client-v2] Out-of-range numeric values are silently wrapped when inserted into Int32/Int64/UInt64 columns (Int8/Int16 correctly throw) #3176
Description
Activity
@chernser I have a fix ready for this. The root cause is one thing rather than the per-type table in the issue: the value is narrowed to a Java primitive before any range check runs, so the check is handed an already-wrapped number.
That also makes the narrow types wrong, which the issue lists as correct —
2^32written to anInt8column becomes0throughintValue(), which then passes theByterange check inBinaryStreamUtilsand is stored. Same forInt16/UInt8/UInt16. So it is the narrowing, not a missing check.The fix keeps the full magnitude during conversion so the range checks that already exist see the value the caller passed:
NumberConverter.toBigIntegergoes throughtoBigDecimal(already exact for everyNumber) instead ofBigInteger.valueOf(longValue()), andconvertToInteger/convertToLongrange-check anything that is not already aByte/Short/Integer/Long, so the common path stays allocation-free. Only the magnitude is checked — a fractional value is still truncated toward zero, and every value that fits its column serializes exactly as before.Before I open it, one question, since this is a behaviour change on the insert path:
Is
IllegalArgumentExceptionon out-of-range the behaviour you want? It matches whatInt8/Int16already do today, so the write path would report one exception type instead of two behaviours — but it is breaking for anyone currently relying on the wrap. I am happy to put it behind a setting instead, or to default to lenient with a warning, if you would rather not change this by default.Two smaller points for the same decision:
NaNand the infinities are rejected by the same change, where they previously stored0andLong.MAX_VALUE.UInt64is narrower than the issue suggests: aBigIntegerargument already passes through untouched and is rejected correctly. Only a non-BigIntegerNumber(such as aBigDecimal) loses its value.
Locally: 84 unit tests pass with the fix, and 15 of them fail with the production change reverted, which are exactly the newly-fixed cases.
Good day, @piyush15102003 !
Thank you!
This is the point where we may be need two options - in most production setup we should not check each value range and just use wider type or accept it is not safe (because we know it is safe). So we might need two versions of converters.
The checked path is two
longcomparisons forByte/Short/Integer/Longwith no allocation — onlyBigDecimal/BigInteger/floating values take the slow path. I'll benchmark the insert path and post numbers.If it's noise, I'd keep one converter. If you still want two, I'll select the implementation when the serializer is built rather than per value, checked by default — say if you want the opposite default.
One note: an
Int32column with 2^33 has no wider type to pick, so unchecked means storing a different number.Benchmarked. My "two
longcomparisons, no allocation" was wrong — that only held forInteger.JMH,
convertToIntegerover 1024 values per op, ns per value:old new Integer 1.4 1.6 no difference Long 8.8 11.5 within noise here BigInteger 11.3 22.0 ~2x BigDecimal 21.2 71.1 ~3.4xThe first run was worse (
Long+57%,BigInteger2.9x,BigDecimal8x). Two of those were my code rather than the check itself:Longfell through a genericinstanceofchain instead of comparing directly, and theBigIntegerpath boxed both bounds withBigInteger.valueOfand ran twocompareToper value. Both fixed —bitLength()is 63 forLong.MIN_VALUEandMAX_VALUE, so one scan rules out anything too wide and the bounds compare as longs.What's left is real.
BigDecimalhas to be converted exactly before its magnitude can be checked at all, so that cost doesn't go away.IntegerandLong— what a POJOint/longfield produces — are free or close to it.So your call stands up: worth an opt-out for
BigDecimal/BigInteger-heavy inserts, not worth it for primitives.Two caveats. This measures
convertToIntegeronly (theInt32path), notInt64/UInt64or a full row throughRowBinaryFormatWriter. And it ran on a laptop with wide error bars — theLongrow's intervals overlap, so that one needs a quiet host before anyone trusts it.
Describe the bug
SerializerUtils.serializePrimitiveDatanarrows the user-supplied value to a Java primitive without a range check for some integer column types. When the value does not fit, the extra bits are dropped silently and a wrong number is stored — no exception is raised and nothing is logged.The behaviour is inconsistent across widths:
Int8,Int16,UInt8,UInt16convertToInteger→BinaryStreamUtils.writeInt8/16/...ClickHouseChecker.betweenthrowsIllegalArgumentExceptionInt32convertToInteger(value)→((Number) value).intValue()Int64,UInt32convertToLong(value)→((Number) value).longValue()Int64)UInt64,Int128,UInt128,Int256,UInt256NumberConverter.toBigInteger(value)→BigInteger.valueOf(((Number) value).longValue())The module even has a correct helper already —
NumberConverter.toInt/toLongcompareintValue()againstlongValue()and throwArithmeticException("integer overflow: ...")— butserializePrimitiveDatadoes not call it for these branches.The server rejects such a narrowing (
SELECT toInt32(toDecimal64(4294967296, 0))→DECIMAL_OVERFLOW), so the client is silently more permissive than the server and corrupts data.ClickHouse server version
26.9.7.9(local server athttp://localhost:8123). The test below was run against it.Reproduction
TestNG test in
client-v2:Actual output (observed,
mainat74b0a62-era checkout,0.11.0-rc1-SNAPSHOT)Expected output
Rows 2, 3 and 4 should be rejected the same way the
Int16row is (anArithmeticException/IllegalArgumentExceptionabout integer overflow), and theInt64/UInt64row with2^64should be rejected too. The in-range row (100000→100000) is already correct and must stay correct.Instead
4294967296becomes0,2147483648becomes-2147483648, and2^64becomes0in both theInt64and theUInt64column — silently.Suggested fix
In
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java:serializePrimitiveData(around lines 649-685): use the already-existing range-checking helpersNumberConverter.toInt(value)forInt32,NumberConverter.toLong(value)forInt64, and a checked conversion forUInt32/UInt64instead ofconvertToInteger/convertToLong.convertToInteger(around line 1050) andconvertToLong(around line 1064): the bare((Number) value).intValue()/longValue()is where the bits are dropped. Either add the overflow check here or stop using these for the fixed-width integer branches.NumberConverter.toBigInteger(around line 117 ofNumberConverter.java):BigInteger.valueOf(((Number) value).longValue())loses the high bits of aBigDecimal/BigIntegerargument; it should use((BigDecimal) value).toBigIntegerExact()forBigDecimaland pass aBigIntegerthrough unchanged, then let thewriteUnsignedInt64/writeInt128/... range checks apply.Link
Found while checking whether ClickHouse/clickhouse-cs#646 (unchecked narrowing in
ClickHouseDecimal'sIConvertiblemembers) also affects this client. The two C#-specific defects in that report (the(short)typo inToInt32, and theConvert.ChangeTypestack overflow) have no counterpart here — clickhouse-java usesjava.math.BigDecimaland has noIConvertible/Convert.ChangeTypeequivalent. The third defect — out-of-range narrowing wrapping instead of raising an overflow error — does reproduce here, atInt32/Int64/UInt64rather than atInt8/Int16.Tracking: ClickHouse/integrations-ai-playground#532