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(