Fix CreateColumnByType for SimpleAggregateFunction with non-terminal inner type

[polyglotAI-bot]

Description

Fixes #540.

CreateColumnByType("SimpleAggregateFunction(any, LowCardinality(String))") returned nullptr instead of a valid column, which made reading any block that contains such a column fail with unsupported column type: SimpleAggregateFunction(...).

In clickhouse/columns/factory.cpp, CreateColumnFromAst handled the TypeAst::SimpleAggregateFunction meta by calling CreateTerminalColumn on the inner (value) type. But CreateTerminalColumn is a switch over terminal type codes only — it falls through to return nullptr for non-terminal wrapper metas (LowCardinality, Nullable, Array, Tuple, Map, Enum). Every other wrapper case in CreateColumnFromAst recurses through CreateColumnFromAst; SimpleAggregateFunction was the only one restricted to terminal inner types.

A SimpleAggregateFunction(func, T) column is serialized on the wire identically to a column of type T, so the created column must be the column for the inner type T. The fix recurses through CreateColumnFromAst (which itself dispatches terminal inner types to CreateTerminalColumn, so the existing terminal path is unchanged), passing settings through so low_cardinality_as_wrapped_column is honored for a SimpleAggregateFunction-wrapped LowCardinality just as it is for a bare one.

Changes

• clickhouse/columns/factory.cpp — in the TypeAst::SimpleAggregateFunction case, recurse through CreateColumnFromAst(GetASTChildElement(ast, -1), settings) instead of CreateTerminalColumn(...) (one-line change).
• ut/CreateColumnByType_ut.cpp — new parametrized suite CreateColumnBySimpleAggregateFunctionType asserting CreateColumnByType builds the correct column for terminal (contrast — unchanged) and non-terminal inner types: LowCardinality(String), Nullable(String), Array(UInt64), Map(String, UInt64), Enum8(...), Tuple(UInt64, String).
• ut/client_ut.cpp — new end-to-end ClientCase.SimpleAggregateFunctionLowCardinality that CREATEs a table with a SimpleAggregateFunction(anyLast, LowCardinality(String)) column, inserts rows and reads them back.

Test

• The parametrized unit test fails on master (returns nullptr) for every non-terminal inner type and passes with the fix; the terminal case SimpleAggregateFunction(sum, UInt64) stays green in both, proving the change is targeted.
• The end-to-end integration test throws unsupported column type: SimpleAggregateFunction(anyLast, LowCardinality(String)) on master and passes with the fix (verified on both uncompressed and LZ4 connections), demonstrating the reported user scenario — reading blocks containing the affected column type — is resolved.
• Broad regression across the factory, type-parser, column and roundtrip suites: 825 passed / 0 failed / 1 pre-existing skip.

Pre-PR validation gate

• Deterministic repro confirmed (nullptr / unsupported column type on master)
• Root cause documented above
• Fix targets the root cause
• Test fails without fix, passes with fix (verified both directions)
• No existing tests broken; no existing tests weakened
• Convention compliance verified per README.md / CI workflows (C++17, tests in the canonical ut/ files)
• Fix is on the live runtime read path (proven by the end-to-end test)
• Non-breaking: additive — previously-nullptr inputs now build a valid column; the terminal path is byte-identical

[Copilot]

Pull request overview

This PR fixes CreateColumnByType failing to construct columns for SimpleAggregateFunction(func, T) when the inner type T is itself non-terminal (e.g., LowCardinality(String), Nullable(String), Array(...), etc.), which previously caused block deserialization to fail (issue #540). It does so by making SimpleAggregateFunction creation recurse through the general AST-driven factory path (so wrappers are handled consistently), and adds both unit and end-to-end tests to prevent regressions.

Changes:

• Fix column factory handling of TypeAst::SimpleAggregateFunction by recursing via CreateColumnFromAst(..., settings) rather than terminal-only creation.
• Add parameterized unit tests covering SimpleAggregateFunction with both terminal and non-terminal inner types.
• Add an integration test that creates, inserts into, and reads from a table containing SimpleAggregateFunction(anyLast, LowCardinality(String)).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
clickhouse/columns/factory.cpp	Makes SimpleAggregateFunction column creation recurse into the inner type using the same wrapper-aware path and preserves settings propagation.
ut/CreateColumnByType_ut.cpp	Adds parameterized coverage ensuring CreateColumnByType returns a non-null column whose type matches the inner type for multiple wrapped inner-type cases.
ut/client_ut.cpp	Adds an end-to-end regression test verifying blocks containing SimpleAggregateFunction(..., LowCardinality(String)) can be read successfully.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.