Conversation
Tests only, no source changes. Purpose is to establish which of the `__aliasMarker` regressions from #1844 still reproduce on `antalya-26.6`, which already carries the upstream fixes that landed after that PR was written: * ClickHouse#94644 (ALIAS column order) * ClickHouse#107675 (distributed ALIAS column order) * ClickHouse#107913 (`buildShardCollapseFanOut`) All 22 distinct test files from #1844 are staged: the 18 active ones plus the 4 unique files kept under `_local_files_and_notes/dropped_tests` there. The six dropped-test files that duplicate active ones byte-for-byte are staged once. Slot numbers from the 26.3 branch that are already taken on 26.6 are reassigned to the 05053-05063 range: 03843 -> 05053 03923 -> 05056 04286 -> 05059 03926 -> 05062 03920 -> 05054 03928 -> 05057 03924 -> 05060 03927 -> 05063 03921 -> 05055 03930 -> 05058 03925 -> 05061 `03648_alias_marker_with_mergeable_state.sh` is updated to the #1844 revision. It is a contract test for the reworked marker, not a bug reproducer, so it is expected to fail until the source change lands. Related: #1844 Related: ClickHouse#106402
…ALIAS columns
A `Distributed` query that reads the same-named `ALIAS` column from two
`JOIN` sources fails on the shard:
Multiple expressions __aliasMarker(__table2.x, '__table4.foo') AS foo
and __aliasMarker(__table1.x, '__table3.foo') AS foo
for alias foo
`ReplaseAliasColumnsVisitor` stamped `setAlias(column_name)` on every marker it
created, so two markers carrying different ids both ended up named `foo` in the
same scope. It also aliased `column_node->getExpression()` in place, which is
shared between occurrences, so aliasing one occurrence changed the others.
Set the alias only where it is read. A top-level projection item keeps it so
the mergeable-state output column keeps its name, and a `JOIN USING` key side
keeps it because the shipped SQL renders the side rather than the key and
`rejectUnshippableJoinUsingKeys` reads that shape. Everywhere else -- `WHERE`,
`GROUP BY`, `ORDER BY`, `HAVING`, `JOIN ON` -- no alias is set, which is what
removes the collision. Clone the defining expression per occurrence so setting
an alias on one no longer reaches another.
The pass moves from `StorageDistributed` to `buildQueryTreeForShard` as
`inlineAliasColumns`, matching the name, file and semantics of the shared
inliner upstream introduced in
ClickHouse#107700, which fixes the same
class of failure (ClickHouse#107990)
without markers. Writing it there keeps the next version bump a merge rather
than a rewrite. The signature takes a `ContextPtr` that upstream's does not,
because marker injection needs the `enable_alias_marker` setting and function
resolution; with the setting off, the pass behaves exactly as upstream's.
Not ported here: the parallel-replicas call sites ClickHouse#107700 adds. No staged test
needs them yet.
Verified against the 20 staged regression tests from
#1844:
`05054_distributed_global_alias_marker_matrix` now passes, and the other 19
are byte-identical to the run before this change -- same error codes, same
messages. `05061` covers `enable_alias_marker = 0`, and `03844` / `05055`
cover nested marker chains; all three still pass.
Related: #1844
Related: ClickHouse#107700
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reading an `ALIAS` column from a `Merge` table whose child is `Distributed`
fails on the initiator:
Not found column x: in block __table1.x UInt64, plus(__table1.x, 1_UInt8) UInt64
`ReadFromMerge::convertAndFilterSourceStream` recomputes alias values at the
Merge level and emitted each one under its plain logical name, while the Merge
target header names its columns with analyzer identifiers. The later
`addMissingDefaults` then found no `__tableN.<alias>` column and filled it with
type defaults. The final header reconciliation matched by position, so a child
that returned the same columns in a different order -- which a `Distributed`
child does, because it inlines alias expressions on the shard and returns them
in whatever order the shard's `ActionsDAG` produced -- silently paired values
with the wrong columns.
Emit each alias output under its analyzer identifier instead, and reconcile the
child header by name. The plain-name to identifier mapping is built by matching
each column declared on the Merge table against the suffixes of the child
header names, rather than by pattern-matching the analyzer's `__tableN.` naming
convention. Driving it from the declared schema is what makes dotted names
work: a `Nested` subcolumn arrives as ``__table1.`n.a` `` on some branches and
`__table1.n.a` on others, and splitting on the last dot mangles both.
Alias expressions name their inputs in plain logical names, so every mapped
column is also exposed under its plain name in the per-alias `ActionsDAG`. An
`ALIAS` defined over another `ALIAS` needs that to resolve.
`makeConvertingActionsPreferNameThenPosition` in `Planner/Utils` performs the
reconciliation. It matches by name only when both headers carry the same set of
names with no duplicates, which makes the rename a pure reordering, and matches
by position otherwise. The position branch is load-bearing rather than a safety
net: a source header can legitimately carry names the result never mentions,
such as the synthetic aggregate-state column behind an optimized trivial
`count()`. A `LOG_TEST` line reports which call site fell back and why.
`convertAndFilterSourceStream` takes the Merge table's `ColumnsDescription` for
this; the planner's `TableExpressionData` is not populated yet at that point,
so the mapping cannot be looked up through `PlannerContext`.
The `TODO(storage-merge-alias)` comment records why this recomputation exists
at all and what would remove it. Its two steps are internal to `StorageMerge`
and unrelated to the commits around it.
Verified against the 20 staged regression tests from
#1844:
`04281_storage_merge_over_distributed_alias`,
`05057_merge_over_distributed_alias_marker_column_swap` and
`05059_dotted_alias_merge_over_distributed` now pass, and the four remaining
failures are byte-identical to the previous run.
Related: #1844
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reading an `ALIAS` column of a Hybrid table from inside a subquery fails on
the initiator:
Cannot find column `max(__table1.computed)` in source stream,
there are only columns: [max(__table2.computed)]
The marker id was built when the marker was injected, from the table alias the
initiator had assigned at that moment. `buildQueryTreeForShard` then runs
`createUniqueAliasesIfNecessary`, which renumbers the `__tableN` aliases of the
shard tree from 1. The header the initiator matches against is built from the
renumbered tree, but the id inside the marker is a `String` constant and keeps
the old number. The shard duly emits a column under the stale name and nothing
matches it.
Keep the id as a live `ColumnNode` from injection onward, so the analyzer
passes transform it along with the rest of the tree, and materialize it into a
`String` constant only at a serialization boundary, in
`finalizeAliasMarkersForDistributedSerialization`. There are two such
boundaries: `buildQueryTreeDistributed`, right after `buildQueryTreeForShard`,
and `executeSubqueryNode`, where a `GLOBAL IN` / `GLOBAL JOIN` subquery is
materialized and shipped as a temporary table. Finalizing in the second place
also stabilizes the tree hash that names the temporary table, which otherwise
hashes a `ColumnNode` whose identifier can still change.
The finalize pass walks bottom-up, so a nested marker chain materializes from
the inside out, and it does not descend into lambda bodies: a marker written by
hand as `arrayMap(x -> __aliasMarker(x, x), ...)` resolves to a lambda
parameter that has no table source to build an id from.
`FunctionAliasMarker` accepts any second argument now, because for most of its
life that argument is a `ColumnNode` rather than a `String`. The rejection it
replaced bought nothing: the function is a pass-through identity, so a marker a
user writes by hand is harmless, and turning it into a server-side error only
made valid SQL fail. For the same reason the id argument is no longer declared
always-constant. Both planner sites fall back to naming the node after the
expression the marker wraps when no finalized id is present, which is the name
it would have had with no marker at all.
Two deliberate differences from the original change in
#1844. `NormalizeAliasMarkerVisitor`
is kept: it flattens nested marker chains at SQL-render time, which happens
after finalization, and the nested-chain tests pass with it in place. And the
finalize pass never throws: a marker it cannot build an id for is left alone
rather than raising a `LOGICAL_ERROR` that valid user SQL can reach.
Verified against the 20 staged regression tests from
#1844:
`03842_hybrid_alias_issue_1424` and
`03933_alias_marker_direct_use_no_logical_error` now pass, nothing regressed,
and the two remaining failures are tests asserting a `serverError` that
upstream fixes no longer raise.
Related: #1844
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three of the staged regression tests expect a `serverError` that upstream fixes stopped raising, so each was asserting a bug that no longer exists. Replace the assertion with the result the query now returns, and fix the framing that went with it. `03934` claimed `enable_alias_marker` was a correctness toggle: with the marker off, a distributed-over-distributed read of a `String` ALIAS and a `UInt64` ALIAS swapped the two columns and failed with `CANNOT_PARSE_TEXT`. Upstream fixed the column ordering, so the setting is what it was always meant to be -- a way for an initiator to stop emitting `__aliasMarker` for a mixed-version cluster whose shards do not understand it. The test now runs the same query with the marker on and off and requires both to return the same rows. `05053` expected `NUMBER_OF_COLUMNS_DOESNT_MATCH` for two ALIAS columns over the same expression with the marker off. `buildShardCollapseFanOut` from ClickHouse#107913 reconstructs the column the shard deduplicated away whether or not a marker is present, so that variant returns the same single row as the other six. `05060` expected `UNKNOWN_IDENTIFIER` from a Hybrid/MergeTree/Iceberg join under `object_storage_cluster_join_mode = 'local'`. It now returns the same rows as the `'allow'` variant beside it, and is held to them. Each reference was taken from a sibling query in the same test that must produce identical output, so a wrong reference here would mean the test was already wrong. `05060` also gained a header note: it needs the stateless S3 mock on localhost:11111, it needs a server listening on `127.0.0.3` for `test_cluster_one_shard_three_replicas_localhost`, and it needs `enable_parallel_blocks_marshalling = 0` until the `DISTINCT` over `ColumnBLOB` abort is fixed. Related: #1844 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A review of the three preceding commits turned up four things worth
changing.
`finalizeAliasMarkersForDistributedSerialization` refused to descend into
a lambda body, but the inliner has no such limit: `inlineExpression`
recurses through every child, so `arrayMap(x -> x + computed, dist)`
wrapped `computed` in a marker that nothing then materialized. Its id
stayed a `ColumnNode` and was rendered straight back into the shipped SQL
as `__table1.computed`, leaving the shard to resolve the very `ALIAS`
column the inlining exists to remove. Where the column is declared on the
`Distributed` table and not on the local one, the shard cannot.
The guard is gone. The case it was written for -- a hand-written
`__aliasMarker(x, x)` inside a lambda, whose id is the lambda parameter
-- is now handled by checking the id's column source instead. A lambda
parameter's source is the `LambdaNode`, which `createUniqueAliasesIfNecessary`
never aliases, so it is recognisable without refusing to look.
`makeConvertingActionsPreferNameThenPosition` is deleted. Its only call
site builds `converted_columns[i]` out of `current_step_columns[i]`, and
every branch of that loop either copies the source name through or --
in the one branch reachable only when the source name is absent from
`header` -- substitutes a name that cannot then appear anywhere in the
result set. So either every name matches its own position, in which case
`MatchColumnsMode::Name` and `::Position` pair the same columns, or the
two name sets differ and the helper falls back to position anyway. The
column swap in `05057` is fixed by emitting alias outputs under analyzer
identifiers, not by this. `src/Planner/Utils.{h,cpp}` are back to their
previous contents.
Finalization moved from `buildQueryTreeDistributed` into the end of
`buildQueryTreeForShard`. Four call sites build a shard tree and only one
finalized it; nothing is broken today, because only the `Distributed`
path injects markers at all, but wiring a second injection site would
have shipped markers nobody materializes. Last in the function rather
than straight after `createUniqueAliasesIfNecessary`, so a long id still
passes `ReplaceLongConstWithScalarVisitor` as a `ColumnNode` rather than
as a `String` that visitor could replace with a scalar.
The plain-name to analyzer-identifier mapping in
`ReadFromMerge::convertAndFilterSourceStream` now takes the longest
matching declared column name rather than the first. This is not a bug
fix: reintroducing the first-match behaviour leaves `05065` passing,
because an analyzer identifier backquotes a dotted name and the child
stream never emits a dotted identifier. It removes the mapping's
dependence on an invariant enforced two layers away.
Two review findings were not acted on. An assertion for an unfinalized
marker id cannot be written: nothing at the planner can tell an injected
marker that missed its boundary from one a user typed, and `03933` runs
exactly that shape on purpose. And `NormalizeAliasMarkerVisitor` does not
discard nested marker chains -- it never descends into a marker's own
arguments and collapses one only when the payload is itself a marker, so
the only id it drops names an intermediate action node. Both are recorded
as comments where a future reader will look.
Upstream ClickHouse#107700 also calls
`inlineAliasColumns` from `ClusterProxy/executeQuery.cpp` and
`findParallelReplicasQuery.cpp`, fixing `NO_SUCH_COLUMN_IN_TABLE` for an
`ALIAS` column on the shipped side of a parallel-replicas query. That bug
is real on this branch's base and predates this work. It is left alone:
this `inlineAliasColumns` injects markers when `enable_alias_marker` is
on, which is the default, so wiring those sites would put markers on two
paths that have never carried one.
New tests: `05064_alias_marker_in_lambda_over_distributed` covers the
lambda case, and `05065_shadowed_dotted_alias_merge_over_distributed`
guards the values for a `Merge` schema declaring both `b` and `` `a.b` ``.
Related: #1844
Related: ClickHouse#107700
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A `GLOBAL JOIN` over a `Distributed` table whose `ALIAS` column is not
declared on the local table failed on the shard:
Identifier '__table1.foo' cannot be resolved from table with name __table1.
In scope SELECT __table1.x AS x, __table1.foo AS foo, __table1.id AS id
FROM t_right_local AS __table1
Nothing in that query references `foo` any more -- inlining replaced it
with its defining expression. What still names it is the marker's id,
which became a live `ColumnNode` so that `createUniqueAliasesIfNecessary`
could renumber it. `CollectColumnSourceToColumnsVisitor` records every
`ColumnNode` it meets, keyed by source, keeping only the name and type,
so the id was gathered as if it were a column the query reads.
Those gathered columns become the projection of the subquery
`getSubqueryFromTableExpression` builds for the shipped temporary table,
rebuilt as bare column nodes with no expression. The alias body is lost
on the way, so the shard is asked for a column its storage does not
declare. `GLOBAL IN` on a table is unaffected -- it reads the storage's
ordinary columns instead of the gathered map.
Skip the marker's second argument when collecting. Traversal is top-down,
so the marker is always seen before the argument it records.
Reachable through `GLOBAL JOIN` and through a `CROSS JOIN` under
`find_cross_join`, and through a plain `JOIN` once
`prefer_global_in_and_join` promotes it.
`05066_global_join_alias_only_on_distributed` covers the alias in the
projection and the alias referenced only in a clause, each with the
marker on and off, plus the promoted plain `JOIN`. It fails on the
previous commit with the error above and passes here.
Related: #1844
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both were argued from cases that cannot occur, and both sit in a path every `Merge` table with child `ALIAS` columns goes through. `use_column_identifier_as_action_node_name` goes back to `false`. It was set to `true` so that an `ALIAS` defined over another one would resolve against the child stream rather than be expanded again, because `false` makes `PlannerActionsVisitor` replace a column node that has an expression with a visit of that expression. No such column node reaches it. `ApplyAliasColumnExpressionsVisitor` runs over each alias before it is recorded, replacing every column node that has an expression with the expression itself, and the visitor descends into what it substituted -- so `b ALIAS a + 1` over `a ALIAS x + 1` is already flattened to physical columns by the time `convertAndFilterSourceStream` re-analyzes it. The naming half of the setting is inert too: no column identifiers are registered for that table expression, so the action node name falls back to the plain column name whichever value is passed. The plain-name to identifier mapping goes back to taking the first matching declared column instead of the longest. The longest match was meant to stop a table declaring both `b` and `` `a.b` `` from letting `b` claim `__table1.a.b`. Nothing produces that name: `buildColumnIdentifier` always backquotes a dotted column name, so the identifier is ``__table1.`a.b` `` and does not end in `.b`. Reintroducing the first-match behaviour leaves `05065` passing, which is what settled it. The clause accepting an unquoted dotted name stays, now described as defensive rather than as something a branch emits. `05065` stays as a values guard for a schema shape nothing else covers, with its header cut down to say that and nothing more. Everything else from the `Merge` commit stands: the plain-name aliasing loop, the identifier-named alias outputs and the declared-schema mapping are the actual fix, and `04281`, `05057` and `05059` cover them. Related: #1844 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`__aliasMarker` stopped requiring a `String` second argument when the id became a live `ColumnNode`, but its registration still declared one. #1844 made the same relaxation and edited only the one-line description, and this branch inherited that, so the documented contract has been wrong since the marker gained its second phase. The argument is an identity rather than a name, so call it `alias_id`, and say what it is for: recording which `ALIAS` column an expression was inlined from, so that a shard names the result the way the initiator expects. Direct use is still not intended, but it is harmless -- the function returns its first argument unchanged. `03648_alias_marker_with_mergeable_state.sh` gains a note on why every invocation passes `--query_kind secondary_query`. The test prints the header a shard leg produces, and a query marked as secondary skips AST-level optimizations and splits aggregation differently, neither of which a real shard leg would have applied. It came from #1844 unchanged and nothing recorded the reason. Related: #1844 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mkmkme
marked this pull request as draft
September 19, 2026 13:38
Collaborator
Author
|
Converted to draft. This PR needs a backport from the upstream to cover another issue. I'll create the backport separately. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the attempt of forward-porting #1844 to 26.6 considering the improvements done in the upstream.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fixed
ALIAScolumns read throughDistributed,MergeoverDistributed, and Hybrid tables. AJOINwhose two sides declare a same-namedALIAScolumn failed withMULTIPLE_EXPRESSIONS_FOR_ALIAS; reading anALIAScolumn of aMergetable over aDistributedchild failed withNOT_FOUND_COLUMN_IN_BLOCKor returned defaults; reading anALIAScolumn of a Hybrid table from inside a subquery failed withTHERE_IS_NO_COLUMN; and anALIAScolumn referenced from inside a lambda over aDistributedtable failed on the shard.Documentation entry for user-facing changes
...
CI/CD Options
Exclude tests:
Regression jobs to run: