Repository navigation
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 317db7ca6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case ObjectStorageClusterJoinMode::LOCAL: // Legacy mode, equal to 'allow' | ||
| return; |
There was a problem hiding this comment.
Preserve initiator-local joins for legacy local mode
When local is selected for an object-storage cluster query that joins a table existing only on the initiator—the scenario in tests/integration/test_s3_cluster/test.py::test_joins—returning here leaves the join in query_to_send, which is dispatched to every shard; for example, s0_1_0 cannot resolve the test's join_table. The previous LOCAL path stripped the join and forced FetchColumns so it ran on the initiator, whereas the new ALLOW behavior requests WithMergeableState; therefore existing local queries and the integration test fail rather than retaining compatible behavior.
Useful? React with 👍 / 👎.
| - `local` — Replaces the database and table in the subquery with local ones for the destination server (shard), leaving the normal `IN`/`JOIN.` | ||
| - `global` — Replaces the `IN`/`JOIN` query with `GLOBAL IN`/`GLOBAL JOIN.` Right table executes first and is added to the secondary query as temporay table. | ||
| - `allow` — Default value. Allows the use of these types of subqueries. | ||
| - `local` — deprecated, legacy mode. The same as 'allow'. |
There was a problem hiding this comment.
Format the allow setting value as inline code
Replace the prose quotation 'allow' with the inline-code form `allow`. This is a literal ClickHouse setting value, and the repository requires such literals in documentation and comments to be wrapped in inline code.
AGENTS.md reference: AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
…al tables Read the left object-storage table on cluster nodes and run `JOIN` / local `IN` on the initiator, including with `allow_experimental_analyzer=0`, so the right table does not need to exist on remote nodes. Keep `local` as a legacy alias of `allow`. Co-authored-by: Cursor <cursoragent@cursor.com>
…uster` The previous initiator-local rewrite duplicated planner/`removeJoin` logic. Call `extractLeftTableExpression`, `buildQueryToReadColumnsFromTableExpression`, and `removeJoin` instead. Co-authored-by: Cursor <cursoragent@cursor.com>
`GLOBAL IN` becomes `ASTTableIdentifier` `_subqueryN`, which `as<ASTIdentifier>` does not match, so remotes still received `GLOBAL IN (_subqueryN)` with analyzer disabled. Co-authored-by: Cursor <cursoragent@cursor.com>
Rewrite JOIN/IN to `GLOBAL JOIN`/`GLOBAL IN` in the old interpreter so `GlobalSubqueriesVisitor` can broadcast right tables, and send those external tables with the remote query. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f10d74d7e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| && (rhs->as<ASTSubquery>() || rhs->as<ASTSelectQuery>() || rhs->as<ASTSelectWithUnionQuery>() | ||
| || rhs->as<ASTIdentifier>() || rhs->as<ASTTableIdentifier>())) |
There was a problem hiding this comment.
Do not globalize expression aliases used as IN sets
When allow_experimental_analyzer=0 and object_storage_cluster_join_mode='global', a valid constant-set alias such as WITH (1, 2) AS vals ... WHERE key IN vals remains an ASTIdentifier; this branch converts it to globalIn, but GlobalSubqueriesMatcher::Data::addExternalStorage accepts only an ASTTableIdentifier, ASTSubquery, or ASTTableExpression, so analysis throws WRONG_GLOBAL_SUBQUERY instead of evaluating the set. Restrict this rewrite to actual table/subquery operands or resolve expression aliases first.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This reproduction does not fail. rewriteASTInFunctionsToGlobalIn runs before TreeRewriter. For WITH (1, 2) AS vals ... WHERE key IN vals, the RHS is still an ASTIdentifier at that point, so it is rewritten to globalIn. Then QueryNormalizer inlines the WITH alias to the constant tuple, and GlobalSubqueriesVisitor sees a literal/function RHS and converts globalIn back to in. addExternalStorage is not invoked, so there is no WRONG_GLOBAL_SUBQUERY.
The suggested fix — skip ASTIdentifier and only rewrite table/subquery operands — would be incorrect here. At rewrite time, IN join_table is also an ASTIdentifier; MarkTableIdentifiersVisitor has not run yet. Dropping that case would skip the rewrite that object_storage_cluster_join_mode=global needs for initiator-local tables.
No code change for this comment.
…torage_cluster_join_mode_local
CI triage for #2228Verdict: 0 PR-caused failures. Three reds on this run — the Head SHA analyzed: 1.
|
|
High: With the old analyzer,
|
|
`removeJoin` drops ORDER BY and GROUP BY but leaves the ALL flags set. With the old analyzer, re-analysis of that query null-dereferences `orderBy` for `ORDER BY ALL` and aggregates `GROUP BY ALL` on the shards before the initiator join. https: //github.com//pull/2228 Co-authored-by: Cursor <cursoragent@cursor.com>
…torage_cluster_join_mode_local Keep the initiator-local join rewrite, and still drop wrap predicates that are unsafe to run on remote nodes. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Crash should be fixed |
|
Yep the crash is fixed: |
…torage_cluster_join_mode_local
|
Port on |
Co-authored-by: Cursor <cursoragent@cursor.com>
…al join. Co-authored-by: Cursor <cursoragent@cursor.com>
…FULL` joins. Co-authored-by: Cursor <cursoragent@cursor.com>
…ps the clause. Co-authored-by: Cursor <cursoragent@cursor.com>
…zer. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a45e64fc0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ++i; | ||
| } | ||
| } | ||
| if (!removeJoin(*select_query, rewriter_result, context)) |
There was a problem hiding this comment.
Clear DISTINCT before dispatching the raw scan
With the old analyzer, an initiator-local join such as SELECT DISTINCT count() FROM s3Cluster(...) JOIN local_dim ... reaches this new removeJoin path. removeJoin replaces the projection with the required left-side columns but preserves ASTSelectQuery::distinct, so each shard deduplicates those source rows before returning them and the initiator computes the aggregate from fewer rows, producing an incorrect count. Reset distinct on the stripped query so the remote FetchColumns stage returns the raw input rows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
No code change for this comment. removeJoin leaves select_query->distinct set, and the shard still returns every source row.
An initiator-local join is requested at FetchColumns. InterpreterSelectQuery calls executeDistinct only when to_stage > FetchColumns. The initiator applies DISTINCT to the joined result, so SELECT DISTINCT count() FROM s3Cluster(...) JOIN local_dim is not computed from a deduplicated scan.
| select_query->setExpression(ASTSelectQuery::Expression::LIMIT_BY, {}); | ||
| select_query->setExpression(ASTSelectQuery::Expression::LIMIT_OFFSET, {}); | ||
| select_query->setExpression(ASTSelectQuery::Expression::LIMIT_LENGTH, {}); |
There was a problem hiding this comment.
Reset LIMIT flags when removing LIMIT clauses
For an old-analyzer query without an explicit join but with an initiator-local IN subquery and ORDER BY ... LIMIT ... WITH TIES, the no-join rewrite removes ORDER BY and these lines remove the LIMIT nodes, but limit_with_ties remains true. Reanalysis in IStorageCluster::read then throws LIMIT WITH TIES without ORDER BY; similarly, LIMIT ... BY ALL leaves limit_by_all set after its clause is removed. Reset both modifier flags together with the corresponding expressions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
No code change for this comment. The non-join rewrite leaves limit_with_ties and limit_by_all set after it removes ORDER BY and LIMIT. removeJoin already clears both flags on the join path. Neither failure described here is reached.
LIMIT WITH TIES without ORDER BY is thrown from executeLimit, inside the same to_stage > FetchColumns block. Formatting prints WITH TIES only when limitLength is present, so the query text sent to the shard does not contain it.
LIMIT BY ALL is rejected by TreeRewriter for every old-analyzer query, including the original one, before this rewrite runs. A query that reaches the strip does not have that flag set.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Remove object_storage_cluster_join_mode=local
Solved #1827
Improved default 'allow' behavior to work in swarm mode (when remote nodes do not have knowledge about tables).
Improved 'global' mode to work with old analyzer.
Documentation entry for user-facing changes
'local' is a legacy setting, behavior is the same as 'allow' after changes in upstream.
CI/CD Options
Exclude tests:
Regression jobs to run: