From 62991a127fab33308235bf4ee9e80bb75551e70a Mon Sep 17 00:00:00 2001 From: Polyglot AI <293096396+polyglotAI-bot@users.noreply.github.com> Date: Wed, 29 Jul 2026 12:40:51 +0000 Subject: [PATCH] Fix CreateColumnByType for SimpleAggregateFunction with non-terminal inner type CreateColumnByType returned nullptr for a SimpleAggregateFunction whose value type is a non-terminal wrapper such as LowCardinality, Nullable, Array, Tuple, Map or Enum (e.g. SimpleAggregateFunction(anyLast, LowCardinality(String))), which made reading any block containing such a column fail with "unsupported column type". The factory handled the SimpleAggregateFunction meta by calling CreateTerminalColumn on the inner type, but that only covers terminal type codes and falls through to nullptr for wrapper metas. Every other wrapper (Array, Nullable, Tuple, Map, LowCardinality) recurses through CreateColumnFromAst; SimpleAggregateFunction was the only one restricted to terminals. Recurse through CreateColumnFromAst instead (passing settings so low_cardinality_as_wrapped_column is honored). A SimpleAggregateFunction is transparent on the wire, so the created column matches its inner type. Fixes: https://github.com/ClickHouse/clickhouse-cpp/issues/540 --- clickhouse/columns/factory.cpp | 2 +- ut/CreateColumnByType_ut.cpp | 29 ++++++++++++++++++++++++++ ut/client_ut.cpp | 38 ++++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 1 deletion(-) diff --git a/clickhouse/columns/factory.cpp b/clickhouse/columns/factory.cpp index a01304f8..c21bf14e 100644 --- a/clickhouse/columns/factory.cpp +++ b/clickhouse/columns/factory.cpp @@ -252,7 +252,7 @@ static ColumnRef CreateColumnFromAst(const TypeAst& ast, CreateColumnByTypeSetti } } case TypeAst::SimpleAggregateFunction: { - return CreateTerminalColumn(GetASTChildElement(ast, -1)); + return CreateColumnFromAst(GetASTChildElement(ast, -1), settings); } case TypeAst::Map: { diff --git a/ut/CreateColumnByType_ut.cpp b/ut/CreateColumnByType_ut.cpp index 279a19cc..0e9dbe69 100644 --- a/ut/CreateColumnByType_ut.cpp +++ b/ut/CreateColumnByType_ut.cpp @@ -8,6 +8,8 @@ #include +#include + namespace { using namespace clickhouse; } @@ -20,6 +22,33 @@ TEST(CreateColumnByType, CreateSimpleAggregateFunction) { ASSERT_NE(nullptr, col->As()); } +// SimpleAggregateFunction is transparent on the wire: the created column must +// match its value (inner) type. The inner type may itself be a wrapper such as +// LowCardinality, Nullable, Array or Map, which previously produced a nullptr +// because only terminal inner types were handled (issue #540). +class CreateColumnBySimpleAggregateFunctionType + : public ::testing::TestWithParam> +{}; + +TEST_P(CreateColumnBySimpleAggregateFunctionType, CreateColumnByType) { + const auto & [type_name, expected_inner_name] = GetParam(); + const auto col = CreateColumnByType(type_name); + ASSERT_NE(nullptr, col) << "CreateColumnByType returned nullptr for " << type_name; + EXPECT_EQ(expected_inner_name, col->GetType().GetName()); +} + +INSTANTIATE_TEST_SUITE_P(InnerType, CreateColumnBySimpleAggregateFunctionType, ::testing::Values( + // Terminal inner type — handled before the fix; must stay unchanged. + std::make_tuple("SimpleAggregateFunction(sum, UInt64)", "UInt64"), + // Non-terminal (wrapper) inner types — returned nullptr before the fix. + std::make_tuple("SimpleAggregateFunction(anyLast, LowCardinality(String))", "LowCardinality(String)"), + std::make_tuple("SimpleAggregateFunction(anyLast, Nullable(String))", "Nullable(String)"), + std::make_tuple("SimpleAggregateFunction(groupArrayArray, Array(UInt64))", "Array(UInt64)"), + std::make_tuple("SimpleAggregateFunction(sumMap, Map(String, UInt64))", "Map(String, UInt64)"), + std::make_tuple("SimpleAggregateFunction(anyLast, Enum8('a' = 1, 'b' = 2))", "Enum8('a' = 1, 'b' = 2)"), + std::make_tuple("SimpleAggregateFunction(anyLast, Tuple(UInt64, String))", "Tuple(UInt64, String)") +)); + TEST(CreateColumnByType, UnmatchedBrackets) { // When type string has unmatched brackets, CreateColumnByType must return nullptr. ASSERT_EQ(nullptr, CreateColumnByType("FixedString(10")); diff --git a/ut/client_ut.cpp b/ut/client_ut.cpp index dff1400e..472087c0 100644 --- a/ut/client_ut.cpp +++ b/ut/client_ut.cpp @@ -728,6 +728,44 @@ TEST_P(ClientCase, SimpleAggregateFunction) { EXPECT_EQ(EXPECTED_ROWS, total_rows); } +TEST_P(ClientCase, SimpleAggregateFunctionLowCardinality) { + const auto & server_info = client_->GetServerInfo(); + if (versionNumber(server_info) < versionNumber(19, 9)) { + GTEST_SKIP() << "Test is skipped since server '" << server_info << "' does not support SimpleAggregateFunction" << std::endl; + } + + // A SimpleAggregateFunction column whose value type is a non-terminal + // wrapper (here LowCardinality(String)) must be readable: the column + // factory previously returned nullptr for such a type, so reading any + // block that contained it failed (#540). + client_->Execute("DROP TEMPORARY TABLE IF EXISTS test_clickhouse_cpp_saf_lc"); + client_->Execute( + "CREATE TEMPORARY TABLE IF NOT EXISTS test_clickhouse_cpp_saf_lc " + "(saf SimpleAggregateFunction(anyLast, LowCardinality(String)))"); + + const std::vector data{"foo", "bar", "foo", "baz"}; + client_->Execute( + "INSERT INTO test_clickhouse_cpp_saf_lc (saf) VALUES ('foo'),('bar'),('foo'),('baz')"); + + size_t total_rows = 0; + client_->Select("SELECT saf FROM test_clickhouse_cpp_saf_lc", [&total_rows, &data](const Block & block) { + if (block.GetRowCount() == 0) + return; + + total_rows += block.GetRowCount(); + ASSERT_EQ(1U, block.GetColumnCount()); + + auto col = block[0]->As>(); + ASSERT_NE(nullptr, col); + ASSERT_EQ(data.size(), col->Size()); + for (size_t r = 0; r < col->Size(); ++r) { + EXPECT_EQ(data[r], (*col)[r]) << " at index: " << r; + } + }); + + EXPECT_EQ(data.size(), total_rows); +} + TEST_P(ClientCase, Cancellable) { /// Create a table. client_->Execute(