ClickHouse / ClickHouse/ClickHouse

Over-strict debug guard in `MergingSortedAlgorithm::checkVirtualRowCoversSortDescription` aborts debug builds when `optimize_distinct_in_order` widens the sort description past a constant-prefix-folded virtual row

Open
#116,155 5 comments 0 reactions 0 assignees View on GitHub
comp-query-execution fuzz
Dominant language
C++
Stars
49.9k
Forks
9k
Avg merge
21h 32m
Merged PRs (30d)
515

Description

# Over-strict debug guard in `MergingSortedAlgorithm::checkVirtualRowCoversSortDescription` aborts debug builds when `optimize_distinct_in_order` widens the sort description past a constant-prefix-folded virtual row

### Describe what's wrong

`checkVirtualRowCoversSortDescription` at `src/Processors/Merges/Algorithms/MergingSortedAlgorithm.cpp:71` throws

> `Logical error: 'Virtual row does not cover sort column 'b'. Virtual row columns: a, sort description [a, b]'`

when `optimize_read_in_order` folds a constant prefix out of the effective sort key (so the virtual row covers only `a`) while `optimize_distinct_in_order` widens the merge's sort description to the full DISTINCT key (`[a, b]`). The check is gated by `do_debug_checks` (`#ifndef NDEBUG`) so it fires in debug builds and under `ABORT_ON_LOGICAL_ERROR` (i.e. the AST fuzzer / CI); it is compiled out in release.

### Does it reproduce on the most recent release?

Reproduces on current master. Introduced by `e5c44e5d35a` ("use virtual row on prefix sort description, add debug checks", 2026-08-04). Under a release binary the assert is compiled out and the query returns results identical to `read_in_order_use_virtual_row = 0` — i.e. the merge itself is safe; the guard is over-strict.

### How to reproduce

Minimal repro, verified aborts on a fresh debug binary (RC 134 / core-dumped):

```sql
CREATE TABLE t (a UInt32, b UInt32) ENGINE = MergeTree ORDER BY (a, b) SETTINGS index_granularity = 8;
SYSTEM STOP MERGES t;
INSERT INTO t SELECT number % 10, 1 FROM numbers(2000);
INSERT INTO t SELECT number % 10, 1 FROM numbers(2000, 2000);
INSERT INTO t SELECT number % 10, 1 FROM numbers(4000, 2000);
INSERT INTO t SELECT number % 10, 1 FROM numbers(6000, 2000);

SELECT DISTINCT a, b
FROM t
WHERE b = 1
ORDER BY 'x' ASC, a DESC
SETTINGS
optimize_read_in_order = 1,
read_in_order_use_virtual_row = 1,
optimize_distinct_in_order = 1,
read_in_order_two_level_merge_threshold = 3,
max_threads = 2
FORMAT Null;
```

Load-bearing bits:

- **Constant sort-key prefix (`ORDER BY 'x' ASC, a DESC`)** — the constant `'x'` is folded out, so the effective sort description for read-in-order is just `a`. The virtual row therefore carries only `a`.
- **`optimize_distinct_in_order = 1`** — widens the merge's sort description to the full DISTINCT key `[a, b]` so that the DISTINCT can stream in order.
- **Multi-part merge** — the four insert-blocks + `SYSTEM STOP MERGES` and `read_in_order_two_level_merge_threshold = 3` push into the merge path where the virtual row is compared against the widened sort description.

Removing the constant prefix (`ORDER BY a DESC`, the shape already covered by the existing regression test `04549`) does NOT trip it. Adding `b` explicitly (`ORDER BY 'x', a DESC, b DESC`) also does NOT trip it. Only the prefix-fold + distinct-widening combination does.

### Expected behavior

Debug and release should behave the same, whichever way the invariant is settled:

- If the merge is actually safe when the sort description widens past the virtual row with contiguous trailing key columns — which appears to be the case here, since `b` is a contiguous key suffix and release results match the unoptimized read — then the debug guard should be relaxed to allow that.
- Otherwise the pipeline should not have been built in this shape in the first place: either the virtual row should be widened to include `b`, or the virtual row optimization should be disabled when `optimize_distinct_in_order` widens the sort description beyond what the virtual row carries.

### Error message and/or stacktrace

```
Logical error: 'Virtual row does not cover sort column 'b'. Virtual row columns: a, sort description [a, b]'
```

Thrown from `checkVirtualRowCoversSortDescription` at `src/Processors/Merges/Algorithms/MergingSortedAlgorithm.cpp:71`. Debug/CI abort only; release compiles the assert out and returns correct results (identical to `read_in_order_use_virtual_row = 0`) in the tested cases, including with a varying `b` (`number % 3` instead of the constant `1`).

### Additional context

- Bisect: introduced by `e5c44e5d35a` ("use virtual row on prefix sort description, add debug checks").
- Found by the [AST fuzzer + server-side correctness oracles](https://github.com/ClickHouse/ClickHouse/pull/99980) fleet, snapshot `inst-03-20260824-011404` on merged31 (`bc95467dc73`) debug.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.