Fix a crash when a primary-key range layer produces an empty pipe

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

The problem

When reading from a MergeTree table using primary-key range layers, the internal logic could produce an empty "pipe" (a data stream abstraction) for a layer. If such an empty pipe was passed to later processing steps (like transforms or merging), it would cause a crash or throw an exception, e.g., "Cannot add simple transform to empty Pipe." This could be triggered by certain query plans (notably with FINAL) and was found by fuzz testing.

Why it's non-trivial

A naive fix might just check for emptiness in one place, but the empty pipe can arise in several different code paths (single-node, distributed, and merging scenarios). Some consumers require positional consistency (e.g., join-by-shards), while others can safely drop empty pipes. The fix must ensure that empty pipes are only dropped when it's safe and that all relevant paths are covered, without breaking assumptions elsewhere.

The approach

The PR introduces targeted guards to skip or filter out empty pipes at key points, and adds a unit test to cover the scenario. Here are the main changes:

1. Skipping empty pipes in distributed FINAL merge lanes

+        if (pipe.empty())
+            continue;

What it does:
In buildDistributedFinalPipe, after constructing a pipe for a lane, the code checks if the pipe is empty. If so, it skips further processing for that lane. This prevents later transform steps from operating on an empty pipe, which would crash.

Why:
Distributed FINAL queries may serialize empty layers as merge lanes. Since merging just unites all lanes, dropping empty ones is safe and avoids exceptions.

2. Avoiding transforms on empty pipes in range filtering

-    if (!filter_function)
+    if (!filter_function || pipe.empty())
         return;

What it does:
In applyRangeFilterFromAST, the function now returns early if the pipe is empty, in addition to the existing check for a null filter function.

Why:
There's nothing to filter in an empty pipe, and trying to apply a transform would fail due to the lack of a header. The expanded comment explains why this is safe for all current consumers.

3. Dropping empty merging pipes before header access in FINAL reads

-                for (auto && merging_pipe : split_ranges_result.merging_pipes)
-                    pipes.push_back(std::move(merging_pipe));
+                for (auto && merging_pipe : split_ranges_result.merging_pipes)
+                {
+                    if (!merging_pipe.empty())
+                        pipes.push_back(std::move(merging_pipe));
+                }

What it does:
In ReadFromMergeTree::spreadMarkRangesAmongStreamsFinal, the code now checks if each merging pipe is empty before adding it to the list for further processing.

Why:
Later steps (like creating projections or merging) assume the pipe has a header. Skipping empty pipes avoids null dereferences or exceptions.

4. Not serializing empty layers as distributed merge buckets

-            for (size_t i = 0; i < split.layers.size(); ++i)
-                buckets.push_back({split.layers[i].getDescriptions(), /*needs_merge=*/ true, split.borders, i});
+            for (size_t i = 0; i < split.layers.size(); ++i)
+                if (!split.layers[i].empty())
+                    buckets.push_back({split.layers[i].getDescriptions(), /*needs_merge=*/ true, split.borders, i});

What it does:
In setupDistributedReadBuckets, when constructing distributed read buckets, the code now skips empty layers.

Why:
This prevents the distributed FINAL path from even serializing empty merge lanes, reducing the chance of hitting the empty-pipe problem downstream.

5. Focused unit test for empty merge lane handling

/// A single empty merge lane: the whole task produces nothing.
{
    std::vector<DistributedReadBucket> lanes;
    lanes.push_back({/*marks=*/ {}, /*needs_merge=*/ true, /*borders=*/ {}, /*index=*/ 0});

    std::optional<ActionsDAG> out_projection;
    Pipe result = buildDistributedFinalPipe(
        lanes, metadata, MergeTreeData::MergingParams{}, /*max_block_size_rows=*/ 8192,
        /*enable_vertical_final=*/ false, context, out_projection, read_lane, read_lane);

    EXPECT_TRUE(result.empty());
    EXPECT_FALSE(out_projection.has_value());
}

What it does:
A new unit test feeds an empty merge lane to buildDistributedFinalPipe and checks that the result is an empty pipe (and doesn't crash).

Why:
This directly covers the previously problematic scenario and prevents regressions.

From the review thread

• AI Review Blocker:
  ...the fix does not look complete.
  ...splitPartsWithRangesByPrimaryKey still wraps an empty layer with a sorting transform before those guards run, so the same empty-layer shape now throws Cannot add simple transform to empty Pipe. before applyRangeFilterFromAST gets a chance to skip it. Skip empty layers before readByLayers, or make create_merging_pipe return early on pipe.empty(), and add a regression test that exercises splitPartsWithRangesByPrimaryKey / single-node FINAL, not only buildDistributedFinalPipe.

• Maintainer response:
  ...the empty-layer guard in applyRangeFilterFromAST let an empty per-layer pipe reach spreadMarkRangesAmongStreamsFinal, which calls createProjection(pipes.front().getHeader()) and addMergingFinal on each merging pipe before any Pipe::unitePipes — a null-header dereference. The FINAL branch now drops empty merging pipes before the first header access, and the comment in applyRangeFilterFromAST no longer claims both consumers rely on unitePipes. Both translation units compile.

Takeaways

• Guard against empty data structures before applying transforms or merges:
  Always check for emptiness before passing a data structure to code that assumes it contains data (e.g., before adding transforms or accessing headers).

• Understand the context and downstream consumers before dropping or skipping data:
  Only skip or drop empty elements when it's safe for the specific consumer—some consumers may rely on positional consistency, while others may not.

• Add regression tests for subtle or non-deterministic internal errors:
  When a bug arises from complex internal state (like empty layers from range splitting), add targeted unit tests to cover the exact scenario and prevent future regressions.