ClickHouse / ClickHouse/clickhouse-cpp
ColumnArray::AppendAsColumn silently accepts a wrong-typed element column, writes zero data bytes and desynchronizes the native block stream
- Vorherrschende Sprache
- C
- Sterne
- 382
- Forks
- 209
- Ø Merge
- 2 T. 19 Std.
- Gemergte PRs (30 T.)
- 14
Beschreibung
## Description
`ColumnArray::AppendAsColumn()` (`clickhouse/columns/array.cpp:49`) relies on `data_->Append()` throwing when the supplied column type does not match the array's element type:
```cpp
void ColumnArray::AppendAsColumn(ColumnRef array) {
// appending data may throw (i.e. due to ype check failure), so do it first to avoid partly modified state.
data_->Append(array);
AddOffset(array->Size());
}
```
But most `Column::Append(ColumnRef)` implementations **silently no-op** on a type mismatch instead of throwing:
* `ColumnVector::Append` — `clickhouse/columns/numeric.cpp:72-76`: `if (auto col = column->As>()) { ... }`, no `else`.
* `ColumnString::Append` — `clickhouse/columns/string.cpp:248-260`: same shape.
* `ColumnFixedString::Append` — `clickhouse/columns/string.cpp:73-79`: same shape, and additionally silent when `string_size_` differs.
So passing a wrong-typed column to `AppendAsColumn` appends **nothing** to `data_` while `AddOffset(array->Size())` still advances the offsets by `array->Size()`. The `ColumnArray` is left internally inconsistent — exactly the state its own constructor rejects with `ValidationError("Mismatch between data and offsets: ...")` — and `SaveBody()` then writes offsets promising N elements followed by zero element bytes.
On the wire this **desynchronizes the native-protocol block**: the server reads the following column's bytes as this array's element data. The user gets no client-side error, only a confusing (and misleading) server-side exception, and the connection is left unusable for the next operation.
This is the C++ analogue of ClickHouse/clickhouse-java#3041, where `SerializerUtils.serializeArrayData` silently writes zero bytes for a non-`null`, non-array/non-`List` value.
Note the same failure class was reported for `ColumnNullable` in PR #376 (closed, unmerged): `nulls_` grows even when the nested `Append()` silently fails. `ColumnArray` has the identical problem with `offsets_`.
## ClickHouse server version
`26.7.2.59` (native protocol, port 9000), verified against a live server.
Repo at commit `737145d`.
## Reproduction
Added to `ut/column_array_ut.cpp` (needs `#include ` for the second test):
```cpp
TEST(ArrayDesync, WrongTypedAppendAsColumn) {
// Array(String), but we append a UInt64 column as one row's elements
auto arr = std::make_shared(std::make_shared());
auto wrong = std::make_shared();
wrong->Append(1); wrong->Append(2); wrong->Append(3);
EXPECT_NO_THROW(arr->AppendAsColumn(wrong)); // currently passes -- the defect
std::cerr << "arr->Size()=" << arr->Size()
<< " claimed elems row0=" << arr->GetSize(0)
<< " actual data size=" << arr->GetAsColumn(0)->Size() << std::endl;
Buffer buf;
BufferOutput out(&buf);
arr->SaveBody(&out);
out.Flush();
std::cerr << "SaveBody bytes = " << buf.size() << std::endl;
}
TEST(ArrayDesync, EndToEndInsert) {
clickhouse::Client client(clickhouse::ClientOptions().SetHost("localhost").SetPort(9000));
client.Execute("DROP TABLE IF EXISTS test_arr_desync");
client.Execute("CREATE TABLE test_arr_desync (id UInt32, val Array(String), tail String) ENGINE = Memory");
clickhouse::Block b;
auto id = std::make_shared(); id->Append(1);
auto val = std::make_shared(std::make_shared());
auto bad = std::make_shared(); bad->Append(7); bad->Append(8);
val->AppendAsColumn(bad); // wrong element type, silently dropped
auto tail = std::make_shared(); tail->Append("TAILVALUE");
b.AppendColumn("id", id);
b.AppendColumn("val", val);
b.AppendColumn("tail", tail);
try { client.Insert("test_arr_desync", b); std::cerr << "INSERT SUCCEEDED" << std::endl; }
catch (const std::exception& e) { std::cerr << "INSERT threw: " << e.what() << std::endl; }
try {
client.Select("SELECT id, val, tail FROM test_arr_desync", [](const clickhouse::Block&) {});
} catch (const std::exception& e) { std::cerr << "SELECT threw: " << e.what() << std::endl; }
}
```
### Actual output
```
[ RUN ] ArrayDesync.WrongTypedAppendAsColumn
arr->Size()=1 claimed elems row0=3 actual data size=0
SaveBody bytes = 8
[ OK ] ArrayDesync.WrongTypedAppendAsColumn
[ RUN ] ArrayDesync.EndToEndInsert
INSERT threw: DB::Exception: Unknown data type family: TAILVALUE
SELECT threw: cannot execute query while executing another operation
[ OK ] ArrayDesync.EndToEndInsert
```
Reading that: `SaveBody` emitted only the 8-byte offset (`3`) and **zero** element bytes. End-to-end, the server consumed the `tail` column's string payload as the *type name* of the next column — hence `Unknown data type family: TAILVALUE`. The connection was then left mid-operation, so the follow-up `SELECT` also failed.
### Expected
`AppendAsColumn` (or the underlying `Append`) should reject a column whose type does not match the array's element type with a clear client-side `ValidationError` / `std::runtime_error` naming both types, leaving the `ColumnArray` unmodified. Appending a correctly-typed column must keep working, and `AppendAsColumn` of an empty correctly-typed column must still add a zero-length row.
## Suggested fix
Two possible layers, not mutually exclusive:
1. **Narrow** — in `ColumnArray::AppendAsColumn` (`clickhouse/columns/array.cpp:49`), validate before mutating:
```cpp
void ColumnArray::AppendAsColumn(ColumnRef array) {
if (!data_->Type()->IsEqual(array->Type()))
throw ValidationError("Cannot append column of type " + array->Type()->GetName()
+ " to Array of " + data_->Type()->GetName());
data_->Append(array);
AddOffset(array->Size());
}
```
This covers the array path only, but it is where the offsets/data desync is introduced.
2. **General** — make the `Append(ColumnRef)` implementations throw on mismatch instead of silently returning (`numeric.cpp:72`, `string.cpp:73`, `string.cpp:248`, and any sibling with the same `if (auto col = column->As<...>())`-with-no-`else` shape). That would also fix the `ColumnNullable` variant from PR #376 and make the existing comment in `AppendAsColumn` true. It is a behavior change for callers currently relying on the silent no-op, so it may warrant its own discussion.
Regression tests: a wrong-typed `AppendAsColumn` throws and leaves `Size()`/`GetOffset()` unchanged, plus contrast cases that a correctly-typed column and an empty correctly-typed column still behave as today.
## Link
Relayed from https://github.com/ClickHouse/clickhouse-java/issues/3041
Beitragsleitfaden
Für dieses Repository ist kein Beitragsleitfaden indexiert
Rechercherichtung
Beginne mit clickhouse/columns/array.cpp:49 sowie den Append-Implementierungen in numeric.cpp:72-76 und string.cpp:73-79,248-260. Führe die fokussierten Tests in ut/column_array_ut.cpp aus und untersuche das bestehende Verhalten der Array-Validierung. Die Aufgabe ist abgeschlossen, wenn ein falsch typisiertes AppendAsColumn abgelehnt wird, ohne Offsets oder Daten zu ändern, während korrekt typisierte und leere Spalten ihr aktuelles Verhalten beibehalten.
Vom Indexierungsmodell aus dem Issue-Text verfasst.
Bewertung
- Tech-Stack
- cpp
- Bereich
- backend-api-design, testing
- Issue-Typ
- Bug
- Schwierigkeit
- 3/5
- Geschätzter Aufwand
- 1-2 Tage
- Aktivitätsstatus
- Ruhig
- Klarheit
- Klar beschrieben
- Anfängerfreundlichkeit
- 75/100