Merge pull request #367 from shachibista/filter-settings-by-engine

Filter settings based on Engine
ClickHouse · Jan 9, 2025 · 67bb498 · 67bb498
2 parents 82a405e + e299ecd
commit 67bb498
Show file tree

Hide file tree

Showing 5 changed files with 33 additions and 6 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,4 +1,7 @@
-### Next Version
+### Release [x.x.x]
+### Improvements
+* Ignores incompatible settings based on the configured Engine.
+
 
 #### New Features
 * [ClickHouse indexes](https://clickhouse.com/docs/en/optimize/sparse-primary-indexes) are now fully supported for `table` materialization.

diff --git a/dbt/adapters/clickhouse/impl.py b/dbt/adapters/clickhouse/impl.py
@@ -47,6 +47,11 @@
 GET_CATALOG_MACRO_NAME = 'get_catalog'
 LIST_SCHEMAS_MACRO_NAME = 'list_schemas'
 
+IGNORED_SETTINGS = {
+    'Memory': ['replicated_deduplication_window'],
+    'S3': ['replicated_deduplication_window'],
+}
+
 
 @dataclass
 class ClickHouseConfig(AdapterConfig):
@@ -456,12 +461,13 @@ def run_sql_for_tests(self, sql, fetch, conn):
             conn.state = 'close'
 
     @available
-    def get_model_settings(self, model):
+    def get_model_settings(self, model, engine='MergeTree'):
         settings = model['config'].get('settings', {})
         materialization_type = model['config'].get('materialized')
         conn = self.connections.get_if_exists()
         conn.handle.update_model_settings(settings, materialization_type)
         res = []
+        settings = self.filter_settings_by_engine(settings, engine)
         for key in settings:
             res.append(f' {key}={settings[key]}')
         settings_str = '' if len(res) == 0 else 'SETTINGS ' + ', '.join(res) + '\n'
@@ -470,6 +476,24 @@ def get_model_settings(self, model):
                     {settings_str}
                     """
 
+    @available
+    def filter_settings_by_engine(self, settings, engine):
+        filtered_settings = {}
+
+        if engine.endswith('MergeTree'):
+            # Special case for MergeTree due to all its variations.
+            ignored_settings = IGNORED_SETTINGS.get('MergeTree', [])
+        else:
+            ignored_settings = IGNORED_SETTINGS.get(engine, [])
+
+        for key, value in settings.items():
+            if key in ignored_settings:
+                logger.warning(f"Setting {key} not available for engine {engine}, ignoring.")
+            else:
+                filtered_settings[key] = value
+
+        return filtered_settings
+
     @available
     def get_model_query_settings(self, model):
         settings = model['config'].get('query_settings', {})

diff --git a/dbt/include/clickhouse/macros/materializations/distributed_table.sql b/dbt/include/clickhouse/macros/materializations/distributed_table.sql
@@ -127,7 +127,7 @@
   {{ order_cols(label="order by") }}
   {{ primary_key_clause(label="primary key") }}
   {{ partition_cols(label="partition by") }}
-  {{ adapter.get_model_settings(model) }}
+  {{ adapter.get_model_settings(model, config.get('engine', default='MergeTree')) }}
 {%- endmacro %}
 
 {% macro create_distributed_local_table(distributed_relation, shard_relation, structure_relation, sql_query=none) -%}

diff --git a/dbt/include/clickhouse/macros/materializations/table.sql b/dbt/include/clickhouse/macros/materializations/table.sql
@@ -189,7 +189,7 @@
     {% if temporary -%}
         create temporary table {{ relation }}
         engine Memory
-        {{ adapter.get_model_settings(model) }}
+        {{ adapter.get_model_settings(model, 'Memory') }}
         as (
           {{ sql }}
         )
@@ -205,7 +205,7 @@
         {{ primary_key_clause(label="primary key") }}
         {{ partition_cols(label="partition by") }}
         {{ ttl_config(label="ttl")}}
-        {{ adapter.get_model_settings(model) }}
+        {{ adapter.get_model_settings(model, config.get('engine', default='MergeTree')) }}
 
         {%- if not has_contract %}
           {%- if not adapter.is_before_version('22.7.1.2484') %}

diff --git a/dbt/include/clickhouse/macros/materializations/view.sql b/dbt/include/clickhouse/macros/materializations/view.sql
@@ -81,7 +81,7 @@
     {{ adapter.get_model_query_settings(model) }}
   )
       {% if model.get('config').get('materialized') == 'view' %}
-      {{ adapter.get_model_settings(model) }}
+      {{ adapter.get_model_settings(model, config.get('engine', default='MergeTree')) }}
     {%- endif %}
 
 {%- endmacro %}