[RFC] Do not require settings declaration if server support setting-as-string

clickhouse_driver/settings/types.py

@@ -1,58 +1,46 @@
 from ..util.compat import asbool
 from ..varint import write_varint
-from ..writer import write_binary_str, write_binary_uint8
+from ..writer import write_binary_str
 
 
 class SettingType(object):
     @classmethod
-    def write_is_important(cls, buf, as_string):
-        if as_string:
-            write_binary_uint8(0, buf)
+    def write(cls, value, buf):
+        cls.write_value(value, buf)
 
     @classmethod
-    def write(cls, value, buf, as_string):
-        cls.write_is_important(buf, as_string)
-        cls.write_value(value, buf, as_string)
-
-    @classmethod
-    def write_value(cls, value, buf, as_string):
+    def write_value(cls, value, buf):
         raise NotImplementedError
 
 
 class SettingUInt64(SettingType):
     @classmethod
-    def write_value(cls, value, buf, as_string):
-        if as_string:
-            write_binary_str(str(value), buf)
-        else:
-            write_varint(int(value), buf)
+    def write_value(cls, value, buf):
+        write_varint(int(value), buf)
 
 
 class SettingBool(SettingType):
     @classmethod
-    def write_value(cls, value, buf, as_string):
+    def write_value(cls, value, buf):
         value = asbool(value)
-        if as_string:
-            write_binary_str(str(int(value)), buf)
-        else:
-            write_varint(value, buf)
+        write_varint(value, buf)
 
 
 class SettingString(SettingType):
     @classmethod
-    def write_value(cls, value, buf, as_string):
+    def write_value(cls, value, buf):
         write_binary_str(value, buf)
 
 
 class SettingChar(SettingType):
     @classmethod
-    def write_value(cls, value, buf, as_string):
+    def write_value(cls, value, buf):
         write_binary_str(value[0], buf)
 
 
 class SettingFloat(SettingType):
     @classmethod
-    def write_value(cls, value, buf, as_string):
+    def write_value(cls, value, buf):
         """
         Float is written in string representation.
         """
@@ -61,10 +49,7 @@ def write_value(cls, value, buf, as_string):
 
 class SettingMaxThreads(SettingUInt64):
     @classmethod
-    def write_value(cls, value, buf, as_string):
-        if as_string:
-            write_binary_str(str(value), buf)
-        else:
-            if value == 'auto':
-                value = 0
-            super(SettingMaxThreads, cls).write_value(value, buf, as_string)
+    def write_value(cls, value, buf):
+        if value == 'auto':
+            value = 0
+        super(SettingMaxThreads, cls).write_value(value, buf)

clickhouse_driver/settings/writer.py

@@ -1,6 +1,6 @@
 import logging
 
-from ..writer import write_binary_str
+from ..writer import write_binary_str, write_binary_uint8
 from .available import settings as available_settings
 
 
@@ -9,13 +9,21 @@
 
 def write_settings(settings, buf, settings_as_strings):
     for setting, value in (settings or {}).items():
-        setting_writer = available_settings.get(setting)
+        # If the server support settings as string we do not need to know
+        # anything about them, so we can write any setting.
+        if settings_as_strings:
+            write_binary_str(setting, buf)
+            write_binary_uint8(0, buf)
+            write_binary_str(str(value), buf)
+            continue
 
+        # If the server requires string in binary, then they cannot be written
+        # without type.
+        setting_writer = available_settings.get(setting)
         if not setting_writer:
-            logger.warning('Unknown setting %s. Skipping', setting)
+            logger.warning('Unknown setting %s. Type is unknown. Skipping', setting)
             continue
-
         write_binary_str(setting, buf)
-        setting_writer.write(value, buf, settings_as_strings)
+        setting_writer.write(value, buf)
 
     write_binary_str('', buf)  # end of settings

tests/test_settings.py

@@ -2,7 +2,6 @@
 from tests.testcase import BaseTestCase
 from tests.util import require_server_version
 
-
 class SettingTestCase(BaseTestCase):
     def test_int_apply(self):
         settings = {'max_query_size': 142}
@@ -25,7 +24,7 @@ def test_float_apply(self):
         self.assertEqual(rv, [('totals_auto_threshold', '1.23', 1)])
 
     def test_bool_apply(self):
-        settings = {'force_index_by_date': 2}
+        settings = {'force_index_by_date': 1}
 
         rv = self.client.execute(
             "SELECT name, value, changed FROM system.settings "
@@ -36,7 +35,7 @@ def test_bool_apply(self):
 
     @require_server_version(1, 1, 54388)
     def test_char_apply(self):
-        settings = {'format_csv_delimiter': 'delimiter'}
+        settings = {'format_csv_delimiter': 'd'}
 
         rv = self.client.execute(
             "SELECT name, value, changed FROM system.settings "
@@ -64,8 +63,14 @@ def test_max_threads_apply(self):
         )
 
     def test_unknown_setting(self):
+        # For both cases unknown setting will be ignored:
+        # - rev >= DBMS_MIN_REVISION_WITH_SETTINGS_SERIALIZED_AS_STRINGS
+        #   the setting will be ignored by the server with the warning message
+        #   (since clickhouse-server does not ignore only important settings,
+        #   the one that has important flag)
+        # - otherwise the unknown setting will be ignored by the driver.
         settings = {'unknown_setting': 100500}
-        self.client.execute('SHOW tables', settings=settings)
+        self.client.execute('SELECT 1', settings=settings)
 
     def test_client_settings(self):
         settings = {'max_query_size': 142}