Allow skipping utf8 converison in Python3 #548

mszjat · 2024-08-12T15:33:23Z

csringhofer

Looks good in general, left a few comments.
It would be also nice to have some tests about getting the expected Python types for the different values of this parameter. There are already some similar tests in test_data_types.py

impala/hiveserver2.py

csringhofer · 2024-08-12T16:36:37Z

impala/hiveserver2.py

@@ -1040,7 +1049,7 @@ def __init__(self, trowset, expect_more_rows, schema, convert_types=True):

            # STRING columns are read as binary and decoded here to be able to handle
            # non-valid utf-8 strings in Python 3.
-            if six.PY3:
+            if six.PY3 and convert_strings_to_unicode:


The case when convert_strings_to_unicode is False but convert_types is True also needs to be consider - the code in _convert_values probably expects Unicode when converting DATE/TIMESTAMP/DECIMAL types.

Makes sense, please check out new design.

csringhofer · 2024-08-12T16:38:43Z

impala/hiveserver2.py

@@ -96,6 +97,9 @@ def cursor(self, user=None, configuration=None, convert_types=True,
            When `False`, timestamps and decimal values will not be converted
            to Python `datetime` and `Decimal` values. (These conversions are
            expensive.) Only applies when using HS2 protocol versions > 6.
+        convert_strings_to_unicode : bool, optional
+            When `False`, string values will not be converted to unicode. 


The return type could be mentioned both for the True/False case.

It should be mentioned that this has effect only in Python3.

csringhofer · 2024-08-23T13:14:05Z

impala/hiveserver2.py

@@ -96,6 +97,12 @@ def cursor(self, user=None, configuration=None, convert_types=True,
            When `False`, timestamps and decimal values will not be converted
            to Python `datetime` and `Decimal` values. (These conversions are
            expensive.) Only applies when using HS2 protocol versions > 6.
+        convert_strings_to_unicode : bool, optional
+            When `True`, the following types will be converted to unicode:


It could be mentioned that these are the types that are transmitted as strings in HS2 protocol

csringhofer · 2024-08-23T13:16:47Z

impala/hiveserver2.py

-                self._convert_strings_to_unicode(type_, is_null, values)
+                if convert_strings_to_unicode:
+                    self._convert_strings_to_unicode(type_, is_null, values, 
+                        types=["STRING", "LIST", "MAP", "STRUCT", "UNIONTYPE", "NULL", "VARCHAR", "CHAR", "TIMESTAMP", "DECIMAL", "DATE"])


These type lists could be moved to a constant,e.g HS2_STRING_TYPES and CONVERTED_TYPES

Absolutely, fixed

csringhofer · 2024-08-23T13:18:13Z

impala/tests/conftest.py

+
+@fixture(scope='session')
+def cur_noconv(con):
+    cur = con.cursor(convert_types=True, convert_strings_to_unicode=False)


here and in test_data_types.py: is there a reason for removing the final new lines?

It's by mistake, fixed

csringhofer · 2024-08-23T13:19:21Z

impala/tests/test_data_types.py

+    setup_test_date_basic(cur, date_table)
+
+@pytest.mark.connect
+def test_date_basic_noconv(cur_noconv, date_table):


Please add tests more for types - date is a special case where string conversion is done regardless of convert_strings_to_unicode

csringhofer · 2024-08-23T13:20:00Z

impala/tests/conftest.py

@@ -113,3 +113,9 @@ def cur(con):
    cur = con.cursor()
    yield cur
    cur.close()
+
+@fixture(scope='session')
+def cur_noconv(con):


convert_types is True here, so I think that cur_no_string_conversion or something similar would be better

csringhofer · 2024-08-26T16:16:11Z

impala/tests/test_data_types.py

-
-@pytest.mark.connect
-def test_date_basic(cur, date_table):
+def setup_test_date(cur, date_table):


The naming doesn't make sense to me - this function does a select and checks the result, so it is not a setup function.

csringhofer · 2024-08-26T16:17:25Z

impala/tests/test_data_types.py

    try:
        yield table_name
    finally:
        cur.execute("DROP TABLE {0}".format(table_name))


-@pytest.mark.connect
-def test_timestamp_basic(cur, timestamp_table):
+def setup_test_timestamp(cur, timestamp_table):


Same as for dates, at this point this function is not about setup.

csringhofer · 2024-08-26T16:41:19Z

impala/tests/test_data_types.py

+    setup_test_date(cur, date_table)
+
+@pytest.mark.connect
+def test_date_no_string_conv(cur_no_string_conv, date_table):


Can you also add similar test for decimals?

csringhofer · 2024-08-26T16:43:46Z

impala/tests/test_data_types.py

+@pytest.mark.connect
+def test_utf8_strings_no_string_conv(cur_no_string_conv):
+    cur = cur_no_string_conv
+    """Use STRING/VARCHAR/CHAR values  with multi byte unicode code points in a query."""


I don't think that this complex unicode handling is needed for the "no conversion" case. The main point should be to check that with unicode conversion the type returned is unicode, while without it it is bytes (in Python 3)

csringhofer · 2024-08-29T07:13:25Z

impala/tests/test_data_types.py

@@ -49,6 +51,47 @@ def test_cursor_description_precision_scale(cur, decimal_table):
    for (exp, obs) in zip(expected, observed):
        assert exp == obs

+
+@fixture(scope='module')
+def decimal_table2(cur):


Was the existing decimal_table not enough for the tests?

I found it more consistent with other tests this way, but it's really minor to change, if you insist.

csringhofer · 2024-08-29T07:15:39Z

impala/tests/test_data_types.py

-    assert result.decode("UTF-8", "replace") == u"�"
+
+    if sys.version_info[0] < 3:
+        assert result[0] == u"Test string"


Would this fail if 'u' was not added? Python does automatic conversion of strings in many cases. We should check the exact type with isinstance.

csringhofer · 2024-08-29T12:27:28Z

impala/tests/test_data_types.py

+    """Read back a few decimal values in a wide range."""
+    cur.execute('select val from {0} order by val'.format(decimal_table))
+    results = cur.fetchall()
+    assert results == [(Decimal('-999999999.999999999'),),


nice to have: it would increase the coverage of the tests if there would be also NULL values in these test tables

Agree, added

csringhofer · 2024-08-29T14:51:37Z

impala/tests/test_data_types.py

+    result = cur.fetchone()
+
+    if sys.version_info[0] < 3:
+        assert not isinstance(result[0], unicode)


isinstance with str would be clearer

csringhofer · 2024-08-29T14:52:16Z

impala/tests/test_data_types.py

@@ -49,6 +51,47 @@ def test_cursor_description_precision_scale(cur, decimal_table):
    for (exp, obs) in zip(expected, observed):
        assert exp == obs

+
+@fixture(scope='module')
+def decimal_table2(cur):


Added convert_strings_to_unicode conn option

9acc61f

csringhofer reviewed Aug 12, 2024

View reviewed changes

mszjat added 4 commits August 13, 2024 10:22

convert_strings_to_unicode default True; minor fixes

888dbde

rewised design draft

3938f5f

Added test for convert_strings_to_unicode

afb36fe

Restrict conversion to Python3 only

03e7f3a

mszjat force-pushed the master branch from 1c34280 to 03e7f3a Compare August 13, 2024 14:12

mszjat added 2 commits August 13, 2024 16:57

Fixed syntax

1b8a8a4

Simplified test

c68165a

csringhofer reviewed Aug 23, 2024

View reviewed changes

mszjat added 3 commits August 26, 2024 12:43

Review fixes, minor

66a590e

Added non-converstion test for strings

08cd5d7

Added non-converstion test for timestamps

5bc6155

csringhofer reviewed Aug 26, 2024

View reviewed changes

more tests

c9ed747

csringhofer reviewed Aug 29, 2024

View reviewed changes

mszjat added 2 commits August 29, 2024 16:27

Fixed string tests

be3edb2

Added NULL value to decimal test

7f831e7

csringhofer reviewed Aug 29, 2024

View reviewed changes

Improved python2 test

09816fc

csringhofer approved these changes Aug 29, 2024

View reviewed changes

csringhofer merged commit 6e2ffa8 into cloudera:master Aug 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow skipping utf8 converison in Python3 #548

Allow skipping utf8 converison in Python3 #548

mszjat commented Aug 12, 2024

csringhofer left a comment

csringhofer Aug 12, 2024

mszjat Aug 13, 2024

csringhofer Aug 12, 2024

csringhofer Aug 23, 2024

mszjat Aug 26, 2024

csringhofer Aug 23, 2024

mszjat Aug 26, 2024

csringhofer Aug 23, 2024

mszjat Aug 26, 2024

csringhofer Aug 23, 2024

csringhofer Aug 23, 2024

mszjat Aug 26, 2024

csringhofer Aug 26, 2024

mszjat Aug 26, 2024

csringhofer Aug 26, 2024

mszjat Aug 26, 2024

csringhofer Aug 26, 2024

mszjat Aug 26, 2024

csringhofer Aug 26, 2024

mszjat Aug 26, 2024

csringhofer Aug 29, 2024

mszjat Aug 29, 2024

csringhofer Aug 29, 2024

csringhofer Aug 29, 2024

mszjat Aug 29, 2024

csringhofer Aug 29, 2024

mszjat Aug 29, 2024

csringhofer Aug 29, 2024

mszjat Aug 29, 2024

csringhofer Aug 29, 2024

Allow skipping utf8 converison in Python3 #548

Allow skipping utf8 converison in Python3 #548

Conversation

mszjat commented Aug 12, 2024

csringhofer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment