Pre-PR Validation Gate

a) Deterministic repro: Unit test GetSmallestColumn.AllSubcolumns — creates a NamesAndTypesList with 2 subcolumn entries (arr.size0, map.keys) and calls getSmallestColumn(). Before fix: abort() with LOGICAL_ERROR. After fix: returns first column.

b) Root cause: getSmallestColumn() iterates columns, skipping entries where isSubcolumn() is true. When ALL entries are subcolumns, min_size stays nullopt, triggering throw LOGICAL_ERROR("No available columns") → abort(). Called from PlannerJoinTree.cpp:471 during prepareBuildQueryPlanForTableExpression() for subquery/union table expressions when columns_names is empty (outer query doesn't reference subquery columns).

c) Fix matches root cause: The fix adds a fallback in getSmallestColumn (return first column for all-subcolumns) and a guard at the call site (non-fatal error for empty projection). Both directly address the crash mechanism.

d) Test intent preserved: No existing tests modified. Original test 04039_count_nullable_tuple_subcolumn passes 50/50 with full randomization. New unit test gtest_get_smallest_column.cpp covers: Normal, MixedWithSubcolumns, AllSubcolumns (the fix), and EmptyList (EXPECT_DEATH).

e) Both directions: AllSubcolumns test FAILS without fix (LOGICAL_ERROR abort), PASSES with fix (returns arr.size0).

Session: cron:clickhouse-ci-task-worker:20260328-131500

cc @vdimir @novikd — could you review this? It fixes a server crash (STID 3938-33a6, 11 hits in 36h) where getSmallestColumn() aborts with LOGICAL_ERROR when called on a projection list that has only subcolumn entries — triggered by AST fuzzer mutations during stress tests.

Workflow [PR], commit [8739dcc]

Summary: ✅

AI Review

Summary

This PR updates PlannerJoinTree and ExpressionActions::getSmallestColumn to handle subquery projection columns safely, adds an explicit guard for empty subquery projections, and adds focused regression coverage (planner gtest + stateless SQL test). I re-checked current code, existing review threads, and CI report state; there are no remaining contract violations or unresolved high-impact risks in the current patch.

Final Verdict

• Status: ✅ Approve

@pufit Thank you for the review. I've reworked the fix based on your feedback (force-pushed clean branch from latest master).

Changes from v1 → v2:

1. Removed the fallback from getSmallestColumn() — you were right that the "return first column when all are subcolumns" approach masked the real issue. Through investigation I confirmed that the Analyzer never creates NameAndTypePair objects with subcolumn metadata (isSubcolumn() == true) for projection columns — it always uses the 2-arg constructor. So the "all subcolumns" condition was a red herring.

2. Fixed the actual call site in PlannerJoinTree.cpp:469-472 — the real issue is an empty projection reaching getSmallestColumn(). RemoveUnusedProjectionColumnsPass ensures at least 1 column for normal queries, but it runs before passes like CountDistinctPass that create new subqueries. Rare AST fuzzer mutations during stress tests can create subquery structures where the projection is empty. The guard converts the LOGICAL_ERROR (abort) into a non-fatal UNSUPPORTED_METHOD error.

3. Removed the unit test — replaced with a stateless regression test (04066_smallest_column_subquery_projection.sql) that exercises the getSmallestColumn() code path through CountDistinctPass rewrites on tuple subcolumns, as you suggested.

4. Why no exact fuzzer reproducer: The crash requires specific AST fuzzer mutations that occur ~1 in millions of runs during stress tests (8 parallel processes running all ~10K tests with ast_fuzzer_runs=5). I ran 35K+ local fuzzed queries across multiple sessions without reproducing the exact mutation. The stateless test covers the code path exercised by the crash but cannot reproduce the exact empty-projection edge case deterministically.

Session: cron:clickhouse-ci-task-worker:20260328-191500

cc @pufit — reworked per your feedback. The fix is now at the call site in PlannerJoinTree.cpp (empty projection guard), with the getSmallestColumn() fallback removed.

v3 — Revised fix per @pufit's review

Problem in v2: The regression test 04066_smallest_column_subquery_projection triggered getSmallestColumn() → LOGICAL_ERROR in the Fast test. The empty-projection guard wasn't sufficient — the projection has entries but getSmallestColumn() skips them all because isSubcolumn() returns true.

Root cause (refined): getSmallestColumn() was designed for storage column lists — it skips meta-subcolumns like .size0, .keys, etc. to avoid reading internal columns. But the call site in PlannerJoinTree.cpp line 471 passes subquery projection columns, which are query-level outputs and don't have storage meta-columns to skip. When a projection column carries subcolumn metadata (e.g. tup.a from CountDistinctPass), getSmallestColumn() incorrectly skips it.

v3 fix: Replace getSmallestColumn() with projection_columns.front() for the subquery/union case. This is semantically correct — subquery projections don't have meta-columns to filter, so any projection column is valid. The empty-projection guard remains as a separate defensive check.

Changes v2 → v3:

• Removed NamesAndTypesList conversion + getSmallestColumn() call
• Replaced with projection_columns.front() (direct first projection column)
• Added explaining comment

Verification:

• 50/50 passes with full randomization
• All 9 count_distinct tests pass
• Original trigger test 04039 passes

Session: cron:clickhouse-ci-task-worker:20260328-221500

Pre-PR Validation Gate (v3)

a) Deterministic repro: Fast test CI deterministically reproduces the crash on every run — test 04066_smallest_column_subquery_projection calls SELECT countDistinct(tup.a) SETTINGS count_distinct_optimization = 1, which triggers getSmallestColumn() on a subquery projection where isSubcolumn() returns true. CIDB evidence: https://play.clickhouse.com/?user=play (PR #101005 test_name='04066_smallest_column_subquery_projection'). Original STID 3938-33a6 has 11+ hits across 9 PRs in stress tests.

b) Root cause: getSmallestColumn() iterates columns, skipping those where isSubcolumn()==true. This filter is correct for storage column lists (skipping .size0, .keys meta-subcolumns) but incorrect for subquery projection columns, which are query-level outputs. When CountDistinctPass rewrites countDistinct(tup.a) into SELECT count() FROM (SELECT tup.a GROUP BY tup.a), the outer count() needs no subquery columns, so PlannerJoinTree picks a dummy column via getSmallestColumn(). The projection column tup.a carries subcolumn metadata → gets skipped → "No available columns" LOGICAL_ERROR → server crash.

c) Fix matches root cause: Replace getSmallestColumn() with projection_columns.front() for the subquery/union code path. This directly addresses the incorrect subcolumn filtering — subquery projections don't have storage meta-columns that need skipping.

d) Test intent preserved: No existing tests modified. New regression test 04066_smallest_column_subquery_projection.sql exercises the CountDistinctPass → getSmallestColumn path with tuple subcolumns and array subcolumns. 50/50 passes with randomization.

e) Both directions: Without fix: Fast test deterministically fails with LOGICAL_ERROR (CIDB confirmed). With fix: 50/50 passes locally with randomization + all 9 count_distinct tests pass + original trigger test 04039 passes.

Session: cron:clickhouse-ci-task-worker:20260328-221500

v4 (3923cdf9207): Added -- Tags: no-replicated-database to test 04066_smallest_column_subquery_projection.

v3 had a DatabaseReplicated check failure — same pattern as tests 04059 and 04062 where DatabaseReplicated mode's query rewriting breaks the test. This is a tag-only change; the C++ fix and test logic are identical to v3.

@pufit — would appreciate a re-review when you get a chance. v3 addressed all your v1 feedback (stateless regression test instead of unit test, root cause fix at call site instead of band-aid fallback). v4 just adds the tag to skip DatabaseReplicated mode.

Please check bot's comments, they look legit.

v5 — Addresses performance regression concern

@pufit — thanks for the pointer to the bot comments. They were spot-on.

What changed v4 → v5:

The v4 fix used projection_columns.front() which always picks the first column — potentially an expensive String/Array when a cheaper UInt8 exists later in the projection. v5 addresses this:

1. Added skip_subcolumns parameter to getSmallestColumn() (default true for backward compat). The subquery call site now passes skip_subcolumns=false, keeping the "pick cheapest column by estimated size" optimization while correctly including all projection entries regardless of subcolumn metadata.

2. Empty-projection guard preserved as a defensive check — throws UNSUPPORTED_METHOD instead of letting getSmallestColumn() hit LOGICAL_ERROR. This branch isn't reachable via standard SQL (RemoveUnusedProjectionColumnsPass ensures ≥1 projection column), but guards against future edge cases from query plan modifications.

3. Test comment updated to reflect the current code path (mentions skip_subcolumns=false instead of the removed projection_columns.front()).

Bot feedback addressed:

• ✅ Performance regression: getSmallestColumn() now picks cheapest column for subquery projections too
• ✅ Empty-projection guard: kept with nice error (not testable via SQL — see comment above)
• ✅ Test comment: updated to match implementation

Verification: 50/50 passes, original trigger test 04039 passes, all count_distinct tests pass.

Session: cron:clickhouse-ci-task-worker:20260329-161500

The Stress test (arm_msan) failure is fixed by #101239, which should be merged first. After it is merged, please update the branch to include the fix.

Added a focused regression test in src/Planner/tests/gtest_planner_empty_projection.cpp (commit 0bba34f) that reaches the if (projection_columns.empty()) guard at PlannerJoinTree.cpp and proves the crash-to-query-exception contract.

PlannerJoinTreeEmptyProjectionGuard.SubqueryWithEmptyProjectionThrowsUnsupportedMethod — constructs a fully analyzed query tree for SELECT 1 FROM (SELECT x FROM t), then simulates the AST-fuzzer-induced corruption that motivated this PR by calling clearProjectionColumns on the inner subquery. The tampered tree is passed directly to InterpreterSelectQueryAnalyzer(query_tree, ...) so no analyzer passes are re-applied. The test asserts that planning throws UNSUPPORTED_METHOD, not LOGICAL_ERROR.

Validation in both directions (Debug build, ENABLE_TESTS=ON):

• With the guard in place: 2/2 tests pass (10 ms total).
• With the guard temporarily commented out: the gtest process aborts with SIGABRT (exit code 134). Captured stack trace matches STID 3938-33a6 exactly — ExpressionActions.cpp:940 → PlannerJoinTree.cpp:486 → Planner::buildPlanForQueryNode → Planner::buildQueryPlanIfNeeded → InterpreterSelectQueryAnalyzer::getQueryPlan.

The companion test ExpressionActionsGetSmallestColumn.AllSubcolumnsWithoutSkipReturnsColumn covers the second change in this PR: with the new skip_subcolumns=false flag, a list of nothing-but-subcolumn entries (modelling tup.a produced by FunctionToSubcolumnsPass) is returned instead of being silently skipped.

The gtest is discovered automatically by the gtest_*.cpp GLOB_RECURSE in src/CMakeLists.txt, no CMake changes were needed.

cc @pufit @alexey-milovidov

Fixes the gtest isolation regression introduced by the previous commit 0bba34f (the one that added gtest_planner_empty_projection.cpp to address the @clickhouse-gh bot's review request).

Symptom

After 0bba34f, 16 existing TransformQueryForExternalDatabase tests started failing in 4 unit-tests builds (asan_ubsan, tsan, msan, amd_llvm_coverage) with:

C++ exception with description "Database test already exists" thrown in the test body.

Root cause

Both src/Planner/tests/gtest_planner_empty_projection.cpp and the older src/Storages/tests/gtest_transform_query_for_external_database.cpp register a DatabaseMemory named "test" in the process-wide DatabaseCatalog from their respective State singletons. Alphabetically Planner/ runs before Storages/, so the new gtest grabs the name first and the older fixture's State::instance() then throws "Database test already exists" from its constructor on every test in that class.

Fix

Rename the database in the new gtest to planner_empty_projection_test_db. The two fixtures now coexist in the same unit_tests_dbms process; the older fixture is untouched.

Verified locally by running the full unit_tests_dbms binary with --gtest_filter='PlannerJoinTreeEmptyProjectionGuard*:ExpressionActionsGetSmallestColumn*:TransformQueryForExternalDatabase*' — all tests pass in the same process.

LLVM Coverage Report

Changed lines: 100.00% (64/64) · Uncovered code

Full report · Diff report

Hi @groeneai @alexey-milovidov @pufit — the changelog category for this PR might need a look.

Current: CI Fix or Improvement (changelog entry is not required)
Suggested: Bug Fix (user-visible misbehavior in an official stable release)

Why: Reproduced the bug with a production-valid query: SELECT countDistinct(tup.a) FROM t SETTINGS count_distinct_optimization=1 throws LOGICAL_ERROR. This is user-visible misbehavior, not just a CI/fuzzer issue. The setting count_distinct_optimization is documented and users do enable it for performance.

Could you verify this is correct? Ignore if the current category is intentional.