ColumnArray::AppendAsColumn silently accepts a wrong-typed element column, writes zero data bytes and desynchronizes the native block stream

Inizia da clickhouse/columns/array.cpp:49 e dalle implementazioni di Append in numeric.cpp:72-76 e string.cpp:73-79,248-260. Esegui i test mirati in ut/column_array_ut.cpp e verifica il comportamento esistente della validazione degli array. Il lavoro è completato quando un AppendAsColumn del tipo errato viene rifiutato senza modificare offset o dati, mentre le colonne tipizzate correttamente e quelle vuote mantengono il comportamento attuale.

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:

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<T>::Append — clickhouse/columns/numeric.cpp:72-76: if (auto col = column->As<ColumnVector<T>>()) { ... }, 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 <clickhouse/client.h> for the second test):

TEST(ArrayDesync, WrongTypedAppendAsColumn) {
    // Array(String), but we append a UInt64 column as one row's elements
    auto arr = std::make_shared<ColumnArray>(std::make_shared<ColumnString>());
    auto wrong = std::make_shared<ColumnUInt64>();
    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<ColumnUInt32>(); id->Append(1);

    auto val = std::make_shared<ColumnArray>(std::make_shared<ColumnString>());
    auto bad = std::make_shared<ColumnUInt64>(); bad->Append(7); bad->Append(8);
    val->AppendAsColumn(bad);                     // wrong element type, silently dropped

    auto tail = std::make_shared<ColumnString>(); 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:

   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