ClickHouse / ClickHouse/clickhouse-java

client-v2: geo columns (Point/Ring/Polygon/...) are misread in the Native format

Đang mở
#3,088 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
area:data-type bug client-api-v2
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ả

## Description

`client-v2` cannot read geo columns (`Point`, `Ring`, `LineString`, `MultiLineString`, `Polygon`, `MultiPolygon`) from the **Native** format. The same queries read correctly with `RowBinaryWithNamesAndTypes`.

Two symptoms, by type:

* **`Point` with more than one row in a block — silent data corruption.** The coordinates are returned scrambled across rows. No exception, and a fixed-width column after the geo column still reads correctly, so nothing signals the corruption.
* **`Ring` / `LineString` / `MultiLineString` / `Polygon` / `MultiPolygon` — the block desynchronizes** and the read fails with `IllegalArgumentException: Non-empty typeName is required`, which does not indicate the cause.

`Point` with exactly one row per block reads correctly by coincidence (with one row, the columnar and the row-wise layouts are byte-identical).

### Steps to reproduce

1. Run a query that selects a geo column with `QuerySettings.setFormat(ClickHouseFormat.Native)` and read it with `client.newBinaryFormatReader(response)`.
2. For `Point`, use a query that returns 3 rows in one block; the returned coordinates do not match the server.
3. For `Ring` (or any other multi-point geo type), the read throws.

### Error Log or Exception StackTrace

```
java.lang.IllegalArgumentException: Non-empty typeName is required
at com.clickhouse.data.ClickHouseColumn.of(ClickHouseColumn.java:...)
at com.clickhouse.client.api.data_formats.NativeFormatReader.readBlock(NativeFormatReader.java:87)
```

The exception is a consequence of the desync: after the geo column is read with the wrong layout, the stream position is inside the payload, so the *next* column header is parsed as an empty name/type.

### Expected Behaviour

The Native format must return the same values as `RowBinaryWithNamesAndTypes`, which agrees with the server.

Server (ClickHouse 26.7.3.19), `FORMAT JSONCompactEachRow`:

```
[0, [1,2], 42]
[1, [3,4], 42]
[2, [5,6], 42]
```

`RowBinaryWithNamesAndTypes` (correct):

```
row 0: rowId=0 g=[1.0, 2.0] tail=42
row 1: rowId=1 g=[3.0, 4.0] tail=42
row 2: rowId=2 g=[5.0, 6.0] tail=42
```

`Native` (actual — coordinates scrambled, no error):

```
row 0: rowId=0 g=[1.0, 3.0] tail=42
row 1: rowId=1 g=[5.0, 2.0] tail=42
row 2: rowId=2 g=[4.0, 6.0] tail=42
```

`Ring`, same shape, `Native` (actual):

```
EXCEPTION: java.lang.IllegalArgumentException: Non-empty typeName is required
```

while `RowBinaryWithNamesAndTypes` returns the correct `[[1.0,2.0],[3.0,4.0]]`, `[[11.0,12.0],[13.0,14.0]]`, `[[21.0,22.0],[23.0,24.0]]`.

### Root cause

`NativeFormatReader.readBlock()` (`client-v2/src/main/java/com/clickhouse/client/api/data_formats/NativeFormatReader.java:109`) enters its columnar branch only when `column.isArray()` is true. The concrete geo types are their own `ClickHouseDataType` values, so `isArray()` is false for all of them and they fall through to the `else` branch at line 118, which calls `binaryStreamReader.readValue(column)` once per row. That dispatches to the **RowBinary** geo decoders — `readGeoPoint()` (`BinaryStreamReader.java:1123`), `readGeoRing()` (`:1132`), `readGeoPolygon()` (`:1147`), `readGeoMultiPolygon()` (`:1161`).

The two encodings are not interchangeable:

* `Point` is `Tuple(Float64, Float64)`. Native writes it column-major — all x values, then all y values. `readGeoPoint()` reads two *adjacent* doubles as one point. The byte count per block is the same either way, so the column boundary is preserved and the error stays silent; only the pairing is wrong.
* `Ring` / `LineString` are `Array(Point)`. Native writes cumulative UInt64 offsets, then the element tuple column-major. `readGeoRing()` reads a per-row varuint count followed by interleaved `(x, y)` pairs, so the first offset byte is consumed as a point count and everything after that is misaligned. `Polygon` / `MultiPolygon` add further offset levels with the same result.

Native bytes for the 3-row `Point` query above, showing the column-major layout the reader does not expect:

```
0303 05 726f774964 06 55496e743634 3 cols, 3 rows, 'rowId' UInt64
0000000000000000 0100000000000000 0200000000000000 0, 1, 2
01 67 05 506f696e74 'g' Point
000000000000f03f 0000000000000840 0000000000001440 x column: 1, 3, 5
0000000000000040 0000000000001040 0000000000001840 y column: 2, 4, 6
04 7461696c 05 496e743332 2a000000 2a000000 2a000000 'tail' Int32: 42, 42, 42
```

### Suggested fix

Two options; the second is smaller but is itself a behavior change:

1. **Decode geo columns column-major in the Native reader.** Route them into the existing columnar path by treating each as its array-of-tuple equivalent (`Point` → columnar `Tuple(Float64, Float64)`, `Ring`/`LineString`/`MultiPoint` → `Array(Point)`, `Polygon`/`MultiLineString` → `Array(Ring)`, `MultiPolygon` → `Array(Polygon)`), then assemble the `double[]` / `double[][]` / `double[][][]` / `double[][][][]` values the RowBinary decoders return today, so the value shape returned to callers does not change.
2. **Reject geo columns in the Native format** with a clear `ClientException` pointing at `RowBinaryWithNamesAndTypes`, matching the precedent already in `readBlock` for the QBit shapes it does not decode (`NativeFormatReader.java:102`). This turns silent corruption into a loud, actionable failure, but it removes a read path that currently appears to work for single-row `Point`.

Whichever is chosen, a regression test should place the geo column **in the middle of the schema with a fixed-width column after it** and read **several rows in one block** — a single-row `Point` passes even with the current code.

Two contrast cases must keep their current behavior: reading these types with `RowBinaryWithNamesAndTypes` is correct today, and the geo **write** path is unaffected.

### Related

* #2955 / #2956 fix per-row lengths from cumulative offsets *inside* the `isArray()` branch. Geo columns never reach that branch, so that fix does not cover them.
* Surfaced by a review comment on #3050 (MultiPoint support): https://github.com/ClickHouse/clickhouse-java/pull/3050#discussion_r3894055614. This defect is pre-existing and independent of that PR, whose only `BinaryStreamReader` change is one extra `case MultiPoint:` in the RowBinary switch. `MultiPoint` shares the `Array(Point)` layout, so once merged it behaves like `Ring` here.

### Code Example

```java
Client client = new Client.Builder()
.addEndpoint("http://localhost:8123")
.setUsername("default").setPassword("")
.compressServerResponse(false)
.build();

String sql = "SELECT number AS rowId,"
+ " (toFloat64(number*2+1), toFloat64(number*2+2))::Point AS g,"
+ " toInt32(42) AS tail FROM numbers(3) ORDER BY rowId";

QuerySettings settings = new QuerySettings().setFormat(ClickHouseFormat.Native);
try (QueryResponse response = client.query(sql, settings).get()) {
ClickHouseBinaryFormatReader reader = client.newBinaryFormatReader(response);
while (reader.next() != null) {
System.out.println(Arrays.toString((double[]) reader.readValue("g")));
}
}
// prints [1.0, 3.0] / [5.0, 2.0] / [4.0, 6.0]
// expected [1.0, 2.0] / [3.0, 4.0] / [5.0, 6.0]

// Replacing Point with ::Ring and the same 3-row shape throws instead:
// java.lang.IllegalArgumentException: Non-empty typeName is required
```

Affected geo types, verified one by one against the server with both formats:

| Type | RowBinaryWithNamesAndTypes | Native |
| --- | --- | --- |
| `Point`, 1 row/block | correct | correct (layouts coincide) |
| `Point`, 3 rows/block | correct | **wrong values, no error** |
| `Ring` | correct | throws |
| `LineString` | correct | throws |
| `MultiLineString` | correct | throws |
| `Polygon` | correct | throws |
| `MultiPolygon` | correct | throws |

### Configuration

#### Environment
* [ ] Cloud
* Client version: `0.11.0-rc1` (`main` at 0b781da1f)
* Language version: OpenJDK 17.0.18
* OS: Ubuntu 24.04 (container)

#### ClickHouse Server
* ClickHouse Server version: 26.7.3.19
* ClickHouse Server non-default settings, if any: none
* `CREATE TABLE` statements for tables involved: none — reproduces on a `SELECT` from `numbers()`
* Sample data for all these tables: n/a

---

Found by automated analysis of the client while working on #3050, and verified against a live ClickHouse server (26.7.3.19) rather than by code inspection: every row above was produced by running both formats through `client.newBinaryFormatReader(...)` and comparing with the server's own `JSONCompactEachRow` output.

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

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

Hướng nghiên cứu

Bắt đầu trong client-v2/src/main/java/com/clickhouse/client/api/data_formats/NativeFormatReader.java, đặc biệt là readBlock(), và so sánh cách xử lý theo cột với các bộ giải mã geo của BinaryStreamReader. Chạy bản tái hiện Native được cung cấp của numbers(3) và so sánh với RowBinaryWithNamesAndTypes, bao gồm một cột có độ rộng cố định sau cột geo. Được xem là hoàn tất khi các giá trị geo khớp với server mà không bị mất đồng bộ, hoặc Native từ chối các kiểu geo không được hỗ trợ bằng một lỗi rõ ràng.

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
api
Loại issue
Lỗi
Độ khó
4/5
Thời gian dự kiến
3-5 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
68/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.