8000 25.8 stable: Fix for Aliases and Lazy Materialization by ilejn · Pull Request #2560 · Altinity/ClickHouse · GitHub
[go: up one dir, main page]
Skip to content 1 < 8000 script type="application/json" data-target="react-partial.embeddedData">{"props":{"docsUrl":"https://docs.github.com/get-started/accessibility/keyboard-shortcuts"}}

25.8 stable: Fix for Aliases and Lazy Materialization - #2560

Merged
zvonand merged 2 commits into
antalya-25.8from
bugfix/antalya-25.8/CH-544
Oct 25, 2026
Merged

25.8 stable: Fix for Aliases and Lazy Materialization#2560
zvonand merged 2 commits into
antalya-25.8from
bugfix/antalya-25.8/CH-544

Conversation

@ilejn
@ilejn ilejn commented Oct 13, 2026
Copy link
Copy Markdown

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 to CHANGELOG.md):

Logical Error fixed if both Alias and original column are selected.

Documentation entry for user-facing changes

...

CI/CD Options

Exclude tests:

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Unit tests
  • Performance tests
  • Aarch64 tests
  • All with ASAN
  • All with TSAN
  • All with MSAN
  • All with UBSAN
  • All with Coverage
  • All Regression
  • Disable CI Cache

Regression jobs to run:

  • Fast suites (mostly <1h)
  • Aggregate Functions (2h)
  • Alter (1.5h)
  • Benchmark (30m)
  • CAS (content-addressed storage; Antalya only)
  • ClickHouse Keeper (1h)
  • Iceberg (2h)
  • LDAP (1h)
  • OAuth (5m)
  • Parquet (1.5h)
  • RBAC (1.5h)
  • SSL Server (1h)
  • S3 (2h)
  • S3 Export (2h)
  • Swarms (30m)
  • Tiered Storage (2h)

@ilejn
ilejn commented Oct 13, 2026 •
edited
Loading
Copy link
Copy Markdown
Author

Repro

CREATE TABLE t_str (id UInt64, ts DateTime, s String, s_alias String ALIAS s)
ENGINE = MergeTree ORDER BY id;
INSERT INTO t_str SELECT number, toDateTime('2026-01-01 00:00:00') + number, toString(number) FROM numbers(100);
SELECT s, s_alias FROM t_str ORDER BY ts DESC LIMIT 10;

The issue is introduced by ClickHouse#55518 and fixed by ClickHouse#90309 , 25.8 remained not covered.

The change is not a backport, it is a very small fix.

@github-actions
github-actions Bot commented Oct 13, 2026 •
edited
Loading
Copy link
Copy Markdown

Workflow [PR], commit [a2ea700]

@ilejn
Fix for Aliases and Lazy Materialization
e47d7e8 
Signed-off-by: Ilya Golshtein <igolshtein@altinity.com>
@ilejn
ilejn force-pushed the bugfix/antalya-25.8/CH-544 branch from 6f0a6cb to e47d7e8 Compare October 13, 2026 16:49
@ilejn
test added
a2ea700 
Signed-off-by: Ilya Golshtein <igolshtein@altinity.com>
@ilejn
ilejn commented Oct 15, 2026
Copy link
Copy Markdown
Author

PR to upstream ClickHouse#126104

@ilejn ilejn changed the title Do Not Merge! Fix for Aliases and Lazy Materialization 25.8 stable: Fix for Aliases and Lazy Materialization Oct 22, 2026
@ilejn ilejn added the 25.8 25.8 Altinity Stable label Oct 22, 2026
ianton-ru
ianton-ru approved these changes Oct 22, 2026
View reviewed changes
@ianton-ru ianton-ru left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
In later versions code was fully refactored in ClickHouse#90309, so fix is required only in 25.8 and older.

@zvonand
zvonand commented Oct 25, 2026
Copy link
Copy Markdown
Member

@blau-ai

@blau-ai
blau-ai commented Oct 25, 2026
Copy link
Copy Markdown
Collaborator

CI triage — 1 real failure, not caused by this PR

Verdict: The only genuine failure is Stress test (amd_ubsan). It is a pre-existing / flaky stress-test crash in the backup-RESTORE path, unrelated to this PR's change. (FinishCIReport and the PR gate are just rollups of that one failure.) Everything the PR actually touches is green.

Why it's not this PR

This PR only changes the lazy-materialization query-plan optimizer and its test:

  • src/Processors/QueryPlan/Optimizations/optimizeLazyMaterialization.cpp
  • tests/queries/0_stateless/05317_aliases_and_lazy_materialization.{sql,reference}

The crash is in a completely different subsystem — RESTORE TABLE ... FROM Disk(...zip...) → minizip → ZipArchiveReader → BackupImpl → MergeTreeData::restorePartFromBackup. The new stateless test added here (05317_...) passed in every stateless suite (Fast test, amd_debug/ubsan/asan parallel+sequential), so the PR's own code works as intended.

The failure

Stress test (amd_ubsan) → Server died / Killed by signal 6 (Aborted). Decisive line from fatal_messages.txt:

<Fatal> : Logical error: 'ReadBuffer is canceled. Can't read from it.'
(query: RESTORE TABLE default.mt_250_parts AS manyparts
        FROM Disk('backups', 'test_c2c08quc/mt_250_parts.zip') SETTINGS allow_different_table_def=true)
Received signal Aborted (6)

Stack: ReadBuffer::next() (src/IO/ReadBuffer.cpp:90, the chassert(!isCanceled(), "ReadBuffer is canceled...")) ← mz_stream_ioapi_read ← ZipArchiveReader::HandleHolder::locateFile ← BackupImpl::copyFileToDisk ← MergeTreeData::restorePartFromBackup.

Evidence it's pre-existing, not PR-induced

Suggested next step

  • Safe to re-run Stress test (amd_ubsan) — expect it to pass (as amd_debug did). No code change is warranted on this PR for it.
  • If it keeps reproducing on re-run, it's an infra/backup-race issue to track separately against the base branch (antalya-25.8), not a blocker for this fix.

Head SHA analyzed: a2ea700. Source: praktika result_pr.json + stress_test_amd_ubsan/fatal_messages.txt.

🤖 Generated with Claude Code

@zvonand
zvonand merged commit 75eae1f into antalya-25.8 Oct 25, 2026
219 of 223 checks passed
@alsugiliazova
alsugiliazova commented Oct 28, 2026
Copy link
Copy Markdown
Member

PR #2560 Audit Review

AI audit note: This review was generated by AI (Grok 4.7).

Verdict

One Low finding. No High or Medium.

The reported logical error is fixed on the default plan. Selecting a column and its alias under ORDER BY … LIMIT reads the source eagerly, so the lazy reader never sees two copies of the same row coordinates. Selecting only the alias still lazy-reads the source, as long as expression merging stays on.

The Low finding is the same check applied one step too early when query_plan_merge_expressions is off. Results stay correct. The source column is read before the limit.

What the change does

removeUsedColumnNames walks each expression and filter between the MergeTree read and the sort. A column still in the lazy set at the end is not read with the part. After LIMIT, LazilyRead fills it from row and part coordinates stored in a ColumnLazy.

The new check, in removeUsedColumnNames (src/Processors/QueryPlan/Optimizations/optimizeLazyMaterialization.cpp), counts how many outputs unwrap to the same INPUT. The second time that name appears, the physical column is removed from the lazy set and read with the rest of the query:

/// The lazy reader maps each source column to a single output name. If an input
/// is forwarded more than once (for example, both a column and its alias), read
/// it eagerly so all outputs contain real data and do not duplicate lazy row coordinates.
if (node->type == ActionsDAG::ActionType::INPUT && !passthrough_inputs.insert(node->result_name).second)
{
    const auto it = alias_index.find(node->result_name);
    if (it != alias_index.end())
        lazily_read_column_name_set.erase(it->second);
}

alias_index is unchanged. It still keeps a single live name per source: when an output is an alias of an input, the input name is erased and the alias name is inserted (optimizeLazyMaterialization.cpp, the second loop in removeUsedColumnNames).

The stateless test is tests/queries/0_stateless/05317_aliases_and_lazy_materialization.sql. It selects s and s_alias (String ALIAS s) with ORDER BY ts DESC LIMIT 10.

Why the crash happens

injectLazilyReadColumns puts the row and part coordinates into the first lazy column and gives every later lazy column an empty ColumnLazy. ColumnLazyTransform skips the empty ones and calls MergeTreeLazilyReader::transformLazyColumns once. The reader then fills every lazy output from that one coordinate column, matching header names through alias_index.

An alias of a ColumnLazy copies the captured coordinates, so both the source and the alias arrive as non-empty ColumnLazy values. The transform calls transformLazyColumns twice. The second call fails chassert(res_columns.empty()) in debug and sanitizer builds. In a release build the assert is compiled out, the second call appends another result, and the source column is left as ColumnLazy. Formatting it throws NOT_IMPLEMENTED / LOGICAL_ERROR. That is the failure the changelog names.

MergeTreeLazilyReader's constructor makes the same mistake from the other side. It registers at most one header column per source, because alias_index has already dropped the source name in favor of the alias. The source column in the block is never replaced with real data.

Why the fix holds on the default plan

query_plan_merge_expressions defaults to on, and that pass runs before lazy materialization. The alias step, the rename to column identifiers, the projection, and the BEFORE ORDER BY expression are fused into one expression under the sort.

buildActionsDAGFromExpressionNode starts from the full input header and then replaces the outputs with the projection list. Merging consumes every child output whose name is an input of that projection, so the fused outputs are the select list (plus the sort key). ActionsDAG::trivial() is true for a DAG of only inputs and aliases, so liftUpFunctions leaves that expression under the sort. Lazy materialization still sees Limit → Sorting → Expression → Read.

Query Fused outputs that unwrap to s What the check does
SELECT s, s_alias … ORDER BY ts LIMIT 10 two Erases s. Both outputs are real strings. ts is erased by the sort walk. The lazy set is empty and the optimization does not run. The test's expected rows are the eager-read result.
SELECT s_alias … ORDER BY ts LIMIT 10 one s stays lazy. The reader maps the single alias name.
SELECT s AS a one Same as a single alias.
SELECT s, s AS a two Same erase as the table-alias case.

A function, ARRAY JOIN, a filter column, or a sort key still erases its inputs on the pre-existing walks. Those paths do not depend on the new check.

Findings

Duplicate passthrough

Low: with expression merging off, computing a table ALIAS column disables lazy materialization of its source.

  • Impact: SELECT s_alias FROM t_str ORDER BY ts DESC LIMIT 10 reads s for every candidate row. The values are correct. The limit no longer skips the source column. Default installations do not hit this: query_plan_merge_expressions is true in src/Core/Settings.cpp.
  • Anchor: the new check in removeUsedColumnNames (optimizeLazyMaterialization.cpp). The DAG that trips it is built by createComputeAliasColumnsStep (src/Planner/PlannerJoinTree.cpp): outputs start as every column of the read header, then the alias is appended. removeUnusedActions(false) keeps the source because it is an output.
  • Trigger: SET query_plan_merge_expressions = 0, lazy materialization left at its default (true, max limit 10), and a query that selects an ALIAS column without the source, ordered by some other column.
  • Why defect: removeUsedColumnNames runs on every expression between the read and the sort, bottom up. The alias step forwards s twice, so s is erased before the projection drops it. The header MergeTreeLazilyReader is constructed from is the sorting output, which then contains the alias once. One name is a case the reader already handles.
  • Fix direction: treat a source as duplicated only when the sorting output header still contains two names for it.
  • Regression test direction: with query_plan_merge_expressions = 0, EXPLAIN the alias-only query and assert s is still in Lazily read columns. Separately, SELECT s, s_alias, fat … ORDER BY ts LIMIT 10 should show fat lazy and s absent from that list.

What was checked and found clean

The reported query under default settings. Both names unwrap to one INPUT after merging, the source is eager, and ColumnLazy coordinates are not duplicated. Deleting the new check brings the logical error back for this query, which is what the stateless test locks. The test does not keep a sibling column lazy: after the fix the lazy set for that query is empty (s removed by the new check, ts removed because it is the sort key), so the optimization returns before inserting LazilyRead.

A single alias, whether a table ALIAS or s AS a, stays one passthrough on the fused DAG and stays lazy.

NameSet::insert returns pair<iterator, bool>; .second is the right field. The lookup and the erase use different containers. The set is local to the function. Planning this pass is single-threaded. No new lock, no iterator invalidation, no overflow.

injectLazilyReadColumns is unchanged. The first lazy column still carries the coordinates and the rest stay empty, which is safe once a duplicated source is not lazy.

Mutations with patch parts, FINAL, sampling, and an already-replaced vector column still refuse the optimization in canUseLazyMaterializationForReadingStep and the patch-part check. This PR does not touch those guards.

Coverage

Category Status Note
Duplicate passthrough of one INPUT Reviewed One Low finding when merging is off. Default fused plan matches the reader's one-name rule
Function / ARRAY JOIN / filter / sort-key consumers Reviewed Pre-existing erases. The new check does not weaken them
Alias-index remap (one live name per source) Reviewed Still drops the source name when an alias output exists. Harmless once a duplicated source is eager
Empty lazy set Reviewed Optimization bails out. The reported test lands here
ColumnLazy lifetime across the limit Reviewed One coordinate column, then one transformLazyColumns call, provided duplicates were erased
Expression merging and liftUpFunctions Reviewed Merge runs first. A pure alias DAG is trivial() and stays under the sort
Exception / partial update during planning Reviewed The lazy set is built before updateLazilyReadInfo. A throw before that writes nothing
Memory lifetime, iterator invalidation, overflow, data races Reviewed Local NameSet. No shared mutable state added
Lock order / cross-thread lifetime N/A No new synchronization
Protocol / version skew / auth N/A No RPC, format, or privilege change

Deferred: none. No EXPLAIN was captured and the stateless test was not re-run. The plan shape above is from the planner and the optimizer passes at a2ea700.

Operational readiness

Persona: anyone running SELECT with lazy materialization left on (the 25.8 default).

Primary flow: SELECT of a column and its alias, or of the alias alone, with ORDER BY on another column and LIMIT at most 10.

The default flow is ready. Both selected names come back as real values. The alias alone is still lazy-read.

The only guardrail is for the Low finding: leave query_plan_merge_expressions at its default if you depend on lazy materialization of an ALIAS column. Turning it off makes that column eager for the whole ORDER BY input. No query result changes.

Recommended fix: move the duplicate check from each intermediate expression onto the sorting output header, and extend 05317 with the alias-only EXPLAIN and a sibling column that must stay lazy.

@DimensionWieldr
DimensionWieldr commented Oct 28, 2026
Copy link
Copy Markdown
Collaborator

Bugfix strictly for 25.8 and older versions.

No PR-caused CI fails.

AI audit returned only low defects.

LGTM

@DimensionWieldr DimensionWieldr added the verified Approved for release label Oct 28, 2026
@ilejn ilejn mentioned this pull request Oct 28, 2026
Open
9 of 30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ianton-ru ianton-ru ianton-ru approved these changes

Assignees

No one assigned

Labels

25.8 25.8 Altinity Stable antalya antalya-25.8 antalya-25.8.33.20003 verified Approved for release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@ilejn @zvonand @blau-ai @alsugiliazova @DimensionWieldr @ianton-ru @svb-alt
0