ClickHouse / ClickHouse/clickhouse-java

client-v2: serializePrimitiveData rejects or silently corrupts String values for UInt64, UUID, Date and DateTime

Đang mở
#3,132 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
Java
Star
1.6k
Fork
636
Merge trung bình
2 ngày 23 giờ
Pull request đã merge (30 ngày)
29

Mô tả

## Summary

`SerializerUtils.serializePrimitiveData` accepts `String` input for most scalar column types via the `convertToInteger` / `convertToLong` / `convertToBigInteger` / `convertToBigDecimal` / `convertToString` helpers, but a subset of types either reject a `String` outright or — for `UInt64` — silently corrupt it. This makes String coercion inconsistent across the type switch.

Line references are against `0.9.5`, file `client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java`.

## Behaviour today

| Column type | String input | Result |
|---|---|---|
| `Int8/16/32`, `UInt8/16`, `Int64`, `UInt32` | `convertToInteger` / `convertToLong` | works |
| `Int128/256`, `UInt128/256` | `convertToBigInteger` | works |
| `Decimal*` | `convertToBigDecimal` | works |
| `String`, `FixedString` | `convertToString` | works |
| **`UInt64`** (`:522`) | `convertToLong` → `Long.parseLong` (`:760`) | **throws above 2^63-1; silently wraps a negative** |
| **`UUID`** (`:575`) | `(UUID) value` | `ClassCastException` |
| **`Date`, `Date32`** (`:553`, `:556`) | `writeDate` / `writeDate32` (`:1105`) | `IllegalArgumentException: Cannot convert … to Long` |
| **`DateTime`, `DateTime64`** (`:559`, `:563`) | `writeDateTime` / `writeDateTime64` (`:1141`) | same |

### The `UInt64` case is a silent-corruption bug

```java
convertToLong("-1") // -> -1L (Long.parseLong, signed)
writeUnsignedInt64(-1L) // -> writeInt64 -> FF FF FF FF FF FF FF FF
```

ClickHouse reads that back as `18446744073709551615`. So a negative String is written as the maximum `UInt64` with no error.

This is already inconsistent within the library: `BinaryStreamUtils.writeUnsignedInt64(OutputStream, BigInteger)` range-checks via `ClickHouseChecker.between(value, TYPE_BIG_INTEGER, BigInteger.ZERO, U_INT64_MAX)`, so a `BigInteger` of `-1` is correctly rejected. Only the String path wraps.

Values in `[2^63, 2^64-1]` are legal `UInt64` values and are encodable on the wire (`writeUnsignedInt64(long)` just emits the raw 8 bytes), but unreachable through the String path.

## Proposed fix

1. **`UInt64`** — use an unsigned-aware conversion at `:522` rather than changing the shared `convertToLong`, which `Int64` (`:504`) and `UInt32` (`:519`) also use and which needs signed parsing:

```java
/** UInt64 runs past Long.MAX_VALUE; the returned long carries the raw bit pattern. */
public static Long convertToUnsignedLong(Object value) {
if (value instanceof String) {
return Long.parseUnsignedLong((String) value);
}
return convertToLong(value);
}
```

2. **`UUID`** — add a `String` branch using `UUID.fromString`.
3. **`Date` / `Date32` / `DateTime` / `DateTime64`** — add a `String` branch in the `writeDate` / `writeDate32` / `writeDateTime` / `writeDateTime64` helpers, which already dispatch on `LocalDate` / `ZonedDateTime` / `OffsetDateTime` / `Timestamp`. Fixing them there also covers their other callers.

### Backward compatibility

Items 2 and 3 are purely additive: those inputs throw today, so nothing that currently succeeds changes behaviour.

Item 1 has one deliberate behaviour change: `"-1"` into a `UInt64` column currently writes `18446744073709551615` and would now throw `NumberFormatException`. That replaces silent corruption with a loud error and aligns the String path with the existing `BigInteger` overload. Suggest keeping it as its own commit so it can be reviewed separately from the additive branches.

The `Long` / `Number` path stays permissive and passes raw bits through unchanged — callers legitimately supply the bit pattern for values above 2^63.

## Why it matters

Reported from the ClickHouse Flink connector. Its checkpoint state format serialises map keys as strings, so for a `Map(K, V)` column the key reaches `serializeMapData` (`:478`) → `serializePrimitiveData` as a `String`. The connector currently has to reject `Map(UInt64, …)`, `Map(UUID, …)`, `Map(Date, …)`, `Map(Date32, …)` and `Map(DateTime(64), …)` at planning time and tell users to change their table DDL, even though all of these are legal ClickHouse map key types.

More generally, any caller passing a `String` value for one of these types — nested or top-level — hits the same behaviour.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Hướng nghiên cứu

Bắt đầu tại client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java ở serializePrimitiveData và lần theo các helper path cho UInt64, UUID, date và date-time. Xác minh behavior chuyển đổi hiện có và range check unsigned long liên quan trong BinaryStreamUtils. Hoàn tất có nghĩa là các giá trị String được xử lý nhất quán cho các kiểu này, các giá trị UInt64 không hợp lệ bị fail thay vì âm thầm wrapping, và các path signed hoặc numeric hiện có vẫn không thay đổi.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
java
Lĩnh vực
databases
Loại issue
Lỗi
Độ khó
3/5
Thời gian dự kiến
1-2 ngày
Mức độ hoạt động
Sôi nổi
Độ rõ ràng
Đặc tả rõ ràng
Mức phù hợp với người mới
82/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.