Fix aggregation flow with remote initiator

[ianton-ru]

Changelog category (leave one):

• Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Fix aggregation flow with remote initiator

Documentation entry for user-facing changes

With object_storage_remote_initiator but without object_storage_cluster setting StorageObjectStorageCluster::getQueryProcessingStage returned QueryProcessingStage::Enum::FetchColumns, as result nodes sent all rows on initiator and aggregation executed on initiator.
Now method returns QueryProcessingStage::Enum::WithMergeableState is proper cases, and pre-aggregation executed on nodes.

CI/CD Options

Exclude tests:

• Fast test
• Integration Tests
• Stateless tests
• Stateful tests
• Performance tests
• All with ASAN
• All with TSAN
• All with MSAN
• All with UBSAN
• All with Coverage
• All with Aarch64
• All Regression
• Disable CI Cache

Regression jobs to run:

• Fast suites (mostly <1h)
• Aggregate Functions (2h)
• Alter (1.5h)
• Benchmark (30m)
• ClickHouse Keeper (1h)
• Iceberg (2h)
• LDAP (1h)
• Parquet (1.5h)
• RBAC (1.5h)
• SSL Server (1h)
• S3 (2h)
• S3 Export (2h)
• Swarms (30m)
• Tiered Storage (2h)

[ianton-ru]

@codex review

[github-actions]

Workflow [PR], commit [532219a]

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e72ebfe177

ℹ️ 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".

[chatgpt-codex-connector]

Keep unsupported storages at FetchColumns

When object_storage_remote_initiator is set for a configuration that does not support cluster mode, such as icebergLocal or other local data-lake storages where getClusterName is empty because isClusterSupported is false, this new exception makes getQueryProcessingStage report WithMergeableState. IStorageCluster::read still immediately falls back to pure_storage for those configurations, and StorageObjectStorage::read ignores processed_stage, so aggregate queries can be planned as if partial aggregation happened remotely even though only raw rows were read. Please only return the distributed stage when the subsequent read path will actually use the remote/cluster execution path.

Useful? React with 👍 / 👎.

[ianton-ru]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 437a47dc4a

ℹ️ 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".

[chatgpt-codex-connector]

Preserve validation for missing remote initiator cluster

When object_storage_remote_initiator=1 is set on the fallback s3/object-storage table functions but both object_storage_cluster and object_storage_remote_initiator_cluster are empty, this now makes is_remote false and routes the query through BaseSimple. That bypasses the existing IStorageCluster::read validation that raises BAD_ARGUMENTS for this invalid configuration, so a misconfigured query silently runs locally instead of failing or using a remote initiator.

Useful? React with 👍 / 👎.

[ianton-ru]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0878f8f10d

ℹ️ 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".

[chatgpt-codex-connector]

Preserve validation for bare remote initiator requests

When object_storage_remote_initiator=1 is set on the fallback s3/object-storage table function without object_storage_cluster or object_storage_remote_initiator_cluster, this new guard leaves is_remote false, so executeImpl builds the simple storage and bypasses the validation added in StorageObjectStorageCluster/IStorageCluster. That makes an invalid remote-initiator request run locally instead of raising the documented exception, which is easy to hit when the companion cluster setting is omitted or misspelled.

Useful? React with 👍 / 👎.

[ianton-ru]

object_storage_remote_initiator_cluster without cluster name or object_storage_remote_initiator_cluster call an exception before.

[mkmkme]

LGTM

[mkmkme]

To be honest, I'm not really fond of these magic constants as it's difficult to see for myself if that's actually the value we should be expecting, but I trust that you've verified it to be actually valid

[ianton-ru]

Me too, but I reused existed data files, https://github.com/Altinity/ClickHouse/blob/antalya-26.3/tests/integration/test_s3_cluster/data/clickhouse/part1.csv and others. Number means nothing, just summa of values in all files.

[ianton-ru]

Stateless test 03443_shared_storage_snapshots unrelated, looks like test is a time sensitive, may be failed when something executed too long.

[alsugiliazova]

Audit update for PR #1872

AI audit note: This review comment was generated by AI (claude-opus-4.7).

PR: Altinity/ClickHouse#1872 — Fix aggregation flow with remote initiator
HEAD: 0878f8f10d653e92284c60d674faf568f14dfe67
Base: antalya-26.3

Confirmed defects

Medium — Silent fallback bypasses remote-initiator validation for s3() fallback table function

• Impact: SELECT ... FROM s3(url, ...) SETTINGS object_storage_remote_initiator=1 (with neither object_storage_cluster nor object_storage_remote_initiator_cluster) now silently runs locally on the initiator instead of raising BAD_ARGUMENTS. Pre-PR, this misconfiguration produced an exception in IStorageCluster::read. Users have no signal that object_storage_remote_initiator was ignored — a typo in the companion cluster-setting name silently downgrades the query to single-node execution.
• Anchor: src/TableFunctions/TableFunctionObjectStorageClusterFallback.cpp / parseArgumentsImpl — the new guard is_remote = settings[object_storage_remote_initiator] && (!object_storage_cluster.empty() || !object_storage_remote_initiator_cluster.empty()).
• Trigger: SELECT ... FROM s3('http://.../*', ...) SETTINGS object_storage_remote_initiator=1 with no cluster settings (e.g. user mistyped object_storage_remote_initiator_cluser).
• Why defect: With is_remote=false the fallback routes to BaseSimple::executeImpl, which constructs a plain StorageObjectStorage. Neither StorageObjectStorageCluster::getQueryProcessingStage nor IStorageCluster::read is invoked, so the existing/new validation throws are unreachable for this entry point. Codex P2 flagged this; the author's dismissal ("call an exception before") does not hold because the table-function change itself removes the path that reached the exception.
• Fix direction: In parseArgumentsImpl, when object_storage_remote_initiator is set but both cluster settings are empty, raise BAD_ARGUMENTS explicitly instead of silently clearing is_remote.
• Regression test direction: Add an integration assertion that s3(url, ...) SETTINGS object_storage_remote_initiator=1 (no cluster settings) raises BAD_ARGUMENTS, mirroring the existing happy-path tests in tests/integration/test_s3_cluster/test.py.

Low — Typo in newly-added error-code declaration (BAD_ARGUMENT vs BAD_ARGUMENTS)

• Impact: Dead declaration in the file; the new throw compiles only by accident via a transitively-included declaration of BAD_ARGUMENTS. If that transitive include is removed in a future refactor, the build breaks at this site.
• Anchor: src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp / namespace ErrorCodes (line ~51) — extern const int BAD_ARGUMENT;.
• Trigger: Static — the declared symbol BAD_ARGUMENT (singular) is not defined in src/Common/ErrorCodes.cpp (only BAD_ARGUMENTS, code 36, exists), and the new throw at line 733 uses ErrorCodes::BAD_ARGUMENTS.
• Why defect: The PR intended to declare the error code used by its new throw but mistyped the symbol; the unused BAD_ARGUMENT extern is silently accepted by the compiler and the throw resolves to a leaked declaration from another header.
• Fix direction: Change extern const int BAD_ARGUMENT; to extern const int BAD_ARGUMENTS;.
• Regression test direction: None needed; covered by the normal build once the local declaration matches the symbol that is actually referenced.

Coverage summary

• Scope reviewed: All 3 source files + the new integration test at HEAD 0878f8f. Call graph: s3() / s3Cluster() / ENGINE=S3Cluster → TableFunctionObjectStorageClusterFallback::parseArgumentsImpl → executeImpl (routes to BaseCluster vs BaseSimple) → StorageObjectStorageCluster::getQueryProcessingStage (new early-return for !isClusterSupported(), new validation for remote_initiator without clusters) → IStorageCluster::read (error-message text only).
• Categories failed: validation-bypass / silent-fallback on misconfigured s3() fallback (Medium); error-code declaration typo (Low).
• Categories passed: branch enumeration of getQueryProcessingStage (5 settings combinations × isClusterSupported true/false) — all reachable branches produce a consistent processed_stage and read path; happy-path WithMergeableState plumbing into IStorageCluster::read for both cluster_name-set and remote_initiator_cluster-set scenarios; unsupported-storage early return correctly avoids the Codex P2 Update README.md #1 issue (returning WithMergeableState for storages whose read falls back to pure); exception-safety (throws occur in planning before state mutation); concurrency (settings read from per-query immutable context, no shared mutable state introduced); test assertions actually verify pre-aggregation (result_rows == "2" for SUM, and 2 ≤ rows ≤ 4 for GROUP BY bit are consistent with WithMergeableState not FetchColumns).
• Not applicable: iterator/reference invalidation, integer overflow/signedness, RAII leaks, multithreaded interleaving (no new shared state), rollback/partial-update (no mutation paths added).
• Assumptions/limits: Static reasoning only; did not run CI or the new integration test. Assumed getOriginalClusterName() is never empty for StorageObjectStorageCluster instances created through normal s3Cluster / ENGINE=S3Cluster paths, so a hypothetical processed_stage=WithMergeableState + readFallBackToPure(...) mismatch (which would require getClusterName() to return empty while remote_initiator_cluster is set and remote_initiator=0) is not realistically reachable; flagged but not promoted to a confirmed defect.

[mkmkme]

Low — Typo in newly-added error-code declaration (BAD_ARGUMENT vs BAD_ARGUMENTS)

That's a good one :D

[ianton-ru]

Add fixes for both defects