Skip to content

Fix ClickHouse cluster CTAS execution - #5978

Open
0xhanh wants to merge 1 commit into
SQLMesh:mainfrom
0xhanh:main
Open

Fix ClickHouse cluster CTAS execution#5978
0xhanh wants to merge 1 commit into
SQLMesh:mainfrom
0xhanh:main

Conversation

@0xhanh

@0xhanh 0xhanh commented Aug 23, 2026

Copy link
Copy Markdown

Description

Fix ClickHouse CTAS behavior in cluster mode.

Cluster CTAS now follows the same two-step flow as ClickHouse Cloud:

  1. Create the table with EMPTY and ON CLUSTER.
  2. Run a single client-side INSERT ... SELECT.

This prevents ON CLUSTER ... AS SELECT from executing the source query on every cluster node and duplicating data. A regression test covers the generated cluster-mode SQL.

Test Plan

  • Ran .venv/bin/python -m pytest tests/core/engine_adapter/test_clickhouse.py -q — 35 passed.
  • Ran Ruff format and lint checks for the changed files.

Checklist

  • I have run make style and fixed any issues
  • I have added tests for my changes (if applicable)
  • All existing tests pass (make fast-test)
  • My commits are signed off (git commit -s) per the [DCO](DCO)

@mday-io

mday-io commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Thanks for this. The two-step flow is the right fix, and matching the Cloud path is the correct approach. A few things before merge:

  1. The commit isn't signed off (CONTRIBUTING.md requires a Signed-off-by trailer matching the author on every commit). Please git commit --amend -s and force-push. The DCO check is also failing in CI.

  2. Our release notes are auto-generated from PR titles (flat list, no categorization), so the title is the release note users see. Suggest something like fix(clickhouse): cluster CTAS no longer duplicates rows across replicas. The commit body is currently empty; a line on the root cause (ON CLUSTER ... AS SELECT executes the SELECT on every node) would help.

This is a silent, non-opt-in behavior change for all cluster users, so please call it out in the PR description.

Any project with cluster set in its connection config is affected on upgrade, with no flag:

  • ReplicatedMergeTree users (the topology in our docs) were getting rows multiplied by replica count on FULL models. After this, those tables return correct counts, and anything downstream that consumed the inflated data (audits, row-count checks, aggregates elsewhere) shifts.
  • Plain MergeTree multi-node without replication: FULL-model data now lands only on the connected node instead of every node. Probably acceptable since incremental inserts already behave this way, but please confirm that's intended rather than an unnoticed regression.

Migration gap

An engine-adapter change doesn't refingerprint models, so existing tables keep their duplicated data until each model is next rebuilt. Users get a mix of corrected and still-inflated tables with nothing forcing reconciliation. Please document that affected models need a restatement to pick up the fix.

Docs

Add a note to docs/integrations/engines/clickhouse.md (cluster section) covering the two-step CTAS behavior in cluster mode and the restate-to-correct guidance. The release notes aren't discoverable later.

Tests

The new test only covers default MergeTree SQL-string shape. Please also add:

  • A ReplicatedMergeTree storage_format case, since that's the real-world topology and the actual duplication scenario.
  • An explicit assertion that the INSERT step does not carry ON CLUSTER.
  • A LIMIT 0 case confirming the second insert is skipped in cluster mode (the guard exists but is untested for this path).

@mday-io mday-io self-assigned this Sep 1, 2026
@mday-io
mday-io self-requested a review September 1, 2026 15:42
@mday-io

mday-io commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Also, should fix #6008

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants