Support `None` values in array, map and row types #281

mdesmet · 2022-11-04T21:21:19Z

Description

Fixes #269

Also refactors mappers from lambda functions to separate classes for better readability.

Non-technical explanation

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

* Fix some things. ({issue}`issuenumber`)

hashhar

LGTM % comment.

I did not comment on code that was just refactored even though they would have been majority of my comments. I'll submit separate PR with some suggestions in that area.

hashhar · 2022-11-05T09:44:16Z

trino/client.py

@@ -295,7 +295,7 @@ def __init__(

    def retry(self, func, args, kwargs, err, attempt):
        delay = self._get_delay(attempt)
-        time.sleep(delay)
+        sleep(delay)


avoid unrelated changes in single commit

hashhar · 2022-11-05T09:49:53Z

trino/client.py

+        if value == 'Infinity':
+            return INF
+        if value == '-Infinity':
+            return NEGATIVE_INF
+        if value == 'NaN':
+            return NAN


Inline all these constants for INF, NEGATIVE_INF and NAN. Not used in multiple places.

hashhar · 2022-11-05T09:51:27Z

trino/client.py

+        }
+
+
+class RowMapperFactory:


Keep NoOpRowMapper to be below RowMapperFactory (or just above).

hashhar · 2022-11-05T09:52:54Z

trino/client.py

-    and returns a RowMapper instance which will process rows of data
-    """
-    no_op_row_mapper = NoOpRowMapper()
+T = TypeVar("T")


I'd argue type args should be defined up top with imports since over time they get reused across entire class so there's no point trying to "keep them close to where they are used".

hashhar · 2022-11-05T10:37:29Z

trino/client.py

+    def map(self, values: List[Any]) -> Optional[Tuple[Optional[Any], ...]]:
+        if values is None:
+            return None
+        return tuple(self.mappers[idx].map(value) for idx, value in enumerate(values))


idx is no better than i. If you want longer name index is much better.

hashhar · 2022-11-05T10:42:12Z

@ebyhr PTAL. Please avoid leaving comments on code that was moved from existing code - out of scope for this PR and I'm submitting another PR on top of this with some cleanups. will request a review there.

ebyhr

Left only minor comments.

ebyhr · 2022-11-09T01:02:50Z

tests/integration/test_types_integration.py

@@ -156,6 +156,8 @@ def test_array(trino_connection):
    SqlTest(trino_connection) \
        .add_field(sql="CAST(null AS ARRAY(VARCHAR))", python=None) \
        .add_field(sql="ARRAY['a', 'b', null]", python=['a', 'b', None]) \
+        .add_field(sql="ARRAY[1.2, 2.4, null]", python=[Decimal("1.2"), Decimal("2.4"), None]) \


"Refactor value mappers to separate classes"

"Refactoring" commit should focus on refactoring. It would be better to change the commit title.

ebyhr · 2022-11-09T01:17:50Z

tests/integration/test_types_integration.py

+        .add_field(sql="TIME '01:23:45.123'", python=time_3) \
+        .add_field(sql="CAST('01:23:45.123' AS TIME(3))", python=time_3) \


What we want to test is type mapping between Python client and Trino server, not literal and cast in Trino server. I suppose we can remove either one. If we still want to test both, test_date misses the cast coverage.

It is indeed useless. I have removed the casts also we should not test precision of null values so I kept only one null test for each data type.

mdesmet · 2022-11-09T13:22:20Z

I made some further modifications:

simplified the code to only apply rounding if the input value exceeds Python's supported precision.
Removed usage of regex for performance
Usage of fromisoformat instead of strptotime to get a 54x boost in performance
Rounding using Decimal

hashhar · 2022-11-10T08:21:00Z

trino/client.py

-        self.pattern = pattern
-        self.time_size = 9 + ms_size - ms_to_trim
+def round_millis(value: str, precision: int) -> str:
+    if len(value) > precision:


This makes assumption that input is a non-fractional number? Let's defensively verify our assumption. Nothing prevents the caller from using the function in other contexts in future.

hashhar · 2022-11-10T08:42:02Z

trino/client.py

-        self.time_size = 9 + ms_size - ms_to_trim
+def round_millis(value: str, precision: int) -> str:
+    if len(value) > precision:
+        return str(round(Decimal(value) / Decimal(10 ** (len(value) - precision))))


It seems to perform half-odd rounding while docs say round does half-even. (https://docs.python.org/3/library/functions.html#round)

if two multiples are equally close, rounding is done toward the even choice

In [18]: round(Decimal('2.5')) Out[18]: 2 In [19]: round(Decimal('3.5')) Out[19]: 4

In [23]: round_millis('1234585', 6) Out[23]: '123458' In [24]: round_millis('1234575', 6) Out[24]: '123458'

Also what if the value rounds up past 1 second? Receiver needs to perform the carry.

e.g. round_millis('9999999', 6) -> '1000000'.

hashhar · 2022-11-10T08:42:22Z

trino/client.py

-            pattern += ".%f"
-        self.pattern = pattern
-        self.time_size = 9 + ms_size - ms_to_trim
+def round_millis(value: str, precision: int) -> str:


It'll be unit-testable and reusable if it returns int and conversion to str is left to caller.

hashhar · 2022-11-10T08:52:43Z

trino/client.py

+
+class TimeValueMapper(ValueMapper[time]):
+    def __init__(self, precision):
+        self.time_default_size = 9  # size of 'HH:MM:SS.' (the time string up to the milliseconds)


Why include the . in the length. I assume to make it easier to use slicing?

hashhar

LGTM. Timestamp changes have been extracted.

cla-bot bot added the cla-signed label Nov 4, 2022

hashhar approved these changes Nov 5, 2022

View reviewed changes

Avoid name clash with datetime time

fa4765c

mdesmet force-pushed the bug/none-values-experimental-python-types branch from 69af6a3 to 1898a2c Compare November 8, 2022 17:51

ebyhr approved these changes Nov 9, 2022

View reviewed changes

mdesmet added 2 commits November 9, 2022 10:39

Refactor value mappers to separate classes

67c5aa0

Fix None support in structural types

4649f02

mdesmet force-pushed the bug/none-values-experimental-python-types branch from 7ee9f2c to e8cb004 Compare November 9, 2022 11:45

mdesmet force-pushed the bug/none-values-experimental-python-types branch from e8cb004 to 35a6fa6 Compare November 9, 2022 20:24

hashhar reviewed Nov 10, 2022

View reviewed changes

mdesmet force-pushed the bug/none-values-experimental-python-types branch from 35a6fa6 to 4649f02 Compare November 10, 2022 09:34

hashhar approved these changes Nov 10, 2022

View reviewed changes

hashhar merged commit dae4c41 into trinodb:master Nov 10, 2022

hashhar mentioned this pull request Nov 10, 2022

Release notes for 0.320.0 #283

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `None` values in array, map and row types #281

Support `None` values in array, map and row types #281

mdesmet commented Nov 4, 2022

hashhar left a comment

hashhar Nov 5, 2022

hashhar Nov 5, 2022

hashhar Nov 5, 2022

hashhar Nov 5, 2022

hashhar Nov 5, 2022

hashhar commented Nov 5, 2022

ebyhr left a comment

ebyhr Nov 9, 2022

mdesmet Nov 9, 2022

ebyhr Nov 9, 2022

mdesmet Nov 9, 2022

mdesmet commented Nov 9, 2022

hashhar Nov 10, 2022

hashhar Nov 10, 2022

hashhar Nov 10, 2022

hashhar Nov 10, 2022

hashhar left a comment

		.add_field(sql="TIME '01:23:45.123'", python=time_3) \
		.add_field(sql="CAST('01:23:45.123' AS TIME(3))", python=time_3) \

Support None values in array, map and row types #281

Support None values in array, map and row types #281

Conversation

mdesmet commented Nov 4, 2022

Description

Non-technical explanation

Release notes

hashhar 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

hashhar commented Nov 5, 2022

ebyhr 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

mdesmet commented Nov 9, 2022

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

hashhar left a comment

Choose a reason for hiding this comment

Support `None` values in array, map and row types #281

Support `None` values in array, map and row types #281