Fix lazy materialization OOB and a Merge LOGICAL_ERROR from unused text index virtual columns

Project: ClickHouse/ClickHouse | Language: C++ | Concepts: data-structures, algorithmic-optimization, error-handling, testing-strategy | Difficulty: senior
Link: https://github.com/ClickHouse/ClickHouse/pull/111467

The problem

When using text indexes on Map columns with lazy materialization and direct text-index reads enabled, ClickHouse could produce a query plan where a virtual column for a text index was included in the read step's output header, even though it was not actually used by the rest of the query. This led to two user-visible failures:

• An out-of-bounds read (release) or assertion failure (debug) in the lazy materialization optimization, due to a mismatch between the expected and actual columns.
• A logical error (server abort) when reading from a Merge table whose children had different text index configurations, because the query plan's output headers were inconsistent.

Why it's non-trivial

A naive fix might simply filter unused columns at some later stage or attempt to always include all possible virtual columns. However, the problem is subtle: the query plan's column bookkeeping relies on the output header of the read step exactly matching the columns that are actually consumed by upper layers. If a virtual column is included in the header but not referenced, downstream optimizations (like lazy materialization) can misalign their internal masks and access memory out of bounds. Additionally, the fix must not break cases where multiple indexes are actually used, or where only one survives after query rewrite.

The approach

The core fix is in how TextIndexDAGReplacer::replace decides which virtual columns to add to the query plan after DAG rewrites and dead code elimination. Instead of recording all candidate virtual columns before pruning, the code now only adds those whose input survived the removeUnusedActions pass.

Key code snippets:

1. Collect candidate virtual columns while walking the DAG, but do not immediately add them:

   +        std::vector<std::pair<String, VirtualColumnDescription>> candidate_virtual_columns;
   ...
   -            for (auto & [index_name, virtual_column] : replaced.added_virtual_columns)
   -                result.added_columns[index_name].add(std::move(virtual_column));
   +            for (auto & [index_name, virtual_column] : replaced.added_virtual_columns)
   +                candidate_virtual_columns.emplace_back(index_name, std::move(virtual_column));
   

   Annotation:
   Instead of directly adding every candidate virtual column to result.added_columns, they are first collected into a temporary vector. This defers the decision of which to keep until after unused actions are removed.

2. After pruning unused actions, only keep virtual columns whose input survived:

   +        /// A virtual column is read only if its input survived `removeUnusedActions`: the rewrite can
   +        /// keep a different index's virtual (or the original expression) instead, leaving this one unused.
   +        for (auto & [index_name, virtual_column] : candidate_virtual_columns)
   +        {
   +            if (replaced_columns_set.contains(virtual_column.name))
   +                result.added_columns[index_name].add(std::move(virtual_column));
   +        }
   

   Annotation:
   This loop ensures that only those virtual columns whose input names are present in the surviving set (after dead code elimination) are added to the plan. This makes the output header of the read step match exactly what is needed, preventing downstream mismatches and errors.

3. Tests were added to cover both failure modes:

   • 02346_text_index_bug111422.sql covers the out-of-bounds/lazy materialization path.
   • 04563_text_index_merge_heterogeneous_children.sql covers the Merge-table header mismatch.

   Annotation:
   Both tests assert not just the absence of a crash but also that the correct columns are present or absent in the plan, and that the results are as expected.

From the review thread

No notable review discussion on this PR.

Takeaways

• Keep internal data structures in strict sync with what downstream consumers expect.
  If an output header advertises more columns than are actually referenced, downstream logic that assumes positional or one-to-one correspondence may fail in subtle or catastrophic ways.

• Prune intermediate artifacts after dead code elimination, not before.
  When rewriting or optimizing a computation graph, only record artifacts (like virtual columns) after the final set of live nodes is known.

• Regression tests should pin all relevant settings to guarantee the failing path is exercised.
  If test coverage depends on settings or query planner choices, explicitly set them in the test to avoid non-deterministic results.