Fix view returning wrong results after DETACH/ATTACH with UNION and INTERSECT#100390
Fix view returning wrong results after DETACH/ATTACH with UNION and INTERSECT#100390alexey-milovidov merged 21 commits intomasterfrom
Conversation
…NTERSECT When a view definition contains mixed `UNION` and `INTERSECT` operators, the `INTERSECT` precedence was lost after `DETACH`/`ATTACH`. This happened because `NormalizeSelectWithUnionQueryVisitor` was called without first running `SelectIntersectExceptQueryVisitor` to resolve `INTERSECT`/`EXCEPT` precedence. The normalizer doesn't understand `INTERSECT`/`EXCEPT` modes and would incorrectly drop SELECT branches connected by these operators. The fix adds `SelectIntersectExceptQueryVisitor` before every call to `NormalizeSelectWithUnionQueryVisitor` in all affected code paths: `StorageView`, `DatabaseOrdinary`, `DatabaseReplicated`, `DatabaseBackup`, `InterpreterSystemQuery`, and `UserDefinedSQLFunctionFactory`. Fixes #99257 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Workflow [PR], commit [5e5e362] Summary: ✅ AI ReviewSummaryThis PR fixes incorrect view semantics after ClickHouse Rules
Final VerdictStatus: ✅ Approve |
clickhouse-gh
Bot
added
the
pr-bugfix
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
| /// This is needed when the AST is freshly parsed from stored metadata | ||
| /// (e.g. during ATTACH) and has not been through executeQuery's visitors. | ||
| { | ||
| SelectIntersectExceptQueryVisitor::Data data{SetOperationMode::DISTINCT, SetOperationMode::DISTINCT}; |
There was a problem hiding this comment.
This hardcodes INTERSECT/EXCEPT defaults to DISTINCT, which can change semantics for views created or attached when intersect_default_mode / except_default_mode are ALL.
The other touched code paths read these defaults from settings; this one should do the same (or preserve already-resolved operators only) to avoid behavior drift after DETACH/ATTACH.
…ctor, extend tests The `StorageView` constructor was calling `SelectIntersectExceptQueryVisitor` on the inner query AST, but this is incorrect for code paths where the AST has already been normalized (e.g., `view()` table function). The visitor expects `modes.size() + 1 == selects.size()`, which does not hold after `NormalizeSelectWithUnionQueryVisitor` has flattened nested unions. This caused a `LOGICAL_ERROR` exception (and server abort with `abort_on_logical_error` enabled) when running queries like `SELECT ... FROM view(SELECT 1 UNION ALL (SELECT 2 UNION ALL SELECT 3))`. The visitor call in `StorageView` is unnecessary because: - For `CREATE VIEW` via `executeQuery`: the AST is already processed by `SelectIntersectExceptQueryVisitor` in the `executeQuery` pipeline. - For `ATTACH` from stored metadata: the Database code paths (`DatabaseOrdinary`, `DatabaseReplicated`, `DatabaseBackup`) already apply the visitor with proper settings before constructing `StorageView`. Also address review feedback: - Remove hardcoded `SetOperationMode::DISTINCT` that ignored `intersect_default_mode` / `except_default_mode` settings. - Add test cases for `EXCEPT` operator. - Add test cases with `intersect_default_mode = 'ALL'` and `except_default_mode = 'ALL'` to verify settings-dependent behavior is preserved after `DETACH`/`ATTACH`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| SET intersect_default_mode = 'ALL'; | ||
| SET except_default_mode = 'ALL'; | ||
|
|
||
| CREATE VIEW v2 AS (SELECT 1 c0, 1 c1 UNION DISTINCT SELECT 2, 2 INTERSECT SELECT 3, 3); |
There was a problem hiding this comment.
These ALL-mode checks are not actually sensitive to ALL vs DISTINCT semantics: both INTERSECT and EXCEPT inputs here contain no duplicates, so SET intersect_default_mode = 'ALL' / SET except_default_mode = 'ALL' produces the same result as DISTINCT.
As written, this can still pass even if mode preservation regresses during DETACH/ATTACH.
Please add cases with duplicates so result multiplicity differs between ALL and DISTINCT, for example:
INTERSECT:SELECT number % 2 FROM numbers(6) INTERSECT SELECT number % 2 FROM numbers(4)EXCEPT:SELECT number % 2 FROM numbers(6) EXCEPT SELECT number % 2 FROM numbers(3)
and verify before/after DETACH/ATTACH under ALL defaults.
…ory, improve tests The previous commit removed `SelectIntersectExceptQueryVisitor` from the `StorageView` constructor, but this broke the `ATTACH TABLE` path because the Database code paths that apply the visitor only run during server startup (`loadTablesMetadata`), not during explicit `ATTACH TABLE` which goes through `InterpreterCreateQuery` → `StorageFactory`. Move the visitor call to `registerStorageView` (the factory function for View storage) where we have access to context settings. This: - Fixes the `ATTACH TABLE` path: the freshly-parsed AST gets INTERSECT/EXCEPT precedence resolved before `NormalizeSelectWithUnionQueryVisitor` runs. - Is a safe no-op for `CREATE VIEW` via `executeQuery`: the AST has already been processed and contains no INTERSECT/EXCEPT modes in `list_of_modes`. - Does not affect the `view()` table function: it creates `StorageView` directly, bypassing the factory. - Uses `intersect_default_mode` / `except_default_mode` settings from context instead of hardcoding `SetOperationMode::DISTINCT`. Also improve ALL-mode tests (v2, v3) to use data with duplicates so that `INTERSECT ALL` / `EXCEPT ALL` actually produce different results from their `DISTINCT` counterparts, making the tests sensitive to the setting being correctly propagated. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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. |
|
The test |
|
The |
LLVM Coverage Report
Changed lines: 80.43% (37/46) · Uncovered code |
alexey-milovidov
left a comment
alexey-milovidov
left a comment
There was a problem hiding this comment.
Looks alright.
robot-ch-test-poll3
added
the
pr-synced-to-cloud
|
Hi — this PR may need backporting to Affected code: Why: This bug causes silent wrong results for views with INTERSECT/EXCEPT after DETACH/ATTACH or server restart. Both If this should be backported, consider adding |
3 participants
When a view definition contains mixed
UNIONandINTERSECToperators (e.g.SELECT 1 UNION DISTINCT SELECT 2 INTERSECT SELECT 3), theINTERSECTprecedence was lost afterDETACH/ATTACHor server restart. This happened becauseNormalizeSelectWithUnionQueryVisitorwas called without first runningSelectIntersectExceptQueryVisitorto resolveINTERSECT/EXCEPTprecedence. The normalizer doesn't understandINTERSECT/EXCEPTmodes and would incorrectly drop SELECT branches connected by these operators.The fix adds
SelectIntersectExceptQueryVisitorbefore every call toNormalizeSelectWithUnionQueryVisitorin all affected code paths:StorageView,DatabaseOrdinary,DatabaseReplicated,DatabaseBackup,InterpreterSystemQuery, andUserDefinedSQLFunctionFactory.Fixes #99257
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fix views with mixed
UNIONandINTERSECT/EXCEPToperators returning wrong results afterDETACH/ATTACHor server restart.Documentation entry for user-facing changes