Fix Column identifier is already registered with additional_result_filter in UNION/EXCEPT#101051
Conversation
…_filter` in UNION/EXCEPT When multiple UNION branches share the same `GlobalPlannerContext` and each applies `additional_result_filter`, the `addAdditionalFilterStepIfNeeded` function creates a fake `TableNode` without an alias for each branch. Both branches try to register the same bare column identifier (e.g. "a") in the shared context, causing the second to throw a LOGICAL_ERROR. Fix by giving each fake table expression a unique alias so that column identifiers become unique across branches (e.g. "_additional_result_filter_0.a" and "_additional_result_filter_1.a"). Fixes #99931 https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=100947&sha=d0c5ef2315e4d001a6d34cf2e54a9ee2b176a538&name_0=PR&name_1=Stress%20test%20%28amd_debug%29 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
clickhouse-gh
Bot
added
the
pr-bugfix
…er-additional-result-filter
…er-additional-result-filter
…er-additional-result-filter
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
| /// When multiple UNION branches share the same GlobalPlannerContext and each applies additional_result_filter, | ||
| /// the bare column names (e.g. "a") would collide. Give the fake table expression a unique alias | ||
| /// so that identifiers become unique (e.g. "_additional_result_filter_0.a"). | ||
| auto unique_alias = "_additional_result_filter_" + std::to_string(planner_context->getGlobalPlannerContext()->nextUniqueId()); |
There was a problem hiding this comment.
_additional_result_filter_0 and a projected column a, GlobalPlannerContext::createColumnIdentifier can still throw Column identifier ... is already registered for _additional_result_filter_0.a.
Please make the synthetic alias collision-resistant against existing identifiers in the shared context, e.g. loop over generated aliases until alias + "." + projection_column does not exist in GlobalPlannerContext (or use a truly unique suffix that cannot appear in user aliases).
|
The Stress test (arm_msan) failure is fixed by #101239, which should be merged first. After it is merged, please update the branch to include the fix. |
…er-additional-result-filter
The synthetic alias `_additional_result_filter_N` used in `addAdditionalFilterStepIfNeeded` was deterministic and could collide with a real user alias. Now loop over candidate aliases, checking each against existing column identifiers in `GlobalPlannerContext`, until a collision-free one is found. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…er-additional-result-filter
…er-additional-result-filter
| CREATE TABLE t_04064 (x UInt64) ENGINE = MergeTree ORDER BY x; | ||
| INSERT INTO t_04064 VALUES (1), (2), (3); | ||
|
|
||
| SELECT * FROM (SELECT x AS a FROM t_04064) |
There was a problem hiding this comment.
UNION/EXCEPT with regular aliases, but it does not exercise the collision scenario fixed in the second commit (user alias like _additional_result_filter_0).
Please add a case where a branch explicitly uses _additional_result_filter_0 (or similar) as a table/query alias and verifies additional_result_filter works without Column identifier ... is already registered exception.
LLVM Coverage Report
Changed lines: 83.33% (15/18) · Uncovered code |
alexey-milovidov
deleted the
fix-column-identifier-additional-result-filter
branch
The original "Column identifier is already registered" exception is fixed by other PRs that landed on master: - #101048 ("Fix exception 'Column identifier is already registered' in planner") - #101051 ("Fix Column identifier is already registered with additional_result_filter in UNION/EXCEPT") Per the author's review comment on this PR, drop the disambiguation workaround in `GlobalPlannerContext::createColumnIdentifier` and keep only the regression test from this PR as a guard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 participants
When multiple UNION/EXCEPT branches share the same
GlobalPlannerContextand each appliesadditional_result_filter, theaddAdditionalFilterStepIfNeededfunction creates a fakeTableNodewithout an alias for each branch. Both branches try to register the same bare column identifier (e.g.a) in the shared context, causing the second to throw a LOGICAL_ERROR exception.Fix by giving each fake table expression a unique alias so that column identifiers become unique across branches.
Fixes #99931
CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=100947&sha=d0c5ef2315e4d001a6d34cf2e54a9ee2b176a538&name_0=PR&name_1=Stress%20test%20%28amd_debug%29
#100947
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fix LOGICAL_ERROR exception "Column identifier is already registered" when
additional_result_filtersetting is used with UNION or EXCEPT queries.Documentation entry for user-facing changes
Version info
26.5.1.81