Fix null value on bytestring columns

[vivienm]

When client setting strings_as_bytes is set, the driver crashes when inserting None values into columns with type Nullable([Fixed]String):

  File "clickhouse_driver/client.py", line 142, in execute
    query_id=query_id, types_check=types_check
  File "clickhouse_driver/client.py", line 246, in process_insert_query
    self.send_data(sample_block, data, types_check=types_check)
  File "clickhouse_driver/client.py", line 270, in send_data
    self.connection.send_data(block)
  File "clickhouse_driver/connection.py", line 429, in send_data
    self.block_out.write(block)
  File "clickhouse_driver/streams/native.py", line 41, in write
    self.fout, types_check=block.types_check)
  File "clickhouse_driver/columns/service.py", line 84, in write_column
    column.write_data(items, buf)
  File "clickhouse_driver/columns/base.py", line 79, in write_data
    self._write_data(items, buf)
  File "clickhouse_driver/columns/base.py", line 83, in _write_data
    self.write_items(prepared, buf)
  File "clickhouse_driver/columns/stringcolumn.py", line 120, in write_items
    items_buf_view[buf_pos:buf_pos + min(length, value_len)] = value
TypeError: a bytes-like object is required, not 'str'

This is likely because the method ByteString.prepare_null is simply String.prepare_null, and returns the empty string '' instead the empty bytes b''.
I've been able to fix it with this PR, but I'm not familiar at all with this project so maybe it's not the right way to do it. Let me know if I can help :)

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling d64929e on vivienm:master into 293b596 on mymarilyn:master.

[coveralls]

Coverage increased (+0.005%) to 95.743% when pulling c6b6a2a on vivienm:master into 293b596 on mymarilyn:master.

[xzkostyan]

Hi, @vivienm!

Great fix. Can you write test to cover this issue?

There is an example of test for Nullable(String).

[vivienm]

Hi @xzkostyan, thanks for your feedback! I've updated the PR with a couple of unit tests.

[vivienm]

This test fails with

FAIL: test_nullable (tests.columns.test_fixedstring.ByteFixedStringTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/mymarilyn/clickhouse-driver/tests/columns/test_fixedstring.py", line 148, in test_nullable
    self.assertEqual(inserted, data)
AssertionError: Lists differ: [(None,), ('test\x00\x00\x00\x... != [(None,), ('test',), (None,), ...
First differing element 1:
('test\x00\x00\x00\x00\x00\x00',)
('test',)
- [(None,), ('test\x00\x00\x00\x00\x00\x00',), (None,), ('nullable\x00\x00',)]
+ [(None,), ('test',), (None,), ('nullable',)]

Null characters are rstripped in FixedString columns, whereas they are not in ByteFixedString columns. I've tried to strip them in ByteFixedStrings too:

diff --git i/clickhouse_driver/columns/stringcolumn.py w/clickhouse_driver/columns/stringcolumn.py
index ff13ffc..5e037d4 100644
--- i/clickhouse_driver/columns/stringcolumn.py
+++ w/clickhouse_driver/columns/stringcolumn.py
@@ -104,7 +104,8 @@ class ByteFixedString(FixedString):
         buf_pos = 0

         for i in compat.range(n_items):
-            items[i] = items_buf_view[buf_pos:buf_pos + length].tobytes()
+            items[i] = items_buf_view[buf_pos:buf_pos + length].tobytes() \
+                .rstrip(b'\x00')
             buf_pos += length

         return items

but then another test is broken:

FAIL: test_not_decoded (tests.columns.test_fixedstring.ByteFixedStringTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "clickhouse-driver/tests/columns/test_fixedstring.py", line 127, in test_not_decoded
    ('test'.encode('cp1251') + b'\x00' * 4, )
AssertionError: Lists differ: [(b'\xff\xed\xe4\xe5\xea\xf1',), (b'test',)] != [(b'\xff\xed\xe4\xe5\xea\xf1\x00\x00',), (b'test\x00\x00\x00\x00',)]

First differing element 0:
(b'\xff\xed\xe4\xe5\xea\xf1',)
(b'\xff\xed\xe4\xe5\xea\xf1\x00\x00',)

- [(b'\xff\xed\xe4\xe5\xea\xf1',), (b'test',)]
+ [(b'\xff\xed\xe4\xe5\xea\xf1\x00\x00',), (b'test\x00\x00\x00\x00',)]
?                             ++++++++            ++++++++++++++++

[xzkostyan]

Hi, @vivienm.

Good job!

Zero bytes are not stripped during FixedString column retrieving when 'strings_as_bytes' is set to True. You should correct last assertation statement in tests.columns.test_fixedstring.ByteFixedStringTestCase.test_nullable to something like this:

            inserted = self.client.execute(query)
            self.assertEqual(inserted, [
                (None, ),
                (b'test\x00\x00\x00\x00\x00\x00', ),
                (None, ),
                (b'nullable\x00\x00',)
            ])

See example in test_not_decoded test.

[vivienm]

Hi @xzkostyan, thanks for your help! I've patched the test as indicated, it seems to work now.