-
Notifications
You must be signed in to change notification settings - Fork 121
Add ClickHouse cluster support, part 2 #904
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Add ClickHouse cluster support, part 2 #904
Conversation
|
👋 @StevenReitsma |
📝 WalkthroughWalkthroughAdds ClickHouse cluster-awareness and query-settings injection across several table-operation macros, replaces a delegated cleanup implementation with explicit DROP statements for cluster nodes, and introduces a ClickHouse-specific create-table-as macro forcing non-temporary creation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@StevenReitsma, thanks for opening the PR! It seems that there is an issue with the Databricks tests. Could you take a look? |
3d1a83a to
b58313d
Compare
a158e1f to
48ec34a
Compare
|
@arbiv I've pushed a potential fix for the failing tests. I don't have a Databricks environment so can't really test it myself. Could you give the CI tests another shot? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @macros/edr/tests/test_utils/clean_elementary_test_tables.sql:
- Around line 38-45: The DROP TABLE generation in macro
clickhouse__get_clean_elementary_test_tables_queries calls an undefined macro
on_cluster_clause, causing runtime failures; define a new macro named
on_cluster_clause that returns either "ON CLUSTER <cluster_name>" when a cluster
config (e.g., a variable like clickhouse_cluster or env/context flag) is present
or an empty string otherwise, and update any other usages (e.g., in
macros/utils/table_operations/delete_and_insert.sql) to call this macro or,
alternatively, replace the call with a conditional expression that emits the ON
CLUSTER clause only when a cluster name is configured.
In @macros/utils/table_operations/delete_and_insert.sql:
- Line 62: Define a new macro on_cluster_clause(relation) that returns the
ClickHouse "ON CLUSTER <cluster_name>" fragment when running in a clustered
adapter and an empty string otherwise, and update the SQL to call that macro;
also fix the adapter.get_model_query_settings call inside the macro by passing
the correct model context (either add model to the macro signature or replace
the call with adapter.get_model_query_settings(delete_relation) /
adapter.get_model_query_settings(insert_relation) depending on which relation’s
settings are required) so that adapter.get_model_query_settings is invoked with
a defined object instead of the undefined variable model.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
macros/edr/tests/on_run_end/handle_tests_results.sqlmacros/edr/tests/test_utils/clean_elementary_test_tables.sqlmacros/utils/table_operations/create_table_as.sqlmacros/utils/table_operations/delete_and_insert.sqlmacros/utils/table_operations/insert_as_select.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- macros/edr/tests/on_run_end/handle_tests_results.sql
- macros/utils/table_operations/create_table_as.sql
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: haritamar
Repo: elementary-data/dbt-data-reliability PR: 825
File: macros/utils/table_operations/create_table_as.sql:0-0
Timestamp: 2025-07-22T15:41:57.338Z
Learning: The dbt.create_table_as macro breaks in dbt-fusion when not run from a materialization context, requiring custom implementations for table creation functionality in dbt-fusion compatible packages.
🔇 Additional comments (1)
macros/utils/table_operations/insert_as_select.sql (1)
11-11: The macroelementary.get_query_settings()is properly defined inmacros/utils/cross_db_utils/get_query_settings.sqland implements correct platform-safe behavior using dbt's adapter dispatch pattern. It returns an empty string for non-ClickHouse adapters and ClickHouse-specific query settings for ClickHouse adapter.
This is a continuation of #878. The query settings were missing for a couple of more queries, which is relevant for the ClickHouse platform in some situations. Hopefully this now covers all possible use-cases.
Also, there was a bug where temporary tables were being created on platforms that don't support it (like ClickHouse). This PR fixes that.
Finally, we add
on_cluster_clausein ClickHouse specific code, which is necessary for clustered self-hosted ClickHouse installations.Summary by CodeRabbit
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.