Skip to content

Commit

Permalink
Add ability to mark settings as important to fail on unknown settings
Browse files Browse the repository at this point in the history
Skipping unknown settings is not always the required behaviour, and most
of the time you want the opposite - fail on unknown setting.

Image that you pass some new setting, i.e. max_insert_threads, and you
assume that the INSERT will be parallel, however due to you server was
too old the setting is silently ignored.

Refs: mymarilyn#142
  • Loading branch information
azat committed Nov 25, 2020
1 parent 08b63ab commit d78a075
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 5 deletions.
3 changes: 3 additions & 0 deletions clickhouse_driver/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,9 @@ def from_url(cls, url):
elif name == 'compress_block_size':
kwargs[name] = int(value)

elif name == 'settings_is_important':
kwargs[name] = int(value)

# ssl
elif name == 'verify':
kwargs[name] = asbool(value)
Expand Down
9 changes: 7 additions & 2 deletions clickhouse_driver/connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,9 @@ class Connection(object):
:param ciphers: see :func:`ssl.wrap_socket` docs.
:param alt_hosts: list of alternative hosts for connection.
Example: alt_hosts=host1:port1,host2:port2.
:param settings_is_important: 0 means unknown settings will be ignored,
1 means that the query will fail with
UNKNOWN_SETTING error.
"""

def __init__(
Expand All @@ -124,7 +127,8 @@ def __init__(
secure=False,
# Secure socket parameters.
verify=True, ssl_version=None, ca_certs=None, ciphers=None,
alt_hosts=None
alt_hosts=None,
settings_is_important=0,
):
if secure:
default_port = defines.DEFAULT_SECURE_PORT
Expand All @@ -145,6 +149,7 @@ def __init__(
self.connect_timeout = connect_timeout
self.send_receive_timeout = send_receive_timeout
self.sync_request_timeout = sync_request_timeout
self.settings_is_important = settings_is_important

self.secure_socket = secure
self.verify_cert = verify
Expand Down Expand Up @@ -563,7 +568,7 @@ def send_query(self, query, query_id=None):
revision >= defines
.DBMS_MIN_REVISION_WITH_SETTINGS_SERIALIZED_AS_STRINGS
)
write_settings(self.context.settings, self.fout, settings_as_strings)
write_settings(self.context.settings, self.fout, settings_as_strings, self.settings_is_important)

if revision >= defines.DBMS_MIN_REVISION_WITH_INTERSERVER_SECRET:
write_binary_str('', self.fout)
Expand Down
4 changes: 1 addition & 3 deletions clickhouse_driver/settings/writer.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@
logger = logging.getLogger(__name__)


def write_settings(settings, buf, settings_as_strings):
is_important = 0

def write_settings(settings, buf, settings_as_strings, is_important=0):
for setting, value in (settings or {}).items():
# If the server support settings as string we do not need to know
# anything about them, so we can write any setting.
Expand Down
7 changes: 7 additions & 0 deletions tests/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,13 @@ def test_parameters_cast(self):
c.connection.context.client_settings['insert_block_size'], 123
)

def test_settings_is_important(self):
c = Client.from_url('clickhouse://host?settings_is_important=1')
self.assertEqual(c.connection.settings_is_important, 1)

c = Client.from_url('clickhouse://host?settings_is_important=0')
self.assertEqual(c.connection.settings_is_important, 0)

@check_numpy
def test_use_numpy(self):
c = Client.from_url('clickhouse://host?use_numpy=true')
Expand Down
12 changes: 12 additions & 0 deletions tests/test_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,18 @@ def test_unknown_setting(self):
settings = {'unknown_setting': 100500}
self.client.execute('SELECT 1', settings=settings)

# DBMS_MIN_REVISION_WITH_SETTINGS_SERIALIZED_AS_STRINGS is 20.1.2+
@require_server_version(20, 1, 2)
def test_unknown_setting_is_important(self):
# In case of rev >= DBMS_MIN_REVISION_WITH_SETTINGS_SERIALIZED_AS_STRINGS
# and setting marked as important, then the query should fail.
settings = {'unknown_setting': 100500}
self.client.connection.settings_is_important = True
with self.assertRaises(ServerException) as e:
self.client.execute('SELECT 1', settings=settings)
self.client.connection.settings_is_important = False
self.assertEqual(e.exception.code, ErrorCodes.UNKNOWN_SETTING)

def test_client_settings(self):
settings = {'max_query_size': 142}

Expand Down

0 comments on commit d78a075

Please sign in to comment.