ClickHouse / ClickHouse/clickhouse-cpp
ColumnArray::AppendAsColumn silently accepts a wrong-typed element column, writes zero data bytes and desynchronizes the native block stream
- Dominant language
- C
- Stars
- 382
- Forks
- 209
- Avg merge
- 3h 58m
- Merged PRs (30d)
- 12
Description
## 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
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.