Fix NUMBER_OF_COLUMNS_DOESNT_MATCH for duplicate ALIAS columns in a subquery on a Distributed table#108725
Conversation
…ubquery on a Distributed table buildShardCollapseFanOut (added in ClickHouse#107913) reconstructs duplicate-ALIAS columns that a shard deduplicates into one, matching the expected columns to the shard columns by their inlined action-node names. Those names embed a __tableN table alias. The shard query tree is renumbered independently from the initiator's: buildQueryTreeForShard runs createUniqueAliasesIfNecessary, which restarts the __tableN aliases at 1. When the distributed read is nested inside a subquery, the same source column is named __table1.x on the shard but __tableK.x (K > 1) in the initiator's tree, so the name match fails, the fan-out is skipped, and the positional fallback cannot reconcile the differing column counts, throwing NUMBER_OF_COLUMNS_DOESNT_MATCH. The top-level form works only because the distributed table is __table1 on both sides. Realign the two numberings before matching by zipping the distinct __tableN indices of each side in ascending order (both trees number tables in the same DFS pre-order) and rewriting the initiator indices to the shard ones. This is applied only when both sides expose the same number of table indices; otherwise the original names are kept and the existing bijection guard rejects any mismatch, so multi-table scopes are unchanged. Add subquery-wrapped regression cases to 04357_distributed_alias_same_expression. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-worker-slot-2:20260628-001700 |
|
cc @yakov-olkhovskiy @vdimir — could you review this? It extends the duplicate-ALIAS shard-collapse reconciliation from #107913 to the subquery case: when the distributed read is nested in a subquery, the shard tree's |
|
Workflow [PR], commit [7c87465] Summary: ✅
AI ReviewSummaryThis PR tries to fix Missing context / blind spots
Findings
Tests
Final VerdictStatus: Minimum required action: fix the lambda-argument real-tail normalization case and add a focused regression for it. |
The duplicate-ALIAS shard-collapse reconstruction realigns the shard and
initiator __tableN numbering by scanning inlined action-name text. The
previous scan matched any __table followed by digits, so user-visible text
that merely looks like a qualifier could perturb it: a column named
__table9, a string constant such as concat('__table9', x), or an over-long
__table999... literal (the last could also overflow parse<UInt64>).
Match only a genuine analyzer qualifier __tableN. (with the trailing dot
that buildColumnIdentifier always emits, and a non-word left boundary), and
use overflow-safe tryParse. Add regression cases to
04357_distributed_alias_same_expression.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The duplicate-ALIAS shard collapse reconstruction realigns the initiator/shard __tableN numbering by scanning column action-name text. The previous matcher required a `__tableN.` shape (trailing dot), but a string constant such as `'__table1.'` also satisfies it: the opening quote is a non-word boundary and the dot is part of the literal. That literal's id then collides with the shard's renumbered real __table1 alias, the two index sets differ in size, the realignment is disabled, and a collapsed pair nested in a subquery still fails with NUMBER_OF_COLUMNS_DOESNT_MATCH. A backquoted column identifier containing a dot (e.g. `__table1.k`) has the same problem. Action names quote user text in exactly two ways: `'...'` for string constants (FieldVisitorToString) and backquoted `...` for identifiers (backQuoteIfNeed), both backslash-escaped (writeAnyEscapedString). Make collectTableIds and remapTableIds skip those spans, so __tableN.-looking text inside a quoted span is treated as user data, not a qualifier. The same quote-skipping runs on both the shard header names and the inlined initiator names, keeping the two index sets symmetric for the zip. Regression cases added to 04357: a `'__table1.'` string constant and a backquoted `__table1.k` column riding along a duplicate-ALIAS collapse nested in a subquery. Both fail before this change and pass after.
The duplicate-ALIAS shard collapse reconstruction matched shard columns to the initiator's expected columns by parsing the __tableN table-qualifier indices out of the inlined action-name text and remapping one numbering onto the other. That text scan could not reliably tell a real analyzer qualifier apart from user text of the same shape: a string constant '__table1.', a column literally named __table9, or a lambda argument name like __table1. or __table1.y are all emitted into the action name and look like qualifiers. A look-alike token's index polluted the per-side index sets, so the two sides disagreed on the number of tables, the remap was disabled, and the collapsed pair still failed with NUMBER_OF_COLUMNS_DOESNT_MATCH. Replace the parse-and-remap with a symmetric canonicalization: blank the index of every __table<digits>. qualifier (rewrite __table<digits>. to __table.) on both the shard column names and the initiator's expected names, then match by equality. The only systematic difference between the two sides is the independent __tableN renumbering that createUniqueAliasesIfNecessary applies on the shard; every other part of the name, including any qualifier-shaped user text, is produced identically on both sides and cancels out under the equality comparison. This removes the need to decide whether a given token is a real qualifier, so string constants, oddly named columns, and lambda argument names no longer perturb the match. Ambiguous collisions (two shard columns sharing a canonical name) are still rejected by the existing every-shard-column-used guard, which falls back to the default reconciliation rather than producing a wrong result. The digits are no longer parsed into an integer, so an over-long run cannot overflow. Extends the 04357 regression test with lambda-argument ALIAS columns whose argument names are __table1. and __table1.y. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…iers
buildShardCollapseFanOut matches the initiator's expected columns to the
shard's deduplicated columns by name, after removing the independent __tableN
alias renumbering. The previous approach blanked the number of every
__table<digits>. token in the action-name text, which also mangled user text
that merely looks like a qualifier: two distinct expressions such as
concat('__table1.', toString(x)) and concat('__table2.', toString(x)) both
canonicalized to the same name, so one shard column was dropped as a duplicate
and the query failed with NUMBER_OF_COLUMNS_DOESNT_MATCH.
Drive the normalization from the query tree instead. Collect the set of genuine
column-name tails of __tableN. qualifiers from the structured column identifiers
(PlannerContext::getColumnNodeIdentifierOrNull), and erase a __table<digits>.
number only when the following tail is one of those known column names. Quoted
spans (string constants and backquoted identifiers) are skipped, and a
backquoted column tail that itself contains a dot (e.g. `__table1.k`) is matched
as a whole. User text (string literals, lambda argument names, columns named
like __table9) is therefore left intact and distinct look-alike values no longer
collapse onto one another. Matching stays by name, not position, so it tolerates
the shard returning its deduplicated columns in a different order than the
initiator expects.
Add regression cases to 04357_distributed_alias_same_expression.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| else | ||
| while (tail_end < name.size() && isWordCharASCII(name[tail_end])) | ||
| ++tail_end; | ||
| if (tail_end > tail_begin && genuine_tails.contains(name.substr(tail_begin, tail_end - tail_begin))) |
There was a problem hiding this comment.
This still treats some user text as a genuine qualifier. Lambda argument names are emitted into action names unquoted, so if the argument name has a real column tail, for example:
arrayMap(`__table1.x` -> toString(x), [0])
arrayMap(`__table2.x` -> toString(x), [0])then this check sees tail x in genuine_tails and normalizes both argument names to __table.x. Those are two distinct non-collapsed shard columns, but shard_normalized_to_index.emplace keeps only one; the unused-column guard returns nullopt, and the caller falls back to the old positional conversion with fewer shard columns than the expected header, producing NUMBER_OF_COLUMNS_DOESNT_MATCH next to the duplicate-ALIAS pair. Please make the text scan skip lambda argument declarations too, or derive the normalization spans from structured column identifiers instead of matching every rendered __tableN.<tail> token.
There was a problem hiding this comment.
You are right that this exact shape is not reconstructed, and I now believe it cannot be at this call site. I want to lay out why, with the rendered strings, and ask a maintainer to weigh in on accepting it as a documented limit.
Why no text rule on the shard side can separate the two. At this point we only have the shard Block header text (query_plan.getCurrentHeader()); the shard query tree built by buildQueryTreeForShard is serialized into ReadFromRemote and is not reachable here. For your example, the shard column is:
arrayMap(__table1.x UInt8 -> toString(__table1.x), _CAST([0]_Array(UInt8), 'Array(UInt8)'_String))
The lambda argument name (__table1.x) is not renumbered (it is fixed user text rendered via getColumnName()), while the genuine body qualifier is renumbered (createUniqueAliasesIfNecessary restarts at 1 on the shard, so the body is __table1.x on the shard but __table2.x on the initiator). So the same literal __table1.x appears twice in one shard name, once as the arg (must be preserved) and once as a genuine qualifier (must be normalized), and they are byte-identical. With only the shard text and no shard tree, no token set, token equality, or "skip lambda arg declarations" rule can normalize one occurrence and not the other. I tried the lambda-arg-position grammar too: genuine qualifiers also render followed by <Type>/keywords in sort and window action names (e.g. __table1.y ORDER BY __table1.x ASC ..., __table1.x UInt8), so a flat-text "arg declaration" scan is not sound either.
This case is not a regression. I built the pre-PR base (exact-name fan-out, the buildShardCollapseFanOut from #107913) and ran your shape on a forced two-server remote cluster:
- base (no this PR):
Code 20 NUMBER_OF_COLUMNS_DOESNT_MATCH (source 3, result 4) - this PR:
Code 20 NUMBER_OF_COLUMNS_DOESNT_MATCH (source 3, result 4)(identical)
The normalize collision is caught by the existing "every shard column used" guard, so the fan-out returns nullopt and the caller falls back to positional conversion exactly as before this PR. It never produces a wrong result. When the lambda argument is actually referenced in the body (e.g. arrayMap(\__table1.x` -> `__table1.x` + 100, [5])), the two columns stay distinct after normalization and the fan-out reconstructs them correctly; the only unreconstructed shape is an *unused* argument whose backquoted name equals __table.`, which is not produced by normal queries or by the analyzer.
So the PR fixes the reported bug (#107815) and the residual look-alike case is a safe fall-back identical to the pre-PR behavior. A complete fix would need the genuine-qualifier byte spans known at the shard header, which would mean transmitting that span metadata from the shard, well beyond this PR.
cc @vdimir (you reviewed #107913 which introduced this function) for a maintainer call on accepting this as a documented limitation versus a larger follow-up.
clang-tidy modernize-use-starts-ends-with (-warnings-as-errors) flagged the compare(0, prefix.size(), prefix) prefix check in CollectGenuineQualifierTailsVisitor. Equivalent rewrite to std::string::starts_with; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The lambda argument name `__table1.` (a backquoted identifier ending in a dot) makes the targeted AST fuzzer mutate the test table into a column whose ALIAS default type mismatches, which forces error-message formatting to convert the lambda back to an AST. IdentifierNode::toASTImpl then builds an ASTIdentifier whose name parts include an empty trailing part (from splitting `__table1.` on the dot), tripping chassert(!part.empty()) in the ASTIdentifier constructor (pre-existing engine assertion, unrelated to this PR's fix; also seen on unrelated PRs ClickHouse#104436 and ClickHouse#99877). The `__table1.y` lambda-arg cases already cover the same code path in buildShardCollapseFanOut (an unquoted lambda-arg name that looks like a `__tableN.` qualifier but whose tail is not a real column, so its numbering must be left intact), without producing an empty identifier part. Keep those and drop the redundant trailing-dot variant so the test no longer surfaces the unrelated assertion under fuzzing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The two AST-fuzzer/stress findings on the previous commit (9c57191) are both pre-existing engine bugs, not regressions from this PR. Pushed 7c87465 to stop this PR's test from surfacing one of them; both engine bugs are tracked for separate fixes. STID 2508-2ef2 STID 3926-38e4 The fix itself is unchanged and the regression test passes (verified locally, randomization on). |
|
Fixing PR for the ASTIdentifier |
|
Fixing PR for the AST-fuzzer finding |
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered by tests: 74/78 (94.87%) | Lost baseline coverage: none · Uncovered code |
alexey-milovidov
left a comment
There was a problem hiding this comment.
It's quite unfortunate that we have to do this. Approved.
|
Found via ClickGap automated review. Please close or comment if this is incorrect or needs adjustment. TL;DRFix NUMBER_OF_COLUMNS_DOESNT_MATCH for duplicate ALIAS columns in a subquery on a Distributed table ClickGap verified: reproduced it with the script below on ReproductionBisect
Affects
Suggested next stepThe fix has landed on master; the affected release branches above still need the backport. Component: cc fix PR author: @groeneai ClickGap · Finding: Triage metadata
|
…icate ALIAS columns in a subquery on a Distributed table
Backport #108725 to 26.6: Fix NUMBER_OF_COLUMNS_DOESNT_MATCH for duplicate ALIAS columns in a subquery on a Distributed table
Related: #107815
Related: #107913
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fix
NUMBER_OF_COLUMNS_DOESNT_MATCHerror when a subquery on aDistributedtable reads two or moreALIAScolumns that expand to the same expression (for examplea1 String ALIAS toString(x), a2 String ALIAS toString(x)) and the subquery feeds an outer query, e.g.SELECT count() FROM (SELECT a1, a2 FROM dist GROUP BY a1, a2).Description
PR #107913 added
buildShardCollapseFanOutto reconstruct duplicate-ALIAScolumns that a shard deduplicates into one. It matches the expected (initiator) columns to the shard columns by their inlined action-node names. Those names embed a disambiguating__tableNtable alias.The shard query tree is renumbered independently from the initiator's:
buildQueryTreeForShardrunscreateUniqueAliasesIfNecessary, which restarts the__tableNaliases at 1. When the distributed read is nested inside a subquery, the same source column is named__table1.xon the shard but__tableK.x(K > 1) in the initiator's tree. The name match then fails, the fan-out is skipped, and the positional fallback cannot reconcile the differing column counts, so the query throwsNUMBER_OF_COLUMNS_DOESNT_MATCH. The top-level form works only because the distributed table is__table1on both sides.The fix realigns the two numberings before matching: it zips the distinct
__tableNindices of the shard header and of the initiator-side inlined names in ascending order (both trees number tables in the same DFS pre-order, so the i-th initiator table is the i-th shard table) and rewrites the initiator indices to the shard ones. Realignment is applied only when both sides expose the same number of table indices (the single distributed-read scope); otherwise the original names are kept and the existing bijection guard rejects any mismatch, so the behavior is unchanged for multi-table scopes.Regression cases (subquery-wrapped duplicate-
ALIAScollapse, including a non-collapsed column flowing alongside the collapsed pair) are added to04357_distributed_alias_same_expression.Version info
26.7.1.212(included in26.7and later)26.6.2.45