Merge pull request #542 from ClickHouse/polyglot/fix-simpleaggregatefunction-nonterminal-inner-type

Fix CreateColumnByType for SimpleAggregateFunction with non-terminal inner type

Author: slabko

clickhouse/columns/factory.cpp

@@ -264,7 +264,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: {

ut/CreateColumnByType_ut.cpp

@@ -9,6 +9,8 @@
 
 #include <gtest/gtest.h>
 
+#include <tuple>
+
 namespace {
 using namespace clickhouse;
 }
@@ -21,6 +23,33 @@ TEST(CreateColumnByType, CreateSimpleAggregateFunction) {
     ASSERT_NE(nullptr, col->As<ColumnInt32>());
 }
 
+// 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<std::tuple<const char* /*type*/, const char* /*expected inner type name*/>>
+{};
+
+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"));

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<std::string> 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<ColumnLowCardinalityT<ColumnString>>();
+        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(