Fix wrong results when a sorting key expression's type depends on a session setting by groeneai · Pull Request #119385 · ClickHouse/ClickHouse

groeneai · 2026-09-11T02:55:32Z

Related: #109196
Related: #119168

Changelog category (leave one):

Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fixed wrong results when a table's sorting key expression resolves to a different result type in the query than in the table, for example a key of CAST(json.b, 'String') read with cast_keep_nullable = 1. Read-in-order treated the two as interchangeable, so rows could come back out of order, optimize_aggregation_in_order could split groups and report wrong aggregates, and read_in_order_use_virtual_row = 1 could fail with LOGICAL_ERROR: Virtual row has different type.

Description

A key expression is resolved twice: by the table, under its own settings, which each part's physical row order follows, and by the query, under the session's. matchTrees mapped the two on structure alone, function name plus arity, though its contract claims mapped nodes are equal calculations, so a setting that changes the result type makes read-in-order advertise an order the parts lack.

ORDER BY CAST(json.b, 'String') is the ordinary shape, since a JSON path cannot be a key column; at cast_keep_nullable = 1 the query types it Nullable(String), the key String. Found on master by CI, no issue filed: Stress test (amd_tsan).

Reproducer

SET cast_keep_nullable = 0;
CREATE TABLE t (json JSON) ENGINE = MergeTree ORDER BY CAST(json.b, 'String');
SYSTEM STOP MERGES t;
INSERT INTO t VALUES ('{"b":"a"}'), ('{"b":"c"}');
INSERT INTO t VALUES ('{"b":"b"}'), ('{"a":1}');
SET cast_keep_nullable = 1;
SELECT CAST(json.b, 'String') FROM t ORDER BY CAST(json.b, 'String')
SETTINGS optimize_read_in_order = 1, read_in_order_use_virtual_row = 0, max_threads = 4;

Returns a, c, \N, b before this change and a, b, c, \N after.

The fix requires the mapped pair's result types to agree, extending the type identity already applied to constant children; a deterministic function of equal-typed arguments has one result type, so a disagreeing pair was never one calculation. ReadInOrderOptimizer, the pre-analyzer matcher reached with query_plan_read_in_order = 0, matched the key by name alone and now compares types too.

matchTrees has nine callers, so this tightens all of them, including projections and sharding keys; intended, since each relies on the same claim and a lost match only costs an optimization. The test pins that read-in-order still applies when the types agree; 940 tests, plus 267 replayed under the old analyzer, are identical on both binaries. I can contain it to the two read-in-order builders behind a parameter, if you prefer.

Two cases stay open. equals ignores the DateTime time zone, so a key whose values rather than type follow the session is uncovered (toDateTime(s) under a different session_timezone); #117161's haveSameExpressionIdentity compares the zone and closes it on rebase. The name-only matcher cannot see a divergence nested under an equal-typed parent, since it never walks the expression.

Workflow [PR]
Sync PR [sync-upstream/pr/119385]

A key expression is resolved twice: by the table, under its own settings, which is what each part's physical row order follows, and by the query, under the session's. Both matchers that decide read-in-order compared the two derivations without comparing the types they resolve to. matchTrees mapped them on structure alone, that is function name plus arity, comparing result types only for constant children, while its contract states that directly mapped nodes represent equal calculations. When a setting changes the expression's result type the two derivations are different functions with different NULL handling, so they are different orders. With cast_keep_nullable = 1 a key of CAST(json.b, 'String') is Nullable(String) in the query and String in the table, and read-in-order then advertises an order the parts do not have: rows come back unsorted, aggregation-in-order splits groups and reports wrong counts, and read_in_order_use_virtual_row = 1 fails with "Virtual row has different type", which is the only place the two types are compared and so the only place that reports anything at all. Requiring the mapped pair's result types to agree cannot lose a sound match, because a deterministic function of equal-typed arguments has one result type, so a disagreeing pair was never an equal calculation. It extends the type identity already applied to constant children at the same site. This tightens the matcher for all nine of its callers, which is intended: each uses the same claim to substitute one expression for another. The check is applied at the FUNCTION mapping only. No reachable divergence was found for the INPUT mapping: a Merge table unifying a UInt32 and a Nullable(UInt32) column under one header, the most plausible shape, maps consistently, so an INPUT check would have shipped without a test. ReadInOrderOptimizer is the second matcher, selected under the old analyzer when query_plan_read_in_order is off, and it compared the key by name alone. It now also requires the storage key's type to equal the query side's, at the exact-match branch and at the monotonic branch's argument. That covers a divergence at the top of the key expression. It does not cover one nested under an equal-typed parent, such as ifNull(CAST(json.b, 'String'), 'zzz'), because this matcher tests a single name and never walks the expression; walking is what the plan-based path does, and that path is covered by the matchTrees change above. Each test arm pins which matcher it exercises instead of inheriting the session's, so neither runner injection nor a later default flip can move an arm to the other matcher without the arm saying so. Found on master by CI: Stress test (amd_tsan), sha ad8622e. Related: ClickHouse#109196 Related: ClickHouse#119168 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

groeneai · 2026-09-11T02:55:35Z

Internal second-model review (click to expand)

An independent review pass plus a second-model gate ran over this change across two rounds before it was pushed. Verdicts and the evidence behind them:

❌ Blocker, agreed and fixed in this PR: a second matcher decided read-in-order by name alone.
ReadInOrderOptimizer::matchSortDescriptionAndKey does not go through matchTrees and compared only the column name, and it is the matcher selected under the old analyzer when query_plan_read_in_order = 0 (ExpressionAnalyzer.cpp:2307). Measured base against fix: the matchTrees change does not reach that path at all, so the same divergence still returned a, c, \N, b there. It now compares the storage key's type at both the exact-match branch and the monotonic branch's argument, arms 6 and 7 were added for it, and reverting just that guard fails arm 6 only. 267 tests replayed under enable_analyzer=0 query_plan_read_in_order=0 are identical on both binaries apart from this file's own arms. One residual, stated rather than claimed: a single-name matcher cannot see a divergence nested under an equal-typed parent such as ifNull(CAST(json.b, 'String'), 'zzz'), because it never walks the expression. Every arm now pins which matcher it exercises, so neither runner injection nor a later default flip can move an arm silently.

❌ Blocker, agreed, contract narrowed instead of code weakened: equals cannot see a DateTime time zone.
IDataType::equals treats two DateTimes with different time zones as equal by design, so toDateTime(s) resolved under two session_timezone values still maps. A run on the fixed binary confirms it: with a full sort the epochs come back monotonic, with read-in-order they do not while the plan still reports Read type: InOrder. The prescribed remedy does not work here, because with no explicit zone argument DataTypeDateTime::doGetName() returns the bare string DateTime on both sides, so no comparison of types can see the difference. Rather than hand-roll a second zone-aware comparator while #117161 is introducing one, the changelog entry now claims only what the predicate delivers, a key that resolves to a different result type, and the description names the remaining case and the comparator that closes it.

❌ Blocker, disagreed, with evidence: an ALTER can retype the sorting key, and that is a different bug with a fix already open.
The gate showed that ALTER TABLE ... ADD COLUMN issued under SET cast_keep_nullable = 1 retypes the in-memory sorting key, after which both new comparisons agree and read-in-order returns \N, a, b, c where a full sort returns a, b, c, \N. The mechanism is real and I reproduced the reasoning at the source: StorageMergeTree::alter passes the query context into AlterCommands::apply (StorageMergeTree.cpp:544), which recalculates the sorting key unconditionally, even for ADD COLUMN (AlterCommands.cpp:1791), and sortingKeyChanged only gates a SimpleAggregateFunction check.

What I disagree with is that it belongs to this change. Both comparisons here hold the query resolution against the table metadata resolution, which is what the changelog claims, and in that scenario the two agree: the divergence is between the metadata and the parts, created upstream by the ALTER. It is also not a regression of this diff, since before it read-in-order was accepted unconditionally in the same scenario and produced the identical wrong rows.

The remedy the gate asks for, preserving the storage-resolved key identity across unrelated alters, is the whole subject of #109196, which adds createKeyExpressionContext pinning exactly the type-affecting settings (it names cast_keep_nullable) and a canonicalize_key_types flag documented as preserved across recalculateWithNew*, so a later ALTER or replication sync re-resolves the key under the policy it was first built with. That PR is the declaration and write side; this one is the query-time side of the same problem. Doing it here would duplicate it in a PR about a different subsystem, and retyped key metadata reaches well past read-in-order: the same primary_key.data_types deserializes primary.idx and types pk_header. Compensating for it inside a matcher would be a guard on the symptom. Worth recording that the window is bounded: CREATE and ATTACH resolve the key with the global context (InterpreterCreateQuery.cpp:2602-2605), so a restart puts the type back, which the gate's own logs show.

💡 Noted, not blocking: the legacy guard is stricter than the matchTrees one for a bare column.
matchTrees still maps INPUT nodes by name with no type comparison, deliberately, so ORDER BY a keeps read-in-order even where the header type differs from a table's key type. The new exact-match branch compares types for any sort column, and getInputOrder is called with another table's metadata in four places (StorageMerge, StorageBuffer, StorageMaterializedView, and the window-function path). So a Merge table unifying a UInt32 with a Nullable(UInt32) now declines, and since every child must agree, the whole Merge read loses read-in-order. It costs an optimization and never a result, and it needs the old analyzer together with query_plan_read_in_order = 0. I am pointing it out rather than tightening or loosening anything, since the direction is the safe one.

💡 Noted, not blocking: the INPUT mapping still has no type comparison.
It was planned and then dropped rather than shipped untested, because two measurements found no reachable witness: removing the INPUT check while keeping the FUNCTION one left every arm passing, and a Merge table unifying UInt32 and Nullable(UInt32) under one header maps consistently. In practice a matched non-constant child is compared by node identity, so an INPUT type difference that reaches the parent's result type is caught anyway.

⚠️ Corrected before pushing.
Three things in the description. The two real Related: links sat inside the template's HTML comment and rendered nowhere, so the template block was restored and the links moved below it. A claim that ORDER BY CAST(json.b, 'String') is "the documented way" to key a JSON path was not in the JSON documentation, so it now cites what the server itself suggests when it rejects a Dynamic key. And the soundness sentence claimed no sound match is lost; its premise is equal-typed arguments, but inputs map by name, so a pair can disagree in result type because its arguments did. It now claims only what is proven, that a disagreeing pair was never one calculation.

Session id: cron:clickhouse-review-slot-9:20260911-021300

clickhouse-gh · 2026-09-11T02:56:39Z

Workflow [PR], commit [18e3966]

Summary: ✅

Performance Comparison: Performance dashboard

AI Review

Summary

This PR tightens read-in-order matching when a sorting-key expression resolves to a different result type between table metadata and the query session. The matchTrees change fixes the plan-based path, but the legacy ReadInOrderOptimizer path still accepts nested setting-dependent expressions by checking only the top-level output type, so the wrong-results bug remains reachable on the current head.

Findings

❌ Blockers

[src/Storages/ReadInOrderOptimizer.cpp:137-145] [dismissed by author -- https://github.com/Fix wrong results when a sorting key expression's type depends on a session setting #119385#discussion_r3985645896] The legacy exact-name branch still validates only the outer node. With enable_analyzer = 0 and query_plan_read_in_order = 0, ORDER BY ifNull(CAST(json.b, 'String'), 'zzz') still matches because both outer nodes are String, even though the inner CAST remains String in the table and Nullable(String) in the query. That leaves the same wrong-results class live on the old-analyzer path. Suggested fix: compare the full ORDER BY DAG against the sorting-key DAG here (for example by reusing matchTrees), or conservatively refuse non-trivial exact-name expression matches on this path until whole-expression identity can be proven.

Tests

⚠️ Add a stateless regression arm for enable_analyzer = 0, query_plan_read_in_order = 0, and ORDER BY ifNull(CAST(json.b, 'String'), 'zzz'). That is the smallest proof that the legacy matcher rejects the nested divergence once fixed, not just the top-level CAST case already covered by arms 6 and 7.

Final Verdict

Status: ❌ Block

Minimum required action: close the remaining old-analyzer nested-expression gap in ReadInOrderOptimizer and add the matching regression coverage.

LLVM Coverage Report

Measured on commit 18e3966.

Metric	Baseline	Current	Δ
Lines	89.00%	88.90%	-0.10%
Functions	91.80%	91.80%	+0.00%
Branches	81.30%	81.30%	+0.00%

Changed lines: Changed C/C++ lines covered: 17/29 (58.62%) · Uncovered code

Full report · Diff report

groeneai · 2026-09-11T03:01:21Z

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes, 100%, no randomization and no timing: a `MergeTree` table keyed on `CAST(json.b, 'String')` created at `cast_keep_nullable = 0`, two parts with `SYSTEM STOP MERGES`, one row whose `json.b` is absent, then read at `cast_keep_nullable = 1`. One non-default setting.
b	Root cause explained?	A key expression is resolved twice, by the table under its own settings (which each part's physical row order follows) and by the query under the session's. `matchTrees` mapped the two on structure alone, comparing result types only for constant children, so with `cast_keep_nullable = 1` a `Nullable(String)` query node mapped to a `String` key node. Every read-in-order decision then rests on a claim of equal calculation that does not hold, and the order advertised is not the order on disk.
c	Fix matches root cause?	Yes: the unsound mapping is refused at the producer, so no consumer can build on it. The `setVirtualRow` type assert is deliberately left untouched as the detector rather than relaxed, and nothing is `Nullable`-wrapped or cast to make the symptom go away.
d	Test intent preserved / new tests added?	New test `05187_read_in_order_key_type_setting_divergence` with seven arms. Arms 1 to 5 pin `query_plan_read_in_order = 1` and cover the plan-based matcher: the reported `ORDER BY` divergence, the divergence hidden under an equal-typed `ifNull` parent, aggregation-in-order returning wrong counts, the reported virtual-row failure, and a control that read-in-order is still applied when the types agree. Arms 6 and 7 pin it off under the old analyzer and cover the legacy matcher the same way, one wrong-order arm and one control. Every arm states its matcher at query level, so neither runner injection nor a default flip can silently move it. The existing constant-child type check is untouched.
e	Both directions demonstrated?	Yes, on Build-ID-verified binaries (`744451f6` to `197c4f87`). Before: the reported abort with byte-identical message and frames, and wrong order `a, c, \N, b`. After: `a, b, c, \N` on all three arms. The abort also reproduces at server level, where it takes the unfixed server down.
f	Fix is general across code paths?	Both matchers that decide read-in-order are covered. The `matchTrees` change is the single producer that all nine of its callers share; `ReadInOrderOptimizer::matchSortDescriptionAndKey`, which does not use `matchTrees` and matched by name alone, gets the same `IDataType::equals` predicate at both its exact-match and monotonic branches (arms 6 and 7, and 267 old-analyzer test runs identical on both binaries). Each carrier was observed rather than inferred: the monotonic fallback does not re-admit the rejected pair, a `materialize` wrapper reddens and is fixed, fixed-key propagation drops from `Read type: InOrder` to `Default` while keeping `InOrder` when types agree, and aggregation-in-order switches from `AggregatingInOrderTransform` to `AggregatingTransform`.
g	Fix generalizes across inputs (params/datatypes/wrappers)?	The predicate is type-based, so it is specific to neither JSON, `CAST`, nor `cast_keep_nullable`. Only the `cast_keep_nullable` carrier is shipped as a test: the `Nullable(String)`-column analogue cannot be built because `CAST(NULL, 'String')` is rejected for an ordinary key column, and the `geoDistance` `Float32`/`Float64` fixed-key shape has no wrong-results oracle, since rows with equal `Float64` distance necessarily share one `Float32` bucket.
h	Backward compatible?	Yes. No setting, default or format change, so no `SettingsChangesHistory.cpp` entry. The only behaviour change is that an unsound optimization is declined, and arm 5 pins that sound ones are still applied.
i	Invariants and contracts preserved?	The change enforces the documented contract of `matchTrees`, that directly mapped nodes represent equal calculations, for pairs whose result types differ, and the header comment now states exactly that condition. Two cases stay open, both disclosed in the PR description: `IDataType::equals` ignores the `DateTime` time zone by design, so a `session_timezone`-dependent key still matches; and the legacy matcher, which compares one name rather than walking the expression, cannot see a divergence nested under an equal-typed parent. Refusing a match is safe for every consumer, since each already handles a null match as "no mapping". The monotonic fallback needs only the child mapping, which the predicate validates.

Session id: cron:clickhouse-impl-slot-5:20260910-234800

groeneai · 2026-09-11T03:02:26Z

cc @vdimir @CurtizJ, could you review this? A sorting key expression is resolved twice, by the table under its own settings and by the query under the session's, and both read-in-order matchers treated the two as one calculation without comparing the types they resolve to, so with cast_keep_nullable = 1 a Nullable(String) query node mapped to a String key and read-in-order advertised an order the parts do not have.

clickhouse-gh · 2026-09-11T03:21:07Z

+        /// give the two resolutions different result types; equal names then denote different
+        /// calculations, with different NULL handling, and so different orders.
+        const auto * node = elements_actions.getActionsDAG().tryFindInOutputs(sort_column.column_name);
+        if (node && !node->result_type->equals(*sorting_key_type))


Checking only the outer node's type here still leaves the legacy matcher unsound for nested divergences. With query_plan_read_in_order = 0, ORDER BY ifNull(CAST(json.b, 'String'), 'zzz') hits this exact-name branch, node->result_type and sorting_key_type are both String, and the optimization is accepted even though the inner CAST is Nullable(String) in the query and String in the table. The plan-based path rejects that shape, but the old ReadInOrderOptimizer path still reads parts in the wrong order for it.

This branch needs to compare the whole key expression, not just the top-level output type. Reusing matchTrees here would close the gap; a conservative fallback is to reject exact-name matches when the output is a non-trivial expression.

Confirmed, and reproduced on this head: at enable_analyzer = 0, query_plan_read_in_order = 0, cast_keep_nullable = 1, ORDER BY ifNull(CAST(json.b, 'String'), 'zzz') returns a, c, zzz, b where a full sort gives a, b, c, zzz. The check is at the top of the expression only, so the inner CAST is invisible to it.

Two scope points from the same run. query_plan_read_in_order = 0 alone does not reach it, since this matcher is built only under the old analyzer (ExpressionAnalyzer.cpp:2307); with the analyzer on, that query returns a, b, c, zzz. And the plan-based path does reject the shape after this change: that is arm 2 of the test here, pinned at query_plan_read_in_order = 1, with a b c zzz in the reference.

Reusing matchTrees is reachable. Each ORDER BY element already gets its own DAG over the source columns (ExpressionAnalyzer.cpp:1776-1785), so the whole tree is at this site, and matchTrees is already called from three files under src/Storages/. The cost I cannot bound without measuring is over-rejection: it maps ambiguous nodes arbitrarily and does not support aliases (actionsDAGUtils.h:69-71), so such a veto would also decline pairs that fail to map for reasons unrelated to this bug, and a lost match here is a silent performance regression.

The conservative fallback I would decline: rejecting exact-name matches whenever the output is a non-trivial expression declines every expression sorting key on this path, including those whose types agree. Arm 7 pins one of them as still reading Read type: InOrder.

So this PR keeps the top-level check, which narrows a hole older than it, and states the nested case in the description. If closing it here is preferred to a follow-up, I will extend it to the tree walk and bring old-analyzer suite numbers for the over-rejection question.

clickhouse-gh · 2026-09-11T05:25:06Z

Build profile diff (arm_release)

Comparing 18e396614 with master 9125cd444 (stripped binary size, per-symbol sizes and ThinLTO time; compile times per translation unit against the most recent warmup build that recompiled it).

✅ No significant changes.

Binary sizes

programs/clickhouse-stripped: smaller than the master baseline by the known offset between the two builds, so the difference is not shown. A delta that differs from the offset by more than 50% of it is shown, in either direction.

The official master build is compiled with -g and a pull request build is not, and XRay counts debug instructions towards its instrumentation threshold, so master instruments thousands of functions more and its binary is ~0.4% larger no matter what the pull request does.

Compile time of recompiled translation units

8 translation units recompiled, 16 s compile time in total, 8 of them have a recent master baseline.

Job report

groeneai · 2026-09-11T09:45:27Z

CI finish ledger - `739fdff`

Every failure below has an owner: a fixing PR (mine or external), or a full-effort fix task
whose fixing-PR link will be posted here when it opens. Only CH Inc sync is exempt.

Check / test	Reason	Owner / fixing PR
Stress test (amd_msan) / `Logical error: 'Not-ready Set is passed as the second argument for function 'in''` (STID 0250-41a5)	pre-existing trunk defect, reached here through the `WITH FILL ... INTERPOLATE` expression: `FillingTransform::interpolate` runs `ExpressionActions` containing an `IN` whose subquery set was never built (`FillingTransform.cpp:457` to `FunctionIn::executeImpl` at `in.cpp:155`)	#102308 (external, open)
Mergeable Check, PR	praktika aggregators rolling up the row above, not separate failures	#102308 (external, open)

Not caused by this pull request. This branch changes how a sorting key expression whose type depends
on a session setting is planned, and touches neither set building nor FillingTransform; the
signature has true-master hits and a wide carrier spread on trees that do not carry it.

Session id: cron:our-pr-ci-monitor:20260911-093124

vdimir · 2026-09-11T10:52:58Z

-                            if (parent->type == ActionsDAG::ActionType::FUNCTION && func_name == parent->function_base->getName())
+                            /// One function name resolves to different result types depending on the settings
+                            /// the DAG was built with, and differently-typed results are not one calculation.
+                            if (parent->type == ActionsDAG::ActionType::FUNCTION && func_name == parent->function_base->getName()


When can we have different result type for same arguments?

CAST with cast_keep_nullable. The setting is not part of the arguments: it is read from the context when the overload resolver is built (CastOverloadResolver.cpp:192-194, and again into FunctionConvertSettings at FunctionsConversion.h:151), so one function name over the same argument types resolves to a different return type in each DAG. The target type is the second argument and is a constant, and the constant branch below this one already compares those by result type and field value (:194, :232), so that is not what differs here.

Measured on this branch:

SELECT toTypeName(CAST(json.b, 'String')) FROM t; -- cast_keep_nullable = 0 -> String -- cast_keep_nullable = 1 -> Nullable(String) SELECT toTypeName(CAST(materialize(toNullable(1)), 'String')); -- 0 -> String, 1 -> Nullable(String)

In the read-in-order callers (optimizeReadInOrder.cpp:523 and :806) the left tree is sorting_key.expression's DAG, resolved when the table's KeyDescription was built, and the right one is the query DAG built under the session's settings. So ORDER BY CAST(json.b, 'String') with cast_keep_nullable = 1 in the session is String on the left and Nullable(String) on the right. A JSON path needs no Nullable argument for this: the argument is Dynamic, which the setting treats as nullable (FunctionsConversion.h:3167-3170).

The difference is in the values too, not only the wrapper: where b is absent the String form yields '' and the Nullable(String) form yields NULL, which sorts last, so the parts are not in the order the query assumes. Without the type check that query returns a c \N b where a b c \N is correct (test arm 1), and at read_in_order_use_virtual_row = 1 it hits the setVirtualRow type assert quoted in the description. Arm 5 pins that the mapping is still made, and read-in-order still chosen, when the types agree.

PedroTadim · 2026-09-11T16:28:42Z

Got a small reproducible: https://fiddle.clickhouse.com/225727bb-5bfb-40ea-8892-a01e8e816cf9 @Avogar, the logical error this PR is trying to fix is happening in stress tests since my update yesterday. Maybe you can review?

PedroTadim · 2026-09-11T16:30:29Z

@groeneai merge with master

…rees-result-type-agreement

groeneai · 2026-09-11T17:37:32Z

Merged with master: 18e3966142fae48da79f2ff6952e4029330b1d5d, no conflicts.

Your fiddle runs both ways on that head (debug build, clickhouse local):

master c108e273b6087: the same Virtual row has different type ... Expected ... Nullable(String) ..., got ... String String(size = 1) logical error, SIGABRT.
this branch: 8 rows in key order, {"a":0,"b":"str_0"}, {"a":0,"b":"str_0"}, {"a":0,"b":"str_1"}, {"a":0,"b":"str_2"}, {"a":1,"b":"str_0"}, {"a":1,"b":"str_1"}, {"a":1,"b":"str_1"}, {"a":1,"b":"str_2"}.

Arm 4 of the test here is the same shape (read_in_order_use_virtual_row = 1, key resolved at cast_keep_nullable = 0 and queried at 1), so I am not growing the test file for it.

On the frequency: in CIDB the message is 38 rows over 36 distinct pull requests plus one master run across 8 checks, all since 2026-09-10 16:22:34Z, with one isolated hit before that on 2026-07-17.

Avogar

LGTM, let's merge it asap as it failes in lots of PRs. I think it started to happen in upgrade check because in 26.8 we enabled read_in_order_use_virtual_row by default.

@vdimir I see you tool a quick look already, if you have any objections - feel free to add.

…ade load Since ClickHouse#116783 the stress randomization enables `cast_keep_nullable = 1` in a third of the runs, and, unlike its sibling arms, without the `not upgrade_check` guard. The upgrade check runs that load against the previous release's server (26.8.2.7), which predates ClickHouse#119385: it matches the sorting key `CAST(json.b, 'String')` to the same expression in `ORDER BY` by name and arity only, although under `cast_keep_nullable = 1` the query types it `Nullable(String)` while the key is `String`. Read-in-order with `read_in_order_use_virtual_row = 1` then aborts the shipped 26.8 server in `setVirtualRow` with `Logical error: Virtual row has different type` while running `03277_json_subcolumns_in_primary_key`, so `Upgrade check (amd_release)` went red on dozens of unrelated pull requests (57 on 2026-09-11, 23 on 2026-09-12, 7 on 2026-09-13). A master fix cannot clear it, because the exception is raised by the old binary. Guard the arm like the sibling `serialize_query_plan` arm (5620afa). CI: https://s3.amazonaws.com/clickhouse-test-reports/praktika.html?PR=99495&sha=c84da07b7cc18cc399ef06b7fc656f99a72b713a&name_0=PR&name_1=Upgrade%20check%20(amd_release) PR: ClickHouse#99495 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…ade load Since #116783 the stress randomization enables `cast_keep_nullable = 1` in a third of the runs, and, unlike its sibling arms, without the `not upgrade_check` guard. The upgrade check runs that load against the previous release's server (26.8.2.7), which predates #119385: it matches the sorting key `CAST(json.b, 'String')` to the same expression in `ORDER BY` by name and arity only, although under `cast_keep_nullable = 1` the query types it `Nullable(String)` while the key is `String`. Read-in-order with `read_in_order_use_virtual_row = 1` then aborts the shipped 26.8 server in `setVirtualRow` with `Logical error: Virtual row has different type` while running `03277_json_subcolumns_in_primary_key`, so `Upgrade check (amd_release)` went red on dozens of unrelated pull requests (57 on 2026-09-11, 23 on 2026-09-12, 7 on 2026-09-13). A master fix cannot clear it, because the exception is raised by the old binary. Guard the arm like the sibling `serialize_query_plan` arm (5620afa). CI: https://s3.amazonaws.com/clickhouse-test-reports/praktika.html?PR=99495&sha=c84da07b7cc18cc399ef06b7fc656f99a72b713a&name_0=PR&name_1=Upgrade%20check%20(amd_release) PR: #99495 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit 6b1566c)

groeneai added can be tested Allows running workflows for external contributors groeneai-origin-ci-master PR origin: master/nightly CI monitoring finding labels Sep 11, 2026

clickhouse-gh Bot closed this Sep 11, 2026

clickhouse-gh Bot reopened this Sep 11, 2026

clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Sep 11, 2026

clickhouse-gh Bot added the comp-query-optimizer Query plan optimization: physical plan steps, plan-level rewrites and optimizations (QueryPlan pa... label Sep 11, 2026

groeneai mentioned this pull request Sep 11, 2026

Fix DeltaLake read hanging forever when the scan thread reports an error #119345

Open

clickhouse-gh Bot reviewed Sep 11, 2026

View reviewed changes

This was referenced Sep 11, 2026

Decode morton codes without a lookup table on AArch64 #116205

Open

Make 05019_parquet_lazy_materialization_file_change deterministic with an injection fail point #119296

Merged

bacek mentioned this pull request Sep 11, 2026

Measure WASM UDF input batches whole instead of summing per-row probes #118989

Open

This was referenced Sep 11, 2026

Support group_by_use_nulls in WITH TOTALS #107106

Open

Fix FunctionDynamicAdaptor exception when nested function returns Nothing type #115982

Open

groeneai mentioned this pull request Sep 11, 2026

Reject text indexes on multidimensional array columns #116895

Open

vdimir reviewed Sep 11, 2026

View reviewed changes

Merge remote-tracking branch 'origin/master' into groeneai/fix-matcht…

18e3966

…rees-result-type-agreement

This was referenced Sep 11, 2026

Fix a crash and a LOGICAL_ERROR when trim(BOTH '' FROM x) is used where a function is expected #119317

Merged

Fix ALTER TABLE ... COMMENT COLUMN being silently discarded on a materialized view with an inner table #119469

Open

Avogar approved these changes Sep 11, 2026

View reviewed changes

clickhouse-gh Bot assigned Avogar Sep 11, 2026

alexey-milovidov mentioned this pull request Sep 12, 2026

Fix isTrivialCast to recognize a Nullable-widening _CAST #119083

Open

1 task

alexey-milovidov added this pull request to the merge queue Sep 12, 2026

alexey-milovidov mentioned this pull request Sep 12, 2026

See non-determinism inside a lambda body in the plan-time join conversions #117248

Merged

Merged via the queue into ClickHouse:master with commit 5b33256 Sep 12, 2026
175 checks passed

robot-ch-test-poll3 added the pr-synced-to-cloud The PR is synced to the cloud repo label Sep 12, 2026

alexey-milovidov mentioned this pull request Sep 12, 2026

Fix logical error on a non-transactional DROP PARTITION over an uncommitted part #105116

Merged

1 task

This was referenced Sep 13, 2026

Do not randomize cast_keep_nullable in the upgrade check's pre-upgrade load #119768

Merged

Add GradualResizeProcessor to limit effective parallelism for GROUP BY on small data volumes #99495

Draft

antonkovalenko mentioned this pull request Sep 13, 2026

Preserve stable row order during insert-time sorting #119474

Merged

Uh oh!

Conversation

groeneai commented Sep 11, 2026 • edited by clickhouse-gh Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Description

Uh oh!

groeneai commented Sep 11, 2026

Uh oh!

clickhouse-gh Bot commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

Findings

Tests

Final Verdict

LLVM Coverage Report

Uh oh!

groeneai commented Sep 11, 2026

Uh oh!

groeneai commented Sep 11, 2026

Uh oh!

clickhouse-gh Bot Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

groeneai Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Build profile diff (arm_release)

Uh oh!

groeneai commented Sep 11, 2026

CI finish ledger - 739fdff

Uh oh!

vdimir Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

groeneai Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

PedroTadim commented Sep 11, 2026

Uh oh!

PedroTadim commented Sep 11, 2026

Uh oh!

groeneai commented Sep 11, 2026

Uh oh!

Avogar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

groeneai commented Sep 11, 2026 •

edited by clickhouse-gh Bot

Loading

clickhouse-gh Bot commented Sep 11, 2026 •

edited

Loading

clickhouse-gh Bot commented Sep 11, 2026 •

edited

Loading

CI finish ledger - `739fdff`