[pcbeingused333]

Fixes #6171

Description

In ClickhouseConnectionConfig._static_connection_kwargs, cluster/cloud mode unconditionally assigned settings["insert_quorum"] = "auto", overwriting any insert_quorum set in connection_settings (which #6171 shows can silently lose data in the insert-overwrite path). settings was also the user's own dict (self.connection_settings or {}), so the assignment mutated it in place.

The fix copies the dict and uses setdefault("insert_quorum", "auto"): auto stays the default in cluster mode, but a user-provided value wins.

Test Plan

Added test_clickhouse_insert_quorum_user_setting in tests/core/test_connection_config.py: a user insert_quorum is kept in cluster mode, the user's dict is not mutated, and the default is still auto when unset. It fails on main ('auto' == '0') and passes with the change; the other ClickHouse connection tests in that file pass too. make fast-test was not run in full (optional database drivers were not installed).

Checklist

• I have run make style and fixed any issues (ruff check and ruff format --check pass on the changed files)
• 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

This change was prepared with the help of an AI assistant (Claude Code), and the author is responsible for the change.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • ClickHouse connections now preserve a configured insert_quorum value and apply the "auto" default for cluster connections only when no value is set.
  • Connection settings are no longer modified in place.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

ClickHouse connection setup now copies connection settings before adding defaults. In cluster or Cloud mode, it sets insert_quorum to "auto" only when the setting is absent. Tests cover preserving a configured value, avoiding input-dictionary mutation, and applying the default.

Changes: ClickHouse connection settings

Layer / File(s)	Summary
Apply and verify quorum settings sqlmesh/core/config/connection.py, tests/core/test_connection_config.py	Connection setup copies the settings dictionary and keeps a user-provided insert_quorum. Cluster mode defaults to "auto" when no value is provided. Tests check both cases and verify that the input dictionary is unchanged.

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix · Severity of issue fixed: High

Suggested reviewers: stuffbyyuki

Merge Risk: Minimal · up to 4b054

The change preserves explicit ClickHouse quorum settings, avoids mutating caller-owned configuration, and retains the intended default.

Security architecture risk: Low · up to 4b054

The change restores operator control of an existing write-consistency setting without changing connection targets, credentials, or TLS controls. Risk is limited, but explicit quorum values can affect write consistency, and clustered failure and recovery behavior has not been validated here.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The changed quorum value is forwarded through connection construction. Its potential consistency impact therefore covers writes using affected cluster or cloud connections, rather than only one model or table. Actual replication outcomes depend on ClickHouse behavior and table configuration, which were not established.

Trust Boundaries and Controls

• inferred — The dbt configuration path supplies custom settings alongside host and credentials. Honoring insert_quorum extends precedence within that existing configuration authority, rather than establishing a new input channel. Authentication and TLS options remain in the existing connection configuration and factory paths; deployment-specific configuration provenance remains unknown.

Resilience and Maintainability Implications

• observed — The unchanged insert-overwrite implementation stages inserts before partition replacement or table exchange and attempts temporary-table cleanup in finally blocks. Table exchange retains its existing non-atomic rename fallback. Preserving quorum does not change this ordering or recovery implementation, and source inspection alone does not establish its runtime consistency guarantees.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title names the main change: ClickHouse cluster mode now keeps a user-provided insert_quorum. The raw summary and objectives confirm the change copies connection settings and uses setdefault so th…
Description check		The description follows the repository template. It has a Description section that explains the bug in ClickhouseConnectionConfig._static_connection_kwargs and the setdefault fix, a Test Plan section …
Linked Issues check		[#6171] The issue requires three coding behaviors: (1) a user-provided connection_settings.insert_quorum must be respected in cluster and cloud mode, (2) the default must only apply when the setting i…
Out of Scope Changes check		The whole-PR diff against the merge base changes two files. sqlmesh/core/config/connection.py contains only the copy of the settings dict, the setdefault change, and a comment line describing the prec…

Full details: Title check

Explanation

The title names the main change: ClickHouse cluster mode now keeps a user-provided insert_quorum. The raw summary and objectives confirm the change copies connection settings and uses setdefault so the user value wins over the auto default. The title uses the fix(clickhouse) prefix and sits within the 68-character length that the author supplied.

Full details: Description check

Explanation

The description follows the repository template. It has a Description section that explains the bug in ClickhouseConnectionConfig._static_connection_kwargs and the setdefault fix, a Test Plan section that names the new test and states the failing and passing results, and a Checklist. It links issue #6171. The unchecked items for make style and make fast-test are explained in the text, so the gaps are disclosed rather than missing.

Full details: Linked Issues check

Explanation

[#6171] The issue requires three coding behaviors: (1) a user-provided connection_settings.insert_quorum must be respected in cluster and cloud mode, (2) the default must only apply when the setting is unset, and (3) the user's settings dict must not be mutated. At head, sqlmesh/core/config/connection.py changes settings = self.connection_settings or {} to settings = dict(self.connection_settings or {}), which removes the aliasing. The cluster/cloud branch changes settings["insert_quorum"] = "auto" to settings.setdefault("insert_quorum", "auto"), so a user value wins and auto remains the default. tests/core/test_connection_config.py adds test_clickhouse_insert_quorum_user_setting, which asserts that a user value of "0" is kept in cluster mode, that the input dict is unchanged after building the kwargs, and that the default is "auto" when no value is set. These changes cover all three requirements. The issue's suggested follow-ups (documenting full-quorum needs, or syncing the temp table before REPLACE PARTITION) are stated as suggestions, not acceptance criteria, and the PR does not claim to implement them. Limitation: the reviewed evidence is the diff hunks and the test. The author reports the new test fails on main and passes with the change, and that make fast-test was not run in full. I did not re-run the tests.

Full details: Out of Scope Changes check

Explanation

The whole-PR diff against the merge base changes two files. sqlmesh/core/config/connection.py contains only the copy of the settings dict, the setdefault change, and a comment line describing the precedence rule. tests/core/test_connection_config.py adds one test function that covers the same behavior. Neither change touches unrelated connection types, configuration fields, or engine adapter code. Every change connects directly to #6171.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

CodeRabbit Verify

• Run CodeRabbit Verify to test the changes in this pull request.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 46e74b1e-e77f-4c30-8100-04d64d7e590d

Commits

Reviewing files that changed from the base of the PR and between c4dbac4 and 4b05402.

Files selected for processing: 2

• sqlmesh/core/config/connection.py
• tests/core/test_connection_config.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.