Skip to content

Make Elementary work with ClickHouse cluster mode #878

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.

Sign up for GitHub

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

Merged
arbiv merged 4 commits into from
Nov 19, 2025
Merged

Make Elementary work with ClickHouse cluster mode #878

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8db4bcf
Add cluster and exchange capabilities to temp relation creation
StevenReitsma Oct 23, 2025
9519574
Use ClickHouse query settings in delete_and_insert
StevenReitsma Oct 23, 2025
f43acef
Use ClickHouse query settings in insert_rows
StevenReitsma Oct 23, 2025
0abcdc4
Fix get_query_settings SQL macro syntax
StevenReitsma Nov 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions macros/utils/cross_db_utils/get_query_settings.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{% macro get_query_settings() %}
{% do return(adapter.dispatch("get_query_settings", "elementary")()) %}
{% endmacro %}

{% macro default__get_query_settings() %}
{% do return("") %}
{% endmacro %}

{% macro clickhouse__get_query_settings() %}
{% do return(adapter.get_model_query_settings(model)) %}
{% endmacro %}

5 changes: 3 additions & 2 deletions macros/utils/table_operations/delete_and_insert.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,15 @@
{% set delete_query %}
alter table {{ relation }} delete where
{{ delete_column_key }} is null
or {{ delete_column_key }} in (select {{ delete_column_key }} from {{ delete_relation }});
or {{ delete_column_key }} in (select {{ delete_column_key }} from {{ delete_relation }})
{{ adapter.get_model_query_settings(model) }};
Comment on lines +64 to +65
Copy link

@coderabbitai coderabbitai bot Oct 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

⚠️ Inconsistent approach to injecting query settings; verify ClickHouse SQL syntax.

Two issues here:

  1. Inconsistent pattern: This file directly calls adapter.get_model_query_settings(model) while insert_rows.sql uses the dispatch macro elementary.get_query_settings(). Both target the same goal (injecting ClickHouse SETTINGS) but through different mechanisms. Standardize on one approach—the dispatch pattern is preferable for extensibility.

  2. SQL syntax concern (line 72): The insert query places the SETTINGS clause between INSERT INTO and SELECT:

    insert into {{ relation }} {{ adapter.get_model_query_settings(model) }} select * from {{ insert_relation }};

    ClickHouse typically expects SETTINGS at the very end of the query. Verify this syntax is valid for ClickHouse INSERT...SELECT...SETTINGS statements. If not, move the SETTINGS clause to the end of the query.

Also applies to: 72-72

I'll verify the concerns in this review comment by examining the actual files and checking ClickHouse syntax.

🏁 Script executed:

#!/bin/bash
# Locate and inspect the SQL files mentioned
echo "=== Locating delete_and_insert.sql ==="
fd -type f -name "delete_and_insert.sql"

echo -e "\n=== Locating insert_rows.sql ==="
fd -type f -name "insert_rows.sql"

Length of output: 414

🌐 Web query:

ClickHouse INSERT SELECT SETTINGS clause syntax location

💡 Result:

Put the SETTINGS clause immediately after the table name and optional column list (i.e., before VALUES, FORMAT, or SELECT).
You can also set query-level options inside the SELECT itself using its own SETTINGS clause. [1][2]

Sources:
[1] ClickHouse INSERT INTO statement (syntax and note about SETTINGS placement).
[2] ClickHouse SELECT — SETTINGS in SELECT queries.

Let me correct the shell script and inspect both files:

🏁 Script executed:

#!/bin/bash
# Find and inspect both SQL files
echo "=== Finding delete_and_insert.sql ==="
find . -name "delete_and_insert.sql" -type f 2>/dev/null | head -5

echo -e "\n=== Finding insert_rows.sql ==="
find . -name "insert_rows.sql" -type f 2>/dev/null | head -5

echo -e "\n=== Content of delete_and_insert.sql (around lines 64-72) ==="
delete_file=$(find . -name "delete_and_insert.sql" -type f 2>/dev/null | head -1)
if [ -n "$delete_file" ]; then
  sed -n '60,75p' "$delete_file" | cat -n
fi

echo -e "\n=== Content of insert_rows.sql (full file for inspection) ==="
insert_file=$(find . -name "insert_rows.sql" -type f 2>/dev/null | head -1)
if [ -n "$insert_file" ]; then
  cat "$insert_file" | cat -n
fi

echo -e "\n=== Search for get_query_settings macro definition ==="
rg -n "get_query_settings" --type sql

Length of output: 11002

Let me examine the macro implementations to verify if the inconsistency claim is valid and check the dispatch patterns:

🏁 Script executed:

#!/bin/bash
# Inspect get_query_settings macro definition and implementations
echo "=== get_query_settings macro definitions ==="
cat ./macros/utils/cross_db_utils/get_query_settings.sql

echo -e "\n=== Search for get_model_query_settings usage and definition ==="
rg -n "get_model_query_settings" -C 3

echo -e "\n=== Check if get_query_settings macro is used elsewhere ==="
rg -n "elementary.get_query_settings\|get_query_settings" --type sql -C 2 | head -50

Length of output: 2290

Standardize on dispatch macro pattern for query settings.

This file directly calls adapter.get_model_query_settings(model) (lines 65, 72) while insert_rows.sql uses the dispatch macro elementary.get_query_settings(). Both target the same goal for ClickHouse, but the dispatch pattern (used in insert_rows.sql) is preferable—it provides adapter-agnostic abstraction and returns an empty string for non-ClickHouse adapters, while direct calls may fail on unsupported adapters.

Replace adapter.get_model_query_settings(model) with elementary.get_query_settings() to match the pattern used elsewhere in the codebase.

🤖 Prompt for AI Agents 
In macros/utils/table_operations/delete_and_insert.sql around lines 64-65 (and
also update the similar call at line 72), replace the direct call to
adapter.get_model_query_settings(model) with the dispatch macro
elementary.get_query_settings() so the code uses the adapter-agnostic pattern
used in insert_rows.sql; update both occurrences accordingly to call
elementary.get_query_settings() with the same model argument so ClickHouse gets
its settings and other adapters safely receive an empty string.

Copy link
Contributor Author

@StevenReitsma StevenReitsma Oct 23, 2025

Choose a reason for hiding this comment

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

I don't really see this as an issue since this code is already in ClickHouse-adapter specific code. But can change if wanted from a human reviewer :)

Copy link

@coderabbitai coderabbitai bot Oct 23, 2025

Choose a reason for hiding this comment

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

@StevenReitsma You're absolutely right! I apologize for the overly prescriptive suggestion.

The key distinction is that your code is in the clickhouse__get_delete_and_insert_queries macro, which is already adapter-specific. In adapter-specific macros, directly calling adapter methods like adapter.get_model_query_settings(model) is perfectly reasonable and arguably more direct.

The dispatch pattern (elementary.get_query_settings()) is most valuable in generic/cross-adapter code where you need a single implementation to work across multiple database types. In insert_rows.sql, the dispatch is used in the base get_insert_rows_queries macro (not an adapter override), which is why the pattern makes sense there.

Your current approach is clean and appropriate for adapter-specific code. No changes needed here! 👍

{% endset %}
{% do queries.append(delete_query) %}
{% endif %}

{% if insert_relation %}
{% set insert_query %}
insert into {{ relation }} select * from {{ insert_relation }};
insert into {{ relation }} {{ adapter.get_model_query_settings(model) }} select * from {{ insert_relation }};
{% endset %}
{% do queries.append(insert_query) %}
{% endif %}
Expand Down
4 changes: 3 additions & 1 deletion macros/utils/table_operations/insert_rows.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,9 @@
insert into {{ table_relation }}
({%- for column in columns -%}
{{- elementary.escape_reserved_keywords(column.name) -}} {{- "," if not loop.last else "" -}}
{%- endfor -%}) values
{%- endfor -%})
{{ elementary.get_query_settings() }}
values
{% endset %}
{% do elementary.end_duration_measure_context('base_query_calc') %}

Expand Down
4 changes: 3 additions & 1 deletion macros/utils/table_operations/make_temp_relation.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@
identifier=tmp_identifier,
schema=base_relation.schema,
database=base_relation.database,
type='table') %}
type='table',
can_on_cluster=base_relation.can_on_cluster,
can_exchange=base_relation.can_exchange) %}
{% do return(tmp_relation) %}
{% endmacro %}
Loading