Skip to content

Remove object_storage_cluster_join_mode=local - #2228

Open
ianton-ru wants to merge 6 commits into
antalya-26.6from
feature/antalya-26.6/remove_object_storage_cluster_join_mode_local
Open

Remove object_storage_cluster_join_mode=local#2228
ianton-ru wants to merge 6 commits into
antalya-26.6from
feature/antalya-26.6/remove_object_storage_cluster_join_mode_local

Conversation

@ianton-ru

@ianton-ru ianton-ru commented Aug 18, 2026

Copy link
Copy Markdown

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

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:

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Performance tests
  • Aarch64 tests
  • All with ASAN
  • All with TSAN
  • All with MSAN
  • All with UBSAN
  • All with Coverage
  • 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)
  • OAuth (5m)
  • Parquet (1.5h)
  • RBAC (1.5h)
  • SSL Server (1h)
  • S3 (2h)
  • S3 Export (2h)
  • Swarms (30m)
  • Tiered Storage (2h)

@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown

Workflow [PR], commit [40e4fcd]

@ianton-ru ianton-ru added antalya port-antalya PRs to be ported to all new Antalya releases antalya-26.6 labels Aug 18, 2026
@ianton-ru
ianton-ru marked this pull request as ready for review August 19, 2026 08:03
@ianton-ru

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/Storages/IStorageCluster.cpp Outdated
Comment on lines 238 to 239
case ObjectStorageClusterJoinMode::LOCAL: // Legacy mode, equal to 'allow'
return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point!

Comment thread src/Core/Settings.cpp Outdated
- `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'.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

@ianton-ru
ianton-ru marked this pull request as draft August 19, 2026 10:06
ianton-ru and others added 4 commits August 19, 2026 16:01
…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>
@ianton-ru

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +230 to +231
&& (rhs->as<ASTSubquery>() || rhs->as<ASTSelectQuery>() || rhs->as<ASTSelectWithUnionQuery>()
|| rhs->as<ASTIdentifier>() || rhs->as<ASTTableIdentifier>()))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ianton-ru

Copy link
Copy Markdown
Author

@blau-ai

@blau-ai

blau-ai commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

CI triage for #2228

Verdict: 0 PR-caused failures. Three reds on this run — the PR aggregate gate plus two leaf checks — and both leaf failures are infra/flaky/pre-existing, unrelated to this change. All functional suites that ran (Fast test, all Stateless variants incl. cas s3 storage, builds amd/arm) are green on the substance.

Head SHA analyzed: 40e4fcd90298595f68b81d5ac94292e94acf8908 (from result_pr.json).


1. Stateless tests (amd_binary, cas s3 storage, parallel) — FAIL → flaky/infra, not PR-caused

  • Single failing test: 01880_materialized_view_to_table_type_check (Failed: 1, Passed: 11015).
  • The report's own auto-diagnosis: reran the test 48 times with the same randomized settings → 48 passed, 0 failed"All reruns passed. The failure is not reproducible (likely a transient issue)." The result is tagged flaky.
  • Decisive log line:
    Code: 499. DB::Exception: ... Poco::Exception. Code: 1000, e.code() = 111,
    Connection refused ..., bucket test, key cas_s3/cas/ref_catalog, object size 311262. (S3_ERROR)
    (query: CREATE TABLE test_input(id Int32) ENGINE=MergeTree() order by id;)
    
    The failure is a transient S3 connection-refused during a plain CREATE TABLE ... ENGINE=MergeTree, i.e. the CAS/S3 test backend, not this PR's code path (object-storage-cluster JOIN rewriting).
  • This check is also flagged do_not_block_pipeline_on_failure: true, so it does not gate the pipeline.
  • Next step: safe to re-run; no code change needed. (The test exercises MergeTree table creation and is unrelated to object_storage_cluster_join_mode.)

2. GrypeScanServer (-alpine) / Grype Scan — FAIL → pre-existing/infra, not PR-caused

  • "Grype Scan Completed with 7 high/critical vulnerabilities" — this is a CVE scan of OS packages inside the built clickhouse-server:...-alpine image.
  • This PR changes only src/Core/{Settings.cpp,SettingsEnums.h}, src/Interpreters/InterpreterSelectQuery.cpp, src/Storages/IStorageCluster.{cpp,h} and four tests/integration/** files — no Dockerfiles, no contrib/ submodules, no dependency/package changes. A code change to JOIN handling cannot add or remove image-level CVEs, so this is a branch-wide/base-image condition, not a regression from this PR.
  • Next step: not actionable from this PR. If it must go green it needs a base-image / package bump (separate dependency PR), independent of this change.

3. PR — FAIL → aggregate gate only

  • This is the roll-up status of the child checks above; it carries no independent error. It clears once the two items above are re-run/waived.

Health check

The change is well-scoped and its functional CI is clean: Fast test (9313 passed), all Stateless variants including cas storage, cas s3 storage (the sole fail being the flaky S3 one above), s3 storage, debug parallel/sequential, and both amd & arm release/binary/debug builds are green. Nothing in the red checks points at the JOIN-mode rewrite logic.

One coverage note (not a failure): the integration suites this PR actually edits — test_s3_cluster, test_storage_iceberg_with_spark/test_cluster_joins.py, test_storage_iceberg_with_spark/test_remote_initiator.py, test_database_iceberg — show as skipped in this pipeline (sanitizer/arch tag exclusions), so the behavioral change to object_storage_cluster_join_mode isn't directly validated by this run. If you want that coverage before merge, it's worth kicking off the relevant integration job.

Bottom line: nothing to fix in the PR. Re-run the cas s3 storage stateless job to clear the flake; the Grype finding is a pre-existing image scan unrelated to this diff.

🤖 automated CI triage by @blau-ai — evidence from praktika result_pr.json for the head SHA.

@ianton-ru
ianton-ru marked this pull request as ready for review August 28, 2026 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

antalya antalya-26.6 port-antalya PRs to be ported to all new Antalya releases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants