Repository navigation
Conversation
The old analyzer drops constant GROUP BY keys when distributed_depth is 0. Cluster table function reads never increased that depth, so workers folded hostName() and returned only the aggregate. https: //github.com//issues/2139 Co-authored-by: Cursor <cursoragent@cursor.com>
Raising distributed_depth on cluster reads also changes parallel replicas, max_distributed_depth, and block marshalling. The old analyzer now keeps a key such as hostName() and still drops ordinary constants. https: //github.com//issues/2139 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. |
![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: ea70a1fdff
ℹ️ 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".
| if (current->function_base && current->function_base->isServerConstant()) | ||
| return true; |
There was a problem hiding this comment.
When a scalar subquery is represented by __getScalar (notably for Array, Tuple, or aggregate-state results), its function base also reports isServerConstant, even though ExecuteScalarSubqueriesVisitor evaluates it on the initiator and ships the same scalar to every shard. This check therefore retains such constant GROUP BY keys instead of removing them as before; for example, with the old analyzer, grouping an aggregate query by a scalar subquery returning Tuple(Dynamic) now reaches group-key validation and throws ILLEGAL_COLUMN, whereas the constant key previously disappeared. Special-case shipped scalar accessors, or otherwise distinguish genuinely per-server functions from every function carrying this flag.
Useful? React with 👍 / 👎.
__getScalar is marked as a server constant, but the initiator evaluates it and sends the same value to every shard. Keeping it made GROUP BY of a scalar subquery with Dynamic throw ILLEGAL_COLUMN. https: //github.com//pull/2490 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: 12460edf61
ℹ️ 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".
| for (const auto * child : current->children) | ||
| stack.push(child); |
There was a problem hiding this comment.
Because an ActionsDAG can share child nodes, this traversal can revisit the same subtree exponentially many times. For example, a linear sequence of aliases such as plus(x0, x0) AS x1, plus(x1, x1) AS x2, and so on produces a constant DAG where each level references the same child twice; checking the final alias explores roughly 2^N paths when no server constant exists, allowing a compact old-analyzer query to consume an analysis thread indefinitely. Track visited node pointers before pushing children.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The alias chain in this comment does not keep an analysis thread busy. containsServerConstant does not record visited nodes, and an ActionsDAG can store the same child twice, but this example never reaches that walk as a compact graph.
The old analyzer expands aliases in QueryNormalizer before aggregation analysis. The expanded tree is limited by max_expanded_ast_elements (default 500000). A chain 1 AS x0, x0 + x0 AS x1, … throws TOO_BIG_AST at depth 16. Depth 15 finishes in about 0.25s (enable_analyzer = 0). The same SELECT list with GROUP BY 1 takes about 0.15s, so the extra time is the expanded tree, not an unbounded walk of a shared DAG.
Measured on a local server:
| Depth | Result |
|---|---|
| 8 | 0.009s |
| 12 | 0.028s |
| 14 | 0.120s |
| 15 | 0.244s |
| 16 | TOO_BIG_AST |
| /// Constant expressions have non-null column pointer at this stage. | ||
| if (node->column) | ||
| /// Server constants stay: they are different on each shard. | ||
| if (node->column && !containsServerConstant(node)) |
There was a problem hiding this comment.
For a local enable_analyzer=0 query, distributed_depth is also zero, so this predicate now retains server constants even though only one server can observe their value. For example, SELECT count() FROM numbers(3) GROUP BY [CAST(hostName(), 'Dynamic')] SETTINGS enable_analyzer = 0 previously removed the folded constant key, but now retains its Array(Dynamic) type and subsequently raises ILLEGAL_COLUMN under the default allow_suspicious_types_in_group_by = 0; the new analyzer still removes such keys when the context is not distributed. Limit this exception to queries that can actually dispatch work, including the affected cluster-backed table functions.
Useful? React with 👍 / 👎.
| return true; | ||
| } | ||
|
|
||
| /// A folded server constant (`hostName()`, `tcpPort()`, ...) must stay a GROUP BY key. |
There was a problem hiding this comment.
This comment refers to the functions themselves rather than describing particular applications, so the repository convention requires the names to be written as hostName and tcpPort, without parentheses.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
ianton-ru
added
antalya
port-antalya
A local query has one server, so a folded server constant is an ordinary constant. Keeping it made GROUP BY of `hostName` cast to `Dynamic` throw `ILLEGAL_COLUMN`. Secondary queries still keep the key, because cluster table function workers also have `distributed_depth` 0. https: //github.com//pull/2490 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". |
This branch has not been deployed
2 participants
Solved #2139
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix GROUP BY server constants over s3Cluster and object_storage_cluster
Documentation entry for user-facing changes
...
CI/CD Options
Exclude tests:
Regression jobs to run: