Allow empty object storage cluster#2028
Conversation
Share cluster resolution via resolveClusterRead so getQueryProcessingStage matches read when object_storage_cluster_fallback_if_empty is enabled, and skip local object_storage_cluster lookup when object_storage_remote_initiator and object_storage_remote_initiator_cluster are both set. Co-authored-by: Cursor <cursoragent@cursor.com>
Cover pure fallback on unknown cluster, aggregate planning, remote-initiator interaction, and integration scenarios with locally unknown object_storage_cluster. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@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: bc7aacf2e4
ℹ️ 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".
| const bool defer_object_storage_cluster_resolution | ||
| = settings[Setting::object_storage_remote_initiator] | ||
| && !settings[Setting::object_storage_remote_initiator_cluster].value.empty(); |
There was a problem hiding this comment.
When object_storage_remote_initiator=1 and only object_storage_remote_initiator_cluster is set on the initiator, this condition now forces fallback_to_pure=false even though there is no local object_storage_cluster to defer. read then takes the clustered path and rewrites the remote query with make_cluster_function=true, producing s3Cluster('', ...) instead of the previous s3(...); on remote users whose profile defines object_storage_cluster, that bypasses the fallback table function and the read runs only on the remote initiator, regressing the existing test_object_storage_remote_initiator_without_cluster_function case that expects replica subqueries. Only defer local cluster resolution when cluster_name_from_settings is non-empty.
Useful? React with 👍 / 👎.
Cover stateless and integration cases where object_storage_cluster is missing locally and remote initiator falls back to non-cluster execution. 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: 1ad4ce9652
ℹ️ 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 (!defer_object_storage_cluster_resolution | ||
| && !result.fallback_to_pure | ||
| && settings[Setting::object_storage_cluster_fallback_if_empty]) |
There was a problem hiding this comment.
Because this check is in the IStorageCluster base and only looks at the resolved cluster name, enabling object_storage_cluster_fallback_if_empty also changes explicit cluster table functions such as s3Cluster('typo', ...): getClusterName returns the function argument, tryGetCluster returns null, and the read falls back to pure_storage on the initiator instead of reporting the bad cluster. This makes a misspelled or temporarily empty explicit cluster silently run as a single-node read whenever the setting is enabled, so the fallback needs to be gated to names that actually came from the object_storage_cluster setting or otherwise kept out of explicit *Cluster calls.
Useful? React with 👍 / 👎.
…uster functions. Distinguish s3() with object_storage_cluster setting from s3Cluster() argument so fallback works for the former but explicit cluster names still fail when unknown. Co-authored-by: Cursor <cursoragent@cursor.com>
Distinguish alternative-syntax table functions from table reads so remote initiator keeps s3() for fallback and uses cluster path for Iceberg tables. 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: 92a51421bd
ℹ️ 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 remote_initiator_cluster_name = settings[Setting::object_storage_remote_initiator_cluster].value; | ||
| if (!remote_initiator_cluster_name.empty()) | ||
| { |
There was a problem hiding this comment.
object_storage_cluster
When object_storage_remote_initiator=1 is used with alternative syntax and only object_storage_cluster is set, this new branch leaves remote_initiator_cluster unset because object_storage_remote_initiator_cluster is empty. That was a supported path: the old code defaulted the remote initiator cluster to cluster_name_from_settings, but now getQueryProcessingStage/read either throw BAD_ARGUMENTS or, with object_storage_cluster_fallback_if_empty=1, run the read locally instead of on a remote initiator. Please preserve the previous default to the object-storage cluster when the explicit remote-initiator cluster setting is absent.
Useful? React with 👍 / 👎.
| if (!cluster_name_in_settings && !getOriginalClusterName().empty()) | ||
| return false; |
There was a problem hiding this comment.
object_storage_cluster
This disables object_storage_cluster_fallback_if_empty for persistent S3/Iceberg tables created with SETTINGS object_storage_cluster='...', because those tables store that setting in getOriginalClusterName while cluster_name_in_settings remains false. As a result, SELECT ... SETTINGS object_storage_cluster_fallback_if_empty=1 against such a table still resolves the missing/empty cluster normally and throws instead of falling back to the local storage, even though the new setting is described as applying when object_storage_cluster is set. Please distinguish table-engine settings from explicit *Cluster arguments before rejecting the fallback.
Useful? React with 👍 / 👎.
… cluster setting. Track explicit *Cluster function arguments separately from table engine and query object_storage_cluster settings so fallback works for persistent tables. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
1 participant
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Allow empty object storage cluster
Documentation entry for user-facing changes
With 'object_storage_cluster' setting query to s3,iceberg and some other sources are executed as cluster request.
But with swarm cluster, when initiator is not a cluster member, may be situation when no one swarm node is alive at the moment. In this case query is failed with
CLUSTER_DOESNT_EXISTerror.New setting
object_storage_cluster_fallback_if_emptyallow to execute read query on local node in this case.Write query is not executed on cluster right now, so attempt to write is still failed in this case to avoid situation when query is success when swarm is empty and failed when has some nodes alive.
CI/CD Options
Exclude tests:
Regression jobs to run: