Skip to content

Commit

Permalink
Correctly warn or error if light weight deletes not available
Browse files Browse the repository at this point in the history
  • Loading branch information
genzgd authored and Savid committed Jan 17, 2024
1 parent 520004c commit 297aa80
Show file tree
Hide file tree
Showing 6 changed files with 68 additions and 24 deletions.
9 changes: 9 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
### Release [1.7.1], TBD
#### Bug Fix
- It was possible for incremental models with the delete+insert strategy to fail if ClickHouse "light weight deletes" were
not enabled or the required setting `allow_nondetermistic_mutations` was not enabled and the user did not have permission
to apply it. This condition is now detected on startup, and an exception will be thrown if `use_lw_deletes` is configured
in the profile. Otherwise, a warning will be logged that incremental models will be slower (because such models will
be downgraded to use the `legacy` incremental strategy). This should prevent the confusing behavior in
https://github.com/ClickHouse/dbt-clickhouse/issues/197 by throwing an early exception for an unsupported configuration.

### Release [1.7.0], 2023-12-07
#### Improvements
- Minimal compatibility with dbt 1.7.x. The date_spine macro and additional automated tests have not been implemented,
Expand Down
2 changes: 1 addition & 1 deletion dbt/adapters/clickhouse/__version__.py
Original file line number Diff line number Diff line change
@@ -1 +1 @@
version = '1.7.0'
version = '1.7.1'
54 changes: 34 additions & 20 deletions dbt/adapters/clickhouse/dbclient.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,15 @@
from abc import ABC, abstractmethod
from typing import Dict

from dbt.exceptions import DbtDatabaseError, FailedToConnectError
from dbt.exceptions import DbtConfigError, DbtDatabaseError, FailedToConnectError

from dbt.adapters.clickhouse.credentials import ClickHouseCredentials
from dbt.adapters.clickhouse.errors import (
lw_deletes_not_enabled_error,
lw_deletes_not_enabled_warning,
nd_mutations_not_enabled_error,
nd_mutations_not_enabled_warning,
)
from dbt.adapters.clickhouse.logger import logger
from dbt.adapters.clickhouse.query import quote_identifier
from dbt.adapters.clickhouse.util import compare_versions
Expand Down Expand Up @@ -131,34 +137,42 @@ def update_model_settings(self, model_settings: Dict[str, str]):
model_settings[key] = value

def _check_lightweight_deletes(self, requested: bool):
lw_deletes = self.get_ch_setting(LW_DELETE_SETTING)
nd_mutations = self.get_ch_setting(ND_MUTATION_SETTING)
lw_deletes, lw_read_only = self.get_ch_setting(LW_DELETE_SETTING)
nd_mutations, nd_mutations_read_only = self.get_ch_setting(ND_MUTATION_SETTING)
if lw_deletes is None or nd_mutations is None:
if requested:
logger.warning(
'use_lw_deletes requested but are not available on this ClickHouse server'
)
logger.warning(lw_deletes_not_enabled_error)
return False, False
lw_deletes = int(lw_deletes) > 0
if not lw_deletes:
try:
self.command(f'SET {LW_DELETE_SETTING} = 1')
self._conn_settings[LW_DELETE_SETTING] = '1'
lw_deletes = True
except DbtDatabaseError:
pass
if lw_read_only:
lw_deletes = False
if requested:
raise DbtConfigError(lw_deletes_not_enabled_error)
logger.warning(lw_deletes_not_enabled_warning)
else:
try:
self.command(f'SET {LW_DELETE_SETTING} = 1')
self._conn_settings[LW_DELETE_SETTING] = '1'
lw_deletes = True
except DbtDatabaseError:
logger.warning(lw_deletes_not_enabled_warning)
nd_mutations = int(nd_mutations) > 0
if lw_deletes and not nd_mutations:
try:
self.command(f'SET {ND_MUTATION_SETTING} = 1')
self._conn_settings[ND_MUTATION_SETTING] = '1'
nd_mutations = True
except DbtDatabaseError:
pass
if nd_mutations_read_only:
nd_mutations = False
if requested:
raise DbtConfigError(nd_mutations_not_enabled_error)
logger.warning(nd_mutations_not_enabled_warning)
else:
try:
self.command(f'SET {ND_MUTATION_SETTING} = 1')
self._conn_settings[ND_MUTATION_SETTING] = '1'
nd_mutations = True
except DbtDatabaseError:
logger.warning(nd_mutations_not_enabled_warning)
if lw_deletes and nd_mutations:
return True, requested
if requested:
logger.warning('use_lw_deletes requested but cannot enable on this ClickHouse server')
return False, False

def _ensure_database(self, database_engine, cluster_name) -> None:
Expand Down
21 changes: 21 additions & 0 deletions dbt/adapters/clickhouse/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,24 @@
Source columns not in target: {0}
"""

lw_deletes_not_enabled_error = """
Attempting to apply the configuration `use_lw_deletes` to enable the delete+insert incremental strategy, but
`light weight deletes` are either not available or not enabled on this ClickHouse server.
"""

lw_deletes_not_enabled_warning = """
`light weight deletes` are either not available or not enabled on this ClickHouse server. This prevents the use
of the delete+insert incremental strategy, which may negatively affect performance for incremental models.
"""

nd_mutations_not_enabled_error = """
Attempting to apply the configuration `use_lw_deletes` to enable the delete+insert incremental strategy, but
the required `allow_nondeterministic_mutations` is not enabled and is `read_only` for this user
"""

nd_mutations_not_enabled_warning = """
The setting `allow_nondeterministic_mutations` is not enabled and is `read_only` for this user` This prevents the use
of `light weight deletes` and therefore the delete+insert incremental strategy. This may negatively affect performance
for incremental models
"""
2 changes: 1 addition & 1 deletion dbt/adapters/clickhouse/httpclient.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ def columns_in_query(self, sql: str, **kwargs) -> List[ClickHouseColumn]:

def get_ch_setting(self, setting_name):
setting = self._client.server_settings.get(setting_name)
return setting.value if setting else None
return (setting.value, setting.readonly) if setting else (None, 0)

def database_dropped(self, database: str):
# This is necessary for the http client to avoid exceptions when ClickHouse doesn't recognize the database
Expand Down
4 changes: 2 additions & 2 deletions dbt/adapters/clickhouse/nativeclient.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,12 @@ def columns_in_query(self, sql: str, **kwargs) -> List[ClickHouseColumn]:
def get_ch_setting(self, setting_name):
try:
result = self._client.execute(
f"SELECT value FROM system.settings WHERE name = '{setting_name}'"
f"SELECT value, readonly FROM system.settings WHERE name = '{setting_name}'"
)
except clickhouse_driver.errors.Error as ex:
logger.warn('Unexpected error retrieving ClickHouse server setting', ex)
return None
return result[0][0] if result else None
return (result[0][0], result[0][1]) if result else (None, 0)

def close(self):
self._client.disconnect()
Expand Down

0 comments on commit 297aa80

Please sign in to comment.