Skip to content

Conversation

@StevenReitsma
Copy link
Contributor

@StevenReitsma StevenReitsma commented Dec 16, 2025

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_clause in ClickHouse specific code, which is necessary for clustered self-hosted ClickHouse installations.

Summary by CodeRabbit

  • New Features

    • Added a ClickHouse-specific path to ensure non-temporary table creation behaves consistently.
  • Bug Fixes

    • Fixed temporary table handling in environments that don’t support database-scoped temporaries.
  • Improvements

    • Made delete and drop operations cluster-aware for distributed setups.
    • Injected query execution settings into insert-as-select and monitoring snapshots to improve consistency and performance.

✏️ Tip: You can customize this high-level summary in your review settings.

@github-actions
Copy link
Contributor

👋 @StevenReitsma
Thank you for raising your pull request.
Please make sure to add tests and document all user-facing changes.
You can do this by editing the docs files in the elementary repository.

@coderabbitai
Copy link

coderabbitai bot commented Dec 16, 2025

📝 Walkthrough

Walkthrough

Adds 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

Cohort / File(s) Summary
Query settings injection
macros/edr/tests/on_run_end/handle_tests_results.sql, macros/utils/table_operations/insert_as_select.sql
Inserts {{ elementary.get_query_settings() }} into INSERT-SELECT / insert-as-select SQL generation immediately after the FROM/temporary selection. No other control flow changes.
ClickHouse cluster-aware drop/delete
macros/edr/tests/test_utils/clean_elementary_test_tables.sql
Replaces delegation with an explicit loop that constructs DROP TABLE IF EXISTS ... SYNC statements per test table using on_cluster_clause(...), ensuring drops target all cluster nodes.
ClickHouse cluster-aware delete modifier
macros/utils/table_operations/delete_and_insert.sql
Adds {{ on_cluster_clause(relation) }} between table name and delete in ClickHouse alter-delete statements to make delete operations cluster-aware.
ClickHouse create-table-as behavior
macros/utils/table_operations/create_table_as.sql
Adds clickhouse__edr_get_create_table_as_sql(temporary, relation, sql_query) macro that forces temporary to false and delegates to dbt.get_create_table_as_sql(false, relation, sql_query).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped through macros, tweaks in my paws,

Added settings and cluster-aware laws,
Dropped tables across nodes with a cheer,
Forced temp-tables to vanish — never fear,
A tiny rabbit’s patch: tidy and clear.

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Add ClickHouse cluster support, part 2' directly matches the PR's primary objectives of adding cluster support and query settings for ClickHouse, and accurately reflects the main changes across all modified files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@StevenReitsma StevenReitsma changed the title Don't create temporary table if adapter has no support and add missing query settings Add ClickHouse cluster support, part 2 Dec 16, 2025
@arbiv
Copy link
Contributor

arbiv commented Dec 30, 2025

@StevenReitsma, thanks for opening the PR! It seems that there is an issue with the Databricks tests. Could you take a look?

@StevenReitsma
Copy link
Contributor Author

StevenReitsma commented Jan 6, 2026

@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?

Copy link

@coderabbitai coderabbitai bot left a 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 3d1a83a and 48ec34a.

📒 Files selected for processing (5)
  • macros/edr/tests/on_run_end/handle_tests_results.sql
  • macros/edr/tests/test_utils/clean_elementary_test_tables.sql
  • macros/utils/table_operations/create_table_as.sql
  • macros/utils/table_operations/delete_and_insert.sql
  • macros/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 macro elementary.get_query_settings() is properly defined in macros/utils/cross_db_utils/get_query_settings.sql and 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.

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