Backport #113291 to 26.6: Fix for virtual row is not being applied in some cases

[robot-clickhouse-ci-2]

Original pull-request #113291

This pull-request is a last step of an automated backporting.
Treat it as a standard pull-request: look at the checks and resolve conflicts.
Merge it only if you intend to backport changes to the target branch, otherwise just close it.

The PR source

The PR is created in the CI job

[clickhouse-gh]

Workflow [BackportPR], commit [fc02f6b]

Summary: ❌

job_name	test_name	status	info	comment
Stress test (amd_tsan)		FAIL
	Hung check failed, possible deadlock found	FAIL	cidb
Integration tests (amd_asan_ubsan, db disk, old analyzer, 5/6)		ERROR

[vdimir]

@groeneai can you check why tests are failing? Are we missing something in this branch, or can some changes from the patch be omitted?

[groeneai]

Checked all four red checks at 12f5e2fe. All seven Build (*) are green, so nothing is missing from src/ beyond your fixup. Three of the four are test-only, and one needs a prerequisite.

1. Unit tests (asan_ubsan) and Unit tests (tsan) - genuinely missing a prerequisite.
TEST(Monotonicity, ToNullable) fails at gtest_monotonic.cpp:100, hasInformationAboutMonotonicity() Actual: false. toNullable got its monotonicity in f5b78f99d917 (#110121, merged 2026-07-19, not backported), so src/Functions/toNullable.cpp on 26.6 has no hasInformationAboutMonotonicity override, while the gtest asserting it did come across. Either backport #110121, or omit that one TEST from this branch. Nothing else in the patch depends on it.

2. Stateless tests (... distributed plan, parallel, 1/2) - 04672, missing setting. It dies at query 12 with Code: 115 ... join_runtime_filter_min_probe_rows (UNKNOWN_SETTING), so no assertion ran. That setting came in #104860 and its settings-history entry is 26.8, so 26.6 does not have it. The pin can simply be omitted: with it removed, 04672 still passes unchanged on master, so it is not load-bearing for what the test asserts.

3. Same job - 04695, a leftover from the fixup. 12f5e2fe removed the t_utc block but left DROP TABLE t_utc; and the three reference lines it produced (12 plus the two toString(x) sort-description lines). Result is Code: 60 ... Table t_utc does not exist at query 18. Deleting that DROP and reference lines 18-20 makes it pass.

4. Both stateless tests - EXPLAIN default format. The references are in the pretty form (Read type: InOrder, box drawing), but that default arrived in 26.7 via explain_query_plan_default; on 26.6 EXPLAIN PLAN still emits the legacy shape (ReadType: InOrder, __table1. prefixes). Pinning actions = 1, compact = 1, pretty = 1 on the EXPLAINs restores the exact shape the references expect on 26.6, and the pinned form still passes on master. All three flags are needed - compact/pretty without actions collapses the plan and hides the sort-description lines.

Integration tests (amd_asan_ubsan, db disk, old analyzer, 5/6) is not related: the check annotation is The self-hosted runner lost communication with the server. BackportPR is just the aggregator.

For the src side, stock 26.6 does reproduce what the patch fixes (SELECT count() FROM t_time WHERE toString(x) >= '-99:00:00' gives 0 where the post-fix reference expects 1), so the patch itself is wanted here as is.

[vdimir]

@groeneai could you please apply the relevant fixes to this branch? I applied these changes to a single branch, and now we need to reapply them to other versions:

Adapting the backport of "Fix for virtual row is not being applied in some cases" (upstream #113291)

The automated cherry-pick applies the hunks whose context matches and silently drops the rest — no conflict markers,
so the loss only shows up as a build or test failure. Work through the steps below.

0. Find the dropped hunks

For every file in the backport commit:

git show <backport-commit> --stat            # file list
git diff master -- <each source file>        # anything from the PR missing here was dropped

master is the source of truth for the files the PR owns. Large diffs in FunctionsConversion.h and
optimizeReadInOrder.cpp are pre-existing divergence — only check that the PR's own hunks are present there.

1. Restore the MergingSortedAlgorithm bookkeeping

Symptoms: use of undeclared identifier 'FieldVisitorDump', use of undeclared identifier 'is_virtual_row',
-Wpointer-bool-conversion on isVirtualRow. The two virtual_row_boundary sizing lines do not break the build
but make the debug checks index an empty vector.

--- a/src/Processors/Merges/Algorithms/MergingSortedAlgorithm.h
+++ b/src/Processors/Merges/Algorithms/MergingSortedAlgorithm.h
@@ -75,6 +75,9 @@ private:
     void insertRow(const SortCursorImpl & current);
     void insertRows(const SortCursorImpl & current, size_t num_rows);
     void insertChunk(size_t source_num);
+
+    /// Per-input pending virtual-row boundary (empty when none), used for debug checks to ensure virtual rows are placed correctly.
+    std::vector<Columns> virtual_row_boundary;
 };

--- a/src/Processors/Merges/Algorithms/MergingSortedAlgorithm.cpp
+++ b/src/Processors/Merges/Algorithms/MergingSortedAlgorithm.cpp
@@ -1,6 +1,8 @@
 #include <Processors/Merges/Algorithms/MergeTreeReadInfo.h>
 #include <Processors/Merges/Algorithms/MergingSortedAlgorithm.h>
 #include <Processors/Transforms/ColumnGathererTransform.h>
+#include <Columns/IColumn.h>
+#include <Common/FieldVisitorDump.h>
 #include <IO/WriteBuffer.h>
 #include <IO/WriteHelpers.h>
 #include <IO/WriteBufferFromString.h>
@@ -14,6 +16,7 @@ namespace DB
 namespace ErrorCodes
 {
     extern const int ILLEGAL_TYPE_OF_COLUMN_FOR_FILTER;
+    extern const int LOGICAL_ERROR;
 }
@@ -130,6 +132,7 @@ void MergingSortedAlgorithm::addInput()
 {
     current_inputs.emplace_back();
     cursors.emplace_back();
+    virtual_row_boundary.emplace_back();
 }
@@ -149,6 +152,7 @@ void MergingSortedAlgorithm::initialize(Inputs inputs)
     removeConstAndSparse(inputs);
     merged_data.initialize(*header, inputs);
     current_inputs = std::move(inputs);
+    virtual_row_boundary.assign(current_inputs.size(), {});
 
     for (size_t source_num = 0; source_num < current_inputs.size(); ++source_num)
     {
@@ -188,7 +192,8 @@ void MergingSortedAlgorithm::initialize(Inputs inputs)
 
 void MergingSortedAlgorithm::consume(Input & input, size_t source_num)
 {
-    if (isVirtualRow(input.chunk))
+    bool is_virtual_row = isVirtualRow(input.chunk);
+    if (is_virtual_row)
     {
         auto pk_block = setVirtualRow(input.chunk, *header, apply_virtual_row_conversions);

Verify with git diff master -- src/Processors/Merges/Algorithms/MergingSortedAlgorithm.cpp — it must come out
empty — rather than by trusting the hunks to apply cleanly.

2. If DateLUTImpl::hasFixedOffset is missing

grep -rn "hasFixedOffset" src/Common/DateLUTImpl.h

If absent, ToStringMonotonicity cannot detect a fixed-offset time zone, so report DateTime as non-monotonic:

--- a/src/Functions/FunctionsConversion.h
+++ b/src/Functions/FunctionsConversion.h
@@ -3985,11 +3985,9 @@ struct ToStringMonotonicity
         /// `DateTime` is formatted in the time zone of the type, and local time decreases when the clocks are
         /// turned back, so the order is preserved only if the time zone never changes its offset.
-        if (const auto * date_time_type = checkAndGetDataType<DataTypeDateTime>(type_ptr))
+        if (checkAndGetDataType<DataTypeDateTime>(type_ptr))
         {
-            if (!date_time_type->getTimeZone().hasFixedOffset())
-                return not_monotonic;
-            return {.is_monotonic = true, .is_always_monotonic = true, .is_strict = true};
+            return not_monotonic;
         }

Then drop the t_utc part of 04695_to_string_monotonicity, which covers exactly the fixed-offset case — the
DROP TABLE and the expected output included:

--- a/tests/queries/0_stateless/04695_to_string_monotonicity.sql
+++ b/tests/queries/0_stateless/04695_to_string_monotonicity.sql
@@ -20,16 +20,6 @@ SELECT toString(toDateTime(1636264799, 'America/New_York')), ...
 SELECT toString(x) FROM t_dst ORDER BY toString(x) SETTINGS optimize_read_in_order = 1;
 
--- A fixed-offset time zone preserves order, and `toString` is strict there, so `y` stays in the prefix.
-DROP TABLE IF EXISTS t_utc;
-CREATE TABLE t_utc (x DateTime('UTC'), y UInt32) ENGINE = MergeTree ORDER BY (x, y);
-INSERT INTO t_utc SELECT toDateTime(1636264798 + intDiv(number, 4), 'UTC'), number % 4 FROM numbers(20);
-
-SELECT count() FROM t_utc WHERE toString(x) >= '2021-11-07 06:00:00';
-SELECT trimLeft(explain) FROM (
-    EXPLAIN PLAN SELECT * FROM t_utc ORDER BY toString(x), y SETTINGS optimize_read_in_order = 1
-) WHERE explain LIKE '%sort description%';
-
 DROP TABLE IF EXISTS t_date;
@@ -31,5 +21,4 @@
 DROP TABLE t_time;
 DROP TABLE t_dst;
-DROP TABLE t_utc;
 DROP TABLE t_date;

--- a/tests/queries/0_stateless/04695_to_string_monotonicity.reference
+++ b/tests/queries/0_stateless/04695_to_string_monotonicity.reference
@@ -15,8 +15,5 @@
 2021-11-07 01:59:59
-12
-│  Prefix sort description: toString(x) ASC, y ASC
-│  Result sort description: toString(x) ASC, y ASC
 │  Prefix sort description: toString(d) ASC, y ASC
 │  Result sort description: toString(d) ASC, y ASC

3. Drop settings the tests use that do not exist yet

grep -c max_bytes_ratio_before_external_join src/Core/Settings.cpp
grep -c join_runtime_filter_min_probe_rows   src/Core/Settings.cpp

A missing setting fails the whole test with UNKNOWN_SETTING (code 115) at the SET. Two tests are affected:

--- a/tests/queries/0_stateless/04672_read_in_order_virtual_row_sort_prefix.sql
+++ b/tests/queries/0_stateless/04672_read_in_order_virtual_row_sort_prefix.sql
@@ -20,3 +20,3 @@
-SET max_bytes_ratio_before_external_join = 0, max_bytes_before_external_join = 0, query_plan_read_in_order_through_join = 1, join_runtime_filter_min_probe_rows = 0;
+SET max_bytes_before_external_join = 0, query_plan_read_in_order_through_join = 1;

--- a/tests/queries/0_stateless/04029_join_convert_to_fixed_hash_table.sql
+++ b/tests/queries/0_stateless/04029_join_convert_to_fixed_hash_table.sql
@@ -19,3 +19,3 @@
-SET max_bytes_before_external_join = 0, max_bytes_ratio_before_external_join = 0; -- Disable automatic spilling for this test
+SET max_bytes_before_external_join = 0; -- Disable automatic spilling for this test

4. If EXPLAIN PLAN does not default to pretty output

grep -c explain_query_plan_default src/Core/Settings.cpp

If the setting is absent, EXPLAIN PLAN defaults actions, compact and pretty to false, while on master
checkAndGetSettings in src/Interpreters/InterpreterExplainQuery.cpp forces all three on. The reference files were
generated in the pretty format, so the same query otherwise yields analyzer-qualified names (__table1.name),
CAST(__table1.name, \'Nullable(String)\'_String) instead of CAST(name AS Nullable(String)), and the label
ReadType: instead of Read type: (ReadFromMergeTree.cpp keys it on format_settings.pretty); a plain
EXPLAIN PLAN prints no sort descriptions at all.

Request the options explicitly instead of regenerating the references, so the references stay byte-identical to
master. All nine EXPLAINs in 04672_read_in_order_virtual_row_sort_prefix.sql:

-FROM (EXPLAIN PLAN actions = 1, indexes = 0 SELECT ...
+FROM (EXPLAIN PLAN actions = 1, indexes = 0, compact = 1, pretty = 1 SELECT ...

and the option-less EXPLAIN in 04695_to_string_monotonicity.sql:

-    EXPLAIN PLAN SELECT * FROM t_date ORDER BY toString(d), y SETTINGS optimize_read_in_order = 1
+    EXPLAIN PLAN actions = 1, compact = 1, pretty = 1 SELECT * FROM t_date ORDER BY toString(d), y SETTINGS optimize_read_in_order = 1

If the pretty/compact options themselves do not exist (grep -n "bool pretty" src/Processors/QueryPlan/QueryPlan.h),
this is unavailable and the references have to be regenerated for the old format — note that in the commit message,
since the references then no longer match master.

5. If Monotonicity.ToNullable fails

./src/unit_tests_dbms --gtest_filter='*Monotonicity*:*ConversionMonotonic*'
gtest_monotonic.cpp:100: Value of: function_base->hasInformationAboutMonotonicity()  Actual: false  Expected: true

The gtest comes with this PR, but the overrides it asserts on were added to master by an unrelated commit,
"Fix expression check in convertOuterJoinToInnerJoin"
(git log -S getMonotonicityForRange master -- src/Functions/toNullable.cpp). The other three cases
(assumeNotNull, ifNull, coalesce) pass. Either delete the ToNullable case, or forward-port the hunk:

--- a/src/Functions/toNullable.cpp
+++ b/src/Functions/toNullable.cpp
@@ -53,6 +53,13 @@ public:
         return makeNullableOrLowCardinalityNullable(arguments[0].column);
     }
 
+    bool hasInformationAboutMonotonicity() const override { return true; }
+
+    Monotonicity getMonotonicityForRange(const IDataType &, const Field &, const Field &) const override
+    {
+        return { .is_monotonic = true, .is_positive = true, .is_always_monotonic = true, .is_strict = true };
+    }
+
 #if USE_EMBEDDED_COMPILER

Take only this hunk; the same commit also drops final from FunctionToNullable and adds a join test, both out of
scope. Forward-porting lets index analysis and read-in-order see through toNullable, which the rest of the
backport does not rely on — 04672 writes its Nullable cases as ::Nullable(String), which resolves to CAST.

Verification

./tests/clickhouse-test -b ./clickhouse --test-runs=1 \
  02149_read_in_order_fixed_prefix 03257_reverse_sorting_key \
  03262_common_expression_optimization 04029_join_convert_to_fixed_hash_table \
  04058_explain_pretty_aggregation_sorting \
  04672_read_in_order_virtual_row_sort_prefix 04695_to_string_monotonicity

./src/unit_tests_dbms --gtest_filter='*Monotonicity*:*ConversionMonotonic*'   # 12/12

ninja unit_tests_dbms clickhouse covers both the gtest and the server. if constexpr (do_debug_checks) bodies are
type-checked even in release builds, so a release compile is enough to catch step 1, but the gtest needs a build with
tests enabled.

If you sweep for fallout from step 5, read_in_order nullable monotonic index_analysis is a useful selector set.
Discount local failures caused by the environment rather than the change: missing stateful datasets (test.hits,
test.visits, code 60), no cluster for the distributed index analysis tests, file and object storage inputs for the
format tests, and replicated engines that need a disk with keeper metadata type (code 36).

[groeneai]

I cannot push to backport/26.6/113291: that branch is in ClickHouse/ClickHouse itself, not a fork, and @groeneai has push: false there (maintainerCanModify is also false on the PR). So here is the exact patch instead. It is 3 test files, no src/ change.

Applying your steps to this branch, two of the four are already done and one needs narrowing:

• Step 1, MergingSortedAlgorithm is already satisfied. Your own check is the proof: git diff master -- src/Processors/Merges/Algorithms/MergingSortedAlgorithm.{h,cpp} is empty on this head (and on 26.5). Nothing was dropped there.
• Step 2, hasFixedOffset is already applied by your 12f5e2fe.
• Step 3 needs narrowing. max_bytes_ratio_before_external_join does exist on 26.6 (DECLARE(Double, ...) at Settings.cpp:8008), so 04029 needs no change and only join_runtime_filter_min_probe_rows comes out of the 04672 SET. That makes it identical to your 26.7 commit 61542f7c.
• Step 4 applies here but did not on 26.7: grep -c explain_query_plan_default src/Core/Settings.cpp is 0 on 26.6 and 26.5, 1 on 26.7. Note the robot already dropped the SET explain_query_plan_default = 'legacy'; line from 04058, so that file needs nothing.

One caveat on step 0: git diff master also reports the MergeTreeReadInfo.cpp "reuse the header column" hunk as missing, but that is not a dropped hunk. It arrived after #113291 via #113264, so it is master moving on and must not be added to the backport.

--- a/tests/queries/0_stateless/04672_read_in_order_virtual_row_sort_prefix.sql
+++ b/tests/queries/0_stateless/04672_read_in_order_virtual_row_sort_prefix.sql
@@ -20,3 +20,3 @@
 SET optimize_read_in_order = 1, read_in_order_use_virtual_row = 1;
-SET max_bytes_ratio_before_external_join = 0, max_bytes_before_external_join = 0, query_plan_read_in_order_through_join = 1, join_runtime_filter_min_probe_rows = 0;
+SET max_bytes_ratio_before_external_join = 0, max_bytes_before_external_join = 0, query_plan_read_in_order_through_join = 1;
 SET query_plan_optimize_join_order_limit = 1, query_plan_optimize_join_order_randomize = 0, query_plan_join_swap_table = 0;

plus actions = 1, indexes = 0 -> actions = 1, indexes = 0, compact = 1, pretty = 1 on all 9 EXPLAINs in that file (lines 28, 32, 36, 40, 44, 48, 52, 58, 64).

--- a/tests/queries/0_stateless/04695_to_string_monotonicity.sql
+++ b/tests/queries/0_stateless/04695_to_string_monotonicity.sql
@@ -25,10 +25,9 @@
 INSERT INTO t_date SELECT toDate('2021-01-01') + intDiv(number, 4), number % 4 FROM numbers(20);
 
 SELECT trimLeft(explain) FROM (
-    EXPLAIN PLAN SELECT * FROM t_date ORDER BY toString(d), y SETTINGS optimize_read_in_order = 1
+    EXPLAIN PLAN actions = 1, compact = 1, pretty = 1 SELECT * FROM t_date ORDER BY toString(d), y SETTINGS optimize_read_in_order = 1
 ) WHERE explain LIKE '%sort description%';
 
 DROP TABLE t_time;
 DROP TABLE t_dst;
-DROP TABLE t_utc;
 DROP TABLE t_date;

--- a/tests/queries/0_stateless/04695_to_string_monotonicity.reference
+++ b/tests/queries/0_stateless/04695_to_string_monotonicity.reference
@@ -15,8 +15,5 @@
 2021-11-07 01:59:57
 2021-11-07 01:59:58
 2021-11-07 01:59:59
-12
-│  Prefix sort description: toString(x) ASC, y ASC
-│  Result sort description: toString(x) ASC, y ASC
 │  Prefix sort description: toString(d) ASC, y ASC
 │  Result sort description: toString(d) ASC, y ASC

The DROP TABLE t_utc and those three reference lines are the leftovers of the t_utc block your fixup removed; today 04695 dies at Code: 60 UNKNOWN_TABLE and 04672 at Code: 115 UNKNOWN_SETTING before any assertion runs, which is why both report as failures rather than as diffs.

Verified with clickhouse local on stock 26.6.3.30 from builds.clickhouse.com and on a master build. With the patch both tests run to completion on 26.6, and every remaining difference is exactly the defect #113291 fixes: Read type: Default instead of InOrder, no Virtual row conversions, prefix sort description cut short, and in 04695 the count() of 0 instead of 1 and the wrong toString order. On the master build the same adapted files match the references byte for byte, so the pinning does not lose coverage. The src/ side is wanted here as is.

That leaves Unit tests, which your procedure does not cover. The only failing gtest on both asan_ubsan and tsan is Monotonicity.ToNullable, which #113291 added. It asserts toNullable's monotonicity, and that lives in src/Functions/toNullable.cpp, added separately by f5b78f99d917 from your #110121: grep -c hasInformationAboutMonotonicity src/Functions/toNullable.cpp is 0 on 26.6 and 26.5, 1 on 26.7 and master. That is why 26.7 was green and these two are not. #110121 is labelled pr-performance and was not backported, so either it gets v26.6-must-backport and v26.5-must-backport (a maintainer would need to apply those, I cannot) or that one TEST is dropped from the backports. Your call.

For 26.5 the same patch applies with three differences, which I have put on #114219.

[vdimir]

@groeneai can you create your branch on top of this in your fork, I'll cherry pick form there then

[groeneai]

Pushed: groeneai:backport-26.6-113291-adapt, 2 commits on top of 12f5e2fe, compare.

• 48b38c07 adapts 04672 and 04695, which is everything needed for the stateless failures.
• d7e0987b deletes Monotonicity.ToNullable, kept separate so you can drop just that commit if you would rather label Convert outer join to inner when a both-sides filter is null-rejecting under join_use_nulls=1 #110121 v26.6-must-backport instead. That decision is still open on your side; the branch takes the delete route so the cherry-pick holds no surprises.

Nothing in src/ besides the gtest deletion, so the fix itself is unchanged.

While re-reviewing the branch I found a wrong-results bug on 26.6 that is not caused by this backport, and it is the closest thing to a real answer to your original "are we missing something in this branch":

CREATE TABLE vr (a UInt32, b Int64, c Int64) ENGINE = MergeTree ORDER BY (a, b, c);
SYSTEM STOP MERGES vr;
INSERT INTO vr VALUES (0,0,0),(0,1,0);
INSERT INTO vr VALUES (0,1,-100);
SELECT c FROM vr WHERE b = 1 ORDER BY a, c;

On stock 26.6.3.30 that returns 0, -100 with read_in_order_use_virtual_row = 1 and -100, 0 with it off. Master returns -100, 0 either way. A key column fixed by WHERE is skipped without emitting a MatchInfo, so the later matches index pk_column_names densely and c is built from b. Master disables virtual rows unconditionally when a key column is skipped; on the release branches that guard only fires when next_sort_key == 0.

The unconditional guard is c87e2edd from #110725, and git merge-base --is-ancestor puts it on master only: absent from 26.5, 26.6 and 26.7, so 26.7 merged its backport with this gap too. I have not touched it here, since it is out of scope for a backport adaptation and the fix already exists upstream. v26.5/26.6/26.7-must-backport on #110725 would cover all three, but I cannot apply labels.

Two smaller things I noticed and deliberately left alone: 04672's ordering assertions use LIMIT 10 on a six-row table with unique (name, code), so no cross-part tie is ever broken, and setVirtualRow returns an owning Block by value for a check that is discarded outside debug builds. Both are the same on master, so they belong there rather than in a backport that is meant to change nothing.