Skip to content

fix: Primary key declaration#648

Open
vrkirillov wants to merge 16 commits into
ClickHouse:mainfrom
vrkirillov:primary-key
Open

fix: Primary key declaration#648
vrkirillov wants to merge 16 commits into
ClickHouse:mainfrom
vrkirillov:primary-key

Conversation

@vrkirillov

@vrkirillov vrkirillov commented May 15, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Added support for using PRIMARY KEY in MergeTree family table definitions in place of ORDER BY
  • Added support lists of columns in PRIMARY KEY declarations

Checklist

Delete items not relevant to your PR:

  • Unit and integration tests covering the common scenarios were added
  • A human-readable description of the changes was provided to include in CHANGELOG

Note

Medium Risk
Changes default DDL for MergeTree tables without order_by (PK-only vs ORDER BY (tuple())), which can affect physical layout and query performance for existing models.

Overview
Adds MergeTree primary_key model config (string or column list) so DDL can emit PRIMARY KEY (...) for table, distributed_table, and seed materializations, with docs and YAML/SQL examples in the changelog.

Refactors shared DDL helpers in table.sql: make_columns, renamed order_by_clause / partition_by_clause, and primary_key before order by / partition by. When only primary_key is set (no order_by), the adapter no longer forces default ORDER BY (tuple()), matching ClickHouse’s ability to rely on the primary key for sorting.

Integration tests assert system.tables primary_key and sorting_key for models with PK-only vs PK + explicit order_by.

Reviewed by Cursor Bugbot for commit c8c860d. Bugbot is set up for automated code reviews on this repo. Configure here.

Comment thread dbt/include/clickhouse/macros/materializations/seed.sql
Comment thread dbt/include/clickhouse/macros/materializations/table.sql Outdated
vrkirillov

This comment was marked as resolved.

vrkirillov

This comment was marked as resolved.

@vrkirillov vrkirillov changed the title Primary key declaration fix: Primary key declaration May 27, 2026

@vrkirillov vrkirillov left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fix recommendations in Cursor

vrkirillov

This comment was marked as resolved.

For MergeTree tables, prefer this order:
ENGINE = MergeTree()
ORDER BY (...)
PARTITION BY (...)
PRIMARY KEY (...)
TTL ...
SETTINGS ...

@cursor cursor 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.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit fb97021. Configure here.

Comment thread dbt/include/clickhouse/macros/materializations/table.sql
…that the helper renders geRename make_columns to expression_list_clause to better reflect that the helper renders generic ClickHouse expression lists, not only column names.

Simplify order_by_clause by handling the three cases directly:
- use explicit order_by when configured
- fall back to primary_key when order_by is omitt_by is omitt_by is omitt_by is omitt_by is omitt_by is omitt_by is omitt_by is o
- otherwise use tuple()
@BentsiLeviav BentsiLeviav requested a review from koletzilla July 6, 2026 11:41
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