Repository navigation
Conversation
…ins. Frontport of #2228. `local` stays as a deprecated alias of `allow`. The right table is joined on the initiator, including with the old analyzer, and `global` broadcasts that table without requiring the analyzer. Co-authored-by: Cursor <cursoragent@cursor.com>
ianton-ru
added
antalya
port-antalya
|
@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. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 454ce3654d
ℹ️ 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".
| { | ||
| auto & tables_element = child->as<ASTTablesInSelectQueryElement &>(); | ||
| if (tables_element.table_join) | ||
| tables_element.table_join->as<ASTTableJoin &>().locality = JoinLocality::Global; |
There was a problem hiding this comment.
With allow_experimental_analyzer=0, object_storage_cluster_join_mode='global', and multiple object-storage workers, this unconditionally converts RIGHT and FULL JOIN to global joins. The complete right side is then broadcast, so each worker can independently emit its preserved unmatched rows, producing duplicate or spurious results. This also contradicts the documented contract and the analyzer path, which deliberately keep these join kinds on the initiator; restrict globalization to the supported INNER, LEFT, and CROSS kinds.
Useful? React with 👍 / 👎.
| if (!needsInitiatorLocalJoin(info)) | ||
| return; | ||
|
|
||
| rewriteQueryForInitiatorLocalJoin(query_to_send, query_info, info, context); |
There was a problem hiding this comment.
When the old analyzer uses allow/local together with object_storage_remote_initiator=1, this rewrites only query_to_send; readFromRemoteInitiator subsequently passes the original full query_info to StorageDistributed::read. That path replaces only the left table expression, leaving the JOIN and its right table in the SQL analyzed on the randomly selected remote initiator. A Memory right table that exists only on the client-facing node therefore causes UNKNOWN_TABLE whenever another host is selected, even though allow promises the right table need not exist remotely.
Useful? React with 👍 / 👎.
| TreeRewriterResult rewriter_result = *query_info.syntax_analyzer_result; | ||
| if (!removeJoin(*select_query, rewriter_result, context)) | ||
| throw Exception(ErrorCodes::LOGICAL_ERROR, "Failed to strip JOIN from query sent to cluster nodes"); |
There was a problem hiding this comment.
For an old-analyzer initiator-local RIGHT or FULL JOIN, removeJoin retains left-table-only WHERE predicates in the shard query, even though applying such a predicate before the join can change which right rows are unmatched. For example, with matching nullable keys and WHERE isNull(left.k), filtering the left row during object-storage pushdown makes the initiator null-extend the now-unmatched right row, after which the original predicate passes and returns a spurious row. Clear these predicates for preserved-right joins or apply the same join-side safety check used by the planner.
Useful? React with 👍 / 👎.
| select_query->setExpression(ASTSelectQuery::Expression::GROUP_BY, {}); | ||
| select_query->group_by_all = false; | ||
| select_query->setExpression(ASTSelectQuery::Expression::HAVING, {}); |
There was a problem hiding this comment.
In the old-analyzer path for an IN/WHERE subquery without an explicit JOIN, removing GROUP BY leaves group_by_with_totals, group_by_with_rollup, and group_by_with_cube set. A query such as ... WHERE k IN (SELECT ... FROM local_table) GROUP BY k WITH TOTALS is consequently serialized for workers as a SELECT with WITH TOTALS but no GROUP BY, causing parsing or analysis to fail instead of returning raw rows to the initiator. Reset the modifier flags here as removeJoin already does in the sibling branch.
Useful? React with 👍 / 👎.
|
|
||
| query_to_send = queryNodeToDistributedSelectQuery(modified_query_tree); | ||
| } | ||
| auto info = getQueryJoinInfo(query_info, context); |
There was a problem hiding this comment.
Under the analyzer, the decision reached here detects foreign sources only through their ColumnNodes, so a table-referencing subquery that projects no table column—such as WHERE value IN (SELECT 1 FROM initiator_only_dim LIMIT 1)—is not classified as initiator-local. The unstripped subquery is then sent to workers, which fail to resolve the initiator-only table, contrary to the newly documented handling of IN/WHERE subqueries. Detection needs to consider referenced table expressions as well as foreign columns.
Useful? React with 👍 / 👎.
ianton-ru
added
the
forwardport
`object_storage_cluster_join_mode = 'global'` was marking every join `GLOBAL`, so each worker emitted the preserved right rows again. Co-authored-by: Cursor <cursoragent@cursor.com>
…al join. `StorageDistributed::read` was still analyzing the original `query_info`, so the JOIN and its right table were planned on a host that does not have that table. Co-authored-by: Cursor <cursoragent@cursor.com>
…FULL` joins. `removeJoin` kept those predicates on the shard scan, which changes which right rows are unmatched before the initiator applies the original predicate. Co-authored-by: Cursor <cursoragent@cursor.com>
…ps the clause. A shard query with `WITH TOTALS`, `ROLLUP`, `CUBE`, or `GROUPING SETS` and no `GROUP BY` is rejected before the initiator can aggregate. Co-authored-by: Cursor <cursoragent@cursor.com>
…zer. `WHERE k IN (SELECT 1 FROM dim)` names `dim` without projecting a column, so workers were asked to resolve a table that exists only on the initiator. Co-authored-by: Cursor <cursoragent@cursor.com>
Fold foreign-table detection into the existing column visitor, use one `RIGHT`/`FULL` check, and drop a left predicate before the existing filter rewrite. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87c2d9a494
ℹ️ 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".
| query_node.getPrewhere() = {}; | ||
| query_node.getWhere() = {}; |
There was a problem hiding this comment.
When a RIGHT or FULL JOIN has a left-side PREWHERE, clearing it here removes the predicate from the shard query, but the initiator does not add an equivalent filter above ReadFromCluster because PREWHERE is delegated to the storage read. For example, ... PREWHERE left.k = 1 RIGHT JOIN dim ... reads all left rows, changing which right rows match and therefore returning incorrect results. Unlike a post-join WHERE, this predicate is defined to run before the join, so it must be preserved on the left scan or explicitly reapplied before the initiator join.
Useful? React with 👍 / 👎.
| select_query->setExpression(ASTSelectQuery::Expression::SELECT, std::move(select_expression_list)); | ||
| select_query->setExpression(ASTSelectQuery::Expression::PREWHERE, {}); | ||
| select_query->setExpression(ASTSelectQuery::Expression::WHERE, {}); |
There was a problem hiding this comment.
With the old analyzer and allow/local, a query such as FROM s3Cluster(...) ARRAY JOIN arr AS elem WHERE elem IN (SELECT ... FROM initiator_only_dim) enters this branch, which replaces the projection and filter but leaves the ARRAY JOIN in the tables list. The shards consequently expand each source row before returning it, and the original initiator plan expands it again; an array of length two can produce four rows instead of two. Retain only the source table expression when building this raw scan, as the analyzer path does.
Useful? React with 👍 / 👎.
| if (!removeJoin(*select_query, rewriter_result, context)) | ||
| throw Exception(ErrorCodes::LOGICAL_ERROR, "Failed to strip JOIN from query sent to cluster nodes"); |
There was a problem hiding this comment.
With the old analyzer, ASTSelectQuery::hasJoin detects a valid shape such as FROM s3Cluster(...) ARRAY JOIN arr INNER JOIN local_dim ..., but removeJoin only recognizes a join when the second table-list element is a normal join. Here the second element is the ARRAY JOIN, so removeJoin returns false and this newly supported initiator-local path throws LOGICAL_ERROR. The stripping logic needs to find later ordinary joins while reducing the worker query to the raw left scan.
Useful? React with 👍 / 👎.
| select_query->group_by_all = false; | ||
| select_query->order_by_all = false; |
There was a problem hiding this comment.
In the old-analyzer initiator-local path, removeJoin replaces the projection with required source columns but leaves select_query->distinct enabled. The shards can therefore discard duplicate source rows before the initiator performs the original join or aggregation; for example, SELECT DISTINCT sum(v) FROM s3Cluster(...) JOIN local_dim ... computes the sum from deduplicated shard input. Reset DISTINCT together with the aggregation and ordering modifiers when converting the query into a raw scan.
Useful? React with 👍 / 👎.
| auto & tables_element = child->as<ASTTablesInSelectQueryElement &>(); | ||
| if (tables_element.table_join) | ||
| tables_element.table_join->as<ASTTableJoin &>().locality = JoinLocality::Global; |
There was a problem hiding this comment.
With allow_experimental_analyzer=0, object_storage_cluster_join_mode='global', and multiple workers, this marks PASTE JOIN global even though the documented worker-side set is limited to INNER, LEFT, and CROSS. Each worker then pairs its left partition with the broadcast right table from the beginning; with a one-row right side and two nonempty workers this emits two rows instead of the single row produced by an initiator-side positional join. Treat PASTE like the preserved-side join kinds in both globalization and processing-stage selection.
Useful? React with 👍 / 👎.
…LL` join. 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: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
CI triage for
|
|
@blau-ai fix the PR-caused failing (and flaky) tests |
PREWHERE is parsed after the table/JOIN section, so the query `FROM ... AS t1 PREWHERE ... RIGHT JOIN ...` failed with SYNTAX_ERROR at the RIGHT keyword (every run of 05391 failed in CI). Reorder to `FROM ... AS t1 RIGHT JOIN ... ON ... PREWHERE ...`, matching the PREWHERE-after-JOIN grammar; the intended behaviour and reference output are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@zvonand I triaged every red check on this PR and split them into PR-caused vs. pre-existing/flaky/infra. One PR-caused failure was a safe test-only fix and I've pushed it; the other two need a build to settle, so I'm reporting rather than guessing. ✅ Fixed and pushed
Fix: moved
|
…iator With the old analyzer and `object_storage_cluster_join_mode='global'`, an `INNER` join leaves `query_to_send` pointing at `query_info.query`. `convertToRemote` then wrapped the left table function in `remote` in place, moving its alias inside, and `StorageDistributed::read` analyzed that query and threw `ALIAS_REQUIRED`. Pass `convertToRemote` a copy instead. Fixes `test_remote_initiator.py::test_remote_initiator_global_join_without_analyzer[s3]`. #2609 (comment) https://altinity-build-artifacts.s3.amazonaws.com/json.html?PR=2609&sha=ac820e248b649580c6488633d4e3eb2b3f1d9160&name_0=PR&name_1=Integration%20tests%20(amd_asan_ubsan%2C%20targeted)
Object storage supports `PREWHERE` only for formats that do. With `TSV` the query failed with `Storage S3 doesn't support PREWHERE` (`ILLEGAL_PREWHERE`) before reaching the join rewrite. #2609 (comment) https://altinity-build-artifacts.s3.amazonaws.com/json.html?PR=2609&sha=ac820e248b649580c6488633d4e3eb2b3f1d9160&name_0=PR&name_1=Stateless%20tests%20(amd_asan_ubsan%2C%20distributed%20plan%2C%20parallel%2C%20selected%20tests)
This branch has not been deployed
3 participants
Frontport of #2228 onto
antalya-26.8.object_storage_cluster_join_mode=localis a deprecated alias ofallow. The defaultallowreads the left object-storage table on the cluster and runsJOIN/ localINon the initiator, so the right table does not need to exist on remote nodes. That path works withallow_experimental_analyzer=0.globalrewrites the query toGLOBAL JOIN/GLOBAL INin the old interpreter and sends the right table with the remote query.ORDER BY ALLandGROUP BY ALLare cleared after the join is stripped, so the old analyzer does not re-expand them on the shards.Related: #2228
Related: #1827
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
object_storage_cluster_join_mode=allowjoins initiator-local tables on the initiator, including with the old analyzer.localis a deprecated alias ofallow.Documentation entry for user-facing changes
CI/CD Options
Exclude tests:
Regression jobs to run:
Made with Cursor