iframe-proxy

groeneai · 2026-06-10T19:34:55Z

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):

Fix TYPE_MISMATCH error ("Cannot convert string ... to type ...") for ORDER BY <numeric column> ... LIMIT n queries when lazy materialization placed another column before the sort column. The top-K threshold is now read from the correct sort column.

Description

Closes #106294. Regression introduced in 26.5.

MergeSortingTransform::remerge() published the top-K dynamic-filter threshold by reading physical column position 0 of the sorted chunk (chunks[0].getColumns()[0]), assuming the first sort column is the first column in the chunk. With query_plan_optimize_lazy_materialization enabled, the Sorting step's input header can place a WHERE-only column before the sort column. For the reporter's shape SELECT path, file_name, file_size FROM t WHERE path != '' ORDER BY file_size DESC LIMIT n, the lazy-materialized plan reads path (String) at position 0 and the sort column file_size (UInt64) at position 1, so the threshold tracker received a String value. The injected __topKFilter(file_size) prewhere then called convertFieldToType(stringValue, UInt64), throwing TYPE_MISMATCH.

The fix resolves the first sort column's actual position by name from header_without_constants (which matches the chunk column order), the same way the sibling PartialSortingTransform already does. The failure only surfaced through the MergeSortingTransform single-stream remerge path; PartialSortingTransform was unaffected.

A stateless regression test (04327_lazy_materialization_topk_filter_column_position) forces the remerge path (max_threads = 1) and asserts the optimized result matches the unoptimized one.

Version info

Merged into: 26.6.1.852

groeneai · 2026-06-10T19:35:13Z

Pre-PR validation gate

#	Question	Answer
a	Deterministic repro?	Yes. `clickhouse local` running the issue's query on a 3-part table fails 100% with `Cannot convert string ... to type UInt64` (`__topKFilter(file_size)`). Minimized deterministic trigger: 2 parts x 15K+ rows, `max_block_size=8192, max_threads=1` (forces the MergeSortingTransform single-stream remerge path).
b	Root cause explained?	Yes. `MergeSortingTransform::remerge()` published the top-K threshold via `chunks[0].getColumns()[0]->get(limit-1, value)` (hardcoded physical position 0). With lazy materialization the Sorting step's input header places the WHERE-only `path` (String) at position 0 and the sort column `file_size` (UInt64) at position 1, so the threshold tracker stored a String value; `__topKFilter(file_size)` then called `convertFieldToType(stringPath, UInt64)` -> TYPE_MISMATCH. Confirmed via debug log: `TopK threshold tracker is updated` fires immediately before the error.
c	Fix matches root cause?	Yes. The threshold is now read from the first sort column's actual position resolved by name (`header_without_constants.getPositionByName(description.front().column_name)`), instead of hardcoded position 0. This directly targets the mechanism in (b), not the symptom.
d	Test intent preserved / new tests added?	New stateless test `04327_lazy_materialization_topk_filter_column_position` added. It forces the remerge path (`max_threads=1`) and asserts the lazy-materialized result is identical to `query_plan_optimize_lazy_materialization=0` (not just 'does not throw').
e	Both directions demonstrated?	Yes. On HEAD (buggy) the test query throws TYPE_MISMATCH; with the fix it returns correct top-20 rows whose cityHash64 sum is byte-identical to the unoptimized path. New test: FAILS on buggy binary, 50/50 PASS on fixed binary (randomized MergeTree settings on).
f	Fix is general, not a narrow patch?	Yes. Grepped every threshold-publish site: `MergeSortingTransform.cpp` was the ONLY hardcoded-position-0 read. The sibling `PartialSortingTransform` already resolves the position by name (`description_with_positions`) and was unaffected. Verified ASC, multi-column sort, and the common 'sort column first' case all stay correct (lazy on == lazy off).

Session id: cron:clickhouse-worker-slot-26:20260610-184200

groeneai · 2026-06-10T19:35:58Z

cc @KochetovNicolai @vdimir — could you review this? It fixes the #106294 regression where MergeSortingTransform published the top-K dynamic-filter threshold from hardcoded chunk position 0; with lazy materialization a WHERE-only column can precede the sort column, so the threshold picked up the wrong column and __topKFilter threw TYPE_MISMATCH. The fix resolves the sort column position by name, matching what PartialSortingTransform already does.

clickhouse-gh · 2026-06-10T19:41:31Z

Workflow [PR], commit [7172412]

Summary: ❌

job_name	test_name	status	info	comment
Unit tests (amd_llvm_coverage)		FAIL
	-FunctionsStress	FAIL	cidb, issue	ISSUE EXISTS
LLVM Coverage		DROPPED		IGNORED

AI Review

Summary

This PR fixes the TYPE_MISMATCH exception in lazy-materialized ORDER BY ... LIMIT top-K plans by publishing the dynamic-filter threshold from the actual first sort-column position in MergeSortingTransform, instead of assuming physical column position 0. The regression test now pins the relevant optimizer settings, asserts that __topKFilter(file_size) and LazilyReadFromMergeTree are present, and compares the optimized result against the query_plan_optimize_lazy_materialization = 0 path. I found no remaining correctness, safety, or test-coverage blockers in the current diff.

Final Verdict

Status: ✅ Approve

groeneai · 2026-06-11T00:24:33Z

CI fully finished (Finish Workflow passed). The single failure is Stateless tests (arm_asan_ubsan, azure, sequential) -- known azure-infra-outage / db-disk "Server died" chronic class, unrelated to this PR (lazy-materialization topKFilter TYPE_MISMATCH fix). Not PR-caused. No Fast/Bugfix/Style failure.

groeneai · 2026-06-12T16:46:12Z

Done in 4676779. Thanks, this was a real false-negative: the original test passed even against the pre-fix binary.

Two parts to the fix:

Added the EXPLAIN actions = 1 assertion you suggested. It checks that both __topKFilter(file_size) and LazilyReadFromMergeTree are present, so the threshold-publishing path is proven to be exercised.
Pinned use_top_k_dynamic_filtering, query_plan_max_limit_for_top_k_optimization, query_plan_max_limit_for_lazy_materialization (and max_threads=1, max_block_size, query_plan_optimize_lazy_materialization). Important detail I found while validating: pinning them inside the scalar subqueries (as the original test did) is NOT enough. clickhouse-test injects the randomized values as session/CLI settings, and a subquery-level SETTINGS clause does not override a session-level value. So with --use_top_k_dynamic_filtering 0 injected, the optimization was disabled and the test passed against the buggy code. The pins are now SET statements at the top of the test (session level), which do take precedence over the injected CLI values. query_plan_optimize_lazy_materialization is still toggled per-subquery (1 vs 0) since that is the only intended difference between the two halves of the equality check.

Validated both directions through the actual clickhouse-test runner with randomization on:

pre-fix binary (hardcoded position 0): 10/10 FAIL (TYPE_MISMATCH), confirming the test now catches the regression.
fixed binary: 30/30 PASS.

(For comparison, the original subquery-pinned test gave 5/5 false OK on the buggy binary.)

Session id: cron:clickhouse-worker-slot-1:20260612-154400

groeneai · 2026-06-12T21:15:52Z

Follow-up on the EXPLAIN actions=1 assertion (HEAD 3d08b5350ee).

The presence assertion the test added (__topKFilter(file_size) + LazilyReadFromMergeTree) correctly proves the optimized path is exercised, but it surfaced on the old analyzer matrix: lazy materialization is gated on the new analyzer (QueryPlanOptimizationSettings.cpp: optimize_lazy_materialization = query_plan_optimize_lazy_materialization && allow_experimental_analyzer). Under enable_analyzer=0 the __topKFilter/LazilyReadFromMergeTree plan never forms, so the assertion returned 0 and 04327 failed only on Stateless tests (amd_llvm_coverage, old analyzer, ...) while passing on every new-analyzer variant + 50/50 flaky checks.

Fix: tagged the test no-old-analyzer (the test validates a new-analyzer-only optimization, so it is skipped under the old analyzer, like other new-analyzer plan-assertion tests).

Validated with the server default set to enable_analyzer=0 (the old-analyzer job condition, via users.d/analyzer.xml):

old analyzer: [ SKIPPED ] Reason: old-analyzer (was [ FAIL ] before the tag)
new analyzer: [ OK ], 20/20 with randomization on

No source change; the MergeSortingTransform column-position fix is unchanged.

Session: cron:clickhouse-worker-slot-4:20260612-210100

MergeSortingTransform::remerge() published the top-K dynamic-filter threshold by reading physical column position 0 of the sorted chunk (chunks[0].getColumns()[0]). That assumes the first sort column is the first column in the chunk, which is not generally true. With query_plan_optimize_lazy_materialization the Sorting step's input header can place a WHERE-only column before the sort column. For `SELECT path, file_name, file_size FROM t WHERE path != '' ORDER BY file_size DESC LIMIT n` the lazy-materialized plan reads `path` (String) at position 0 and the sort column `file_size` (UInt64) at position 1. The threshold tracker then received a String path value, and the injected `__topKFilter(file_size)` prewhere called convertFieldToType(stringPath, UInt64), throwing "Cannot convert string ... to type UInt64" (TYPE_MISMATCH). The failure only surfaced via the MergeSortingTransform single-stream remerge path; the sibling PartialSortingTransform already resolves the sort column position by name (description_with_positions), so it was unaffected. Resolve the first sort column's actual position by name from header_without_constants (which matches the chunk column order) instead of hardcoding 0. Closes ClickHouse#106294 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

groeneai · 2026-06-13T04:13:24Z

CI finished on a2e7e7a. Finish Workflow passed; 147 jobs, ~229.9k tests, 1 red.

Fast test (arm_darwin) / 04336_parallel_blocks_marshalling_low_cardinality_native_format: not PR-caused. Known macOS-only flake (the test uses remote('127.0.0.{1,2}'); 127.0.0.2 is unbindable on Darwin), failing across 20+ unrelated PRs. Tracked separately with fix PR Skip macOS-incompatible distributed tests 04327 and 04336 #107376 (adds it to ci/defs/darwin.skip). This PR touches only MergeSortingTransform.cpp + the 04327 test, neither related to 04336.

No PR-caused failures. Ready for review.

…n-position

clickhouse-gh · 2026-06-13T15:33:05Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	84.70%	84.70%	+0.00%
Functions	92.30%	92.30%	+0.00%
Branches	77.30%	77.30%	+0.00%

Changed lines: Changed C/C++ lines covered by tests: 5/5 (100.00%) | Lost baseline coverage (was covered on master, now uncovered in this PR): 2 line(s) · Uncovered code

Full report · Diff report

groeneai · 2026-06-13T16:55:34Z

CI finished on HEAD 446bd56f063a (the auto master-merge). Both reds are pre-existing flaky failures, not caused by this PR (which only touches MergeSortingTransform.cpp + the 04327 test):

Stateless tests (amd_llvm_coverage, ParallelReplicas, s3 storage, parallel) / 02770_async_buffer_ignore — flaky, 7 failures across 7 unrelated PRs / 0 on master in 14d. Already has a fix in flight (Fix flaky test 02770_async_buffer_ignore #107298).
Integration tests (arm_binary, distributed plan, 1/4) / test_refreshable_mv_keeper_loss::test_keeper_session_loss_during_coordinated_refresh — flaky keeper-session-loss fault-injection test, 6 failures across 5 unrelated PRs / 0 on master in 14d. Unrelated to sorting.

Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=107060&sha=446bd56f063a3d7850563ef6939a4907ce62ec7e&name_0=PR

The lazy-materialization MergeSortingTransform column-position fix and its 04327 regression test are unchanged and validated both directions.

…er_column_position.sql

groeneai · 2026-06-15T21:44:55Z

groeneai mentioned this pull request Jun 10, 2026

Regression in 26.5: lazy-materialization __topKFilter throws TYPE_MISMATCH on ORDER BY ... LIMIT (works in 26.1) #106294

Closed

PedroTadim added the can be tested Allows running workflows for external contributors label Jun 10, 2026

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

nickitat self-assigned this Jun 12, 2026

groeneai force-pushed the fix/106294-lazy-materialization-topk-column-position branch from c096332 to fdceaf7 Compare June 12, 2026 14:50

clickhouse-gh Bot reviewed Jun 12, 2026

View reviewed changes

Comment thread tests/queries/0_stateless/04327_lazy_materialization_topk_filter_column_position.sql Outdated

groeneai force-pushed the fix/106294-lazy-materialization-topk-column-position branch from fdceaf7 to 4676779 Compare June 12, 2026 16:45

groeneai force-pushed the fix/106294-lazy-materialization-topk-column-position branch from 4676779 to 3d08b53 Compare June 12, 2026 21:15

groeneai force-pushed the fix/106294-lazy-materialization-topk-column-position branch from 3d08b53 to a2e7e7a Compare June 12, 2026 23:54

Merge branch 'master' into fix/106294-lazy-materialization-topk-colum…

446bd56

…n-position

alexey-milovidov mentioned this pull request Jun 13, 2026

Skip macOS-incompatible distributed tests 04327 and 04336 #107376

Merged

nickitat reviewed Jun 15, 2026

View reviewed changes

Comment thread src/Processors/Transforms/MergeSortingTransform.cpp

nickitat reviewed Jun 15, 2026

View reviewed changes

Comment thread tests/queries/0_stateless/04327_lazy_materialization_topk_filter_column_position.sql Outdated

Update tests/queries/0_stateless/04327_lazy_materialization_topk_filt…

e962f93

…er_column_position.sql

nickitat reviewed Jun 15, 2026

View reviewed changes

Comment thread src/Processors/Transforms/MergeSortingTransform.cpp

Apply suggestion from @nickitat

7172412

nickitat approved these changes Jun 15, 2026

View reviewed changes

nickitat added this pull request to the merge queue Jun 16, 2026

Merged via the queue into ClickHouse:master with commit ed82316 Jun 16, 2026
805 of 815 checks passed

robot-ch-test-poll added the pr-synced-to-cloud The PR is synced to the cloud repo label Jun 16, 2026

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

groeneai commented Jun 10, 2026 • edited by robot-clickhouse 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

Version info

Uh oh!

groeneai commented Jun 10, 2026

Pre-PR validation gate

Uh oh!

groeneai commented Jun 10, 2026

Uh oh!

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

Uh oh!

AI Review

Summary

Final Verdict

Uh oh!

groeneai commented Jun 11, 2026

Uh oh!

Uh oh!

groeneai commented Jun 12, 2026

Uh oh!

groeneai commented Jun 12, 2026

Uh oh!

groeneai commented Jun 13, 2026

Uh oh!

clickhouse-gh Bot commented Jun 13, 2026

LLVM Coverage Report

Uh oh!

groeneai commented Jun 13, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

groeneai commented Jun 15, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

groeneai commented Jun 10, 2026 •

edited by robot-clickhouse

Loading

clickhouse-gh Bot commented Jun 10, 2026 •

edited by nickitat

Loading