Skip to content

fix(clickhouse): unwrap parenthesized PRIMARY_KEY independent of ORDER_BY#5881

Open
anxkhn wants to merge 1 commit into
SQLMesh:mainfrom
anxkhn:fix/clickhouse-primary-key-paren
Open

fix(clickhouse): unwrap parenthesized PRIMARY_KEY independent of ORDER_BY#5881
anxkhn wants to merge 1 commit into
SQLMesh:mainfrom
anxkhn:fix/clickhouse-primary-key-paren

Conversation

@anxkhn

@anxkhn anxkhn commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

The ClickHouse engine adapter builds the PRIMARY KEY clause in
_build_table_properties_exp. When the PRIMARY_KEY physical property is a
single parenthesized column (for example PRIMARY_KEY = (a)), sqlglot parses it
into an exp.Paren that has to be unwrapped, otherwise the adapter emits a
doubly-parenthesized PRIMARY KEY ((a)).

The unwrap guard checked the wrong value: it tested ordered_by_raw (the
ORDER_BY value) for exp.Paren instead of primary_key. This looks like a
copy-paste leftover from the ORDER_BY block just above it, which correctly uses
ordered_by_raw. As a result the parenthesized PRIMARY_KEY was only unwrapped
when ORDER_BY happened to be a Paren too. With a bare column, a tuple, or no
ORDER_BY at all, the parentheses were kept and the generated primary key came
out malformed:

-- PRIMARY_KEY = (a) with ORDER_BY = a
-- before: ENGINE=MergeTree ORDER BY (a) PRIMARY KEY ((a))
-- after:  ENGINE=MergeTree ORDER BY (a) PRIMARY KEY (a)

So the emitted primary key wrongly depended on the unrelated ORDER_BY shape.

The fix checks primary_key for exp.Paren, making the PRIMARY_KEY block
symmetric with the ORDER_BY block above it. It is a one-line change to the
guard.

Test Plan

Added two regression assertions to
tests/core/engine_adapter/test_clickhouse.py::test_model_properties, pairing
PRIMARY_KEY = (a) with a bare ORDER_BY = a and with a tuple
ORDER_BY = (a, b); both now assert PRIMARY KEY (a). The existing case in that
test used ORDER_BY = (a) (also a Paren), which is exactly why the bug was
masked. Reverting the source line makes the new bare-ORDER_BY assertion fail
with PRIMARY KEY ((a)); restoring it passes.

pytest tests/core/engine_adapter/test_clickhouse.py::test_model_properties -q   # 1 passed
pytest tests/core/engine_adapter/test_clickhouse.py -q -m "not slow and not docker"   # 32 passed
make style   # passed

I did not mark the full make fast-test checklist item because the broad suite
is currently blocked in this local environment by an unrelated pyodbc / system
unixODBC import failure in
tests/core/test_connection_config.py::test_mssql_pyodbc_connection_string_generation.
The ClickHouse-specific regression coverage above passes.

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

…R_BY

The PRIMARY_KEY branch in _build_table_properties_exp tested the ORDER_BY
value (ordered_by_raw) for exp.Paren instead of the primary_key value, a
copy-paste leftover from the ORDER_BY block above it. As a result a single
parenthesized PRIMARY_KEY such as PRIMARY_KEY = (a) was only unwrapped when
ORDER_BY also happened to be a Paren; with a bare column, a tuple, or no
ORDER_BY the parentheses were kept and the adapter emitted the malformed
PRIMARY KEY ((a)). The generated primary key wrongly depended on the
unrelated ORDER_BY shape.

Check primary_key for exp.Paren so it is unwrapped regardless of ORDER_BY,
and add regression cases pairing PRIMARY_KEY = (a) with a bare and a tuple
ORDER_BY.

Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
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.

1 participant