-
Notifications
You must be signed in to change notification settings - Fork 92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support of float32 type #1113
Conversation
@@ -466,6 +466,27 @@ def test_w_float_str(self): | |||
|
|||
self.assertEqual(self._callFUT(value_pb, field_type), expected_value) | |||
|
|||
def test_w_float32(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per FLOAT32 design, the request from client is sent as number_value
field. What will happen when the input is float("-inf")
or float("nan")
which gets encoded to string_value
field and the column in table is of type FLOAT32?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to verify both insert and read with this kind of data when column type is FLOAT32.
For read we already have an integration test for FLOAT64 test_execute_sql_w_float_bindings_transfinite
. We can copy it for FLOAT32.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
behaviour for float("-inf")
or float("nan")
is same as FLOAT64, added the skipped test case for manual verification.
@@ -228,6 +228,11 @@ def _parse_value_pb(value_pb, field_type): | |||
return float(value_pb.string_value) | |||
else: | |||
return value_pb.number_value | |||
elif type_code == TypeCode.FLOAT32: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have integration tests for every type in test_session_api.py. Can we have a test for FLOAT32
?
Reference: test_execute_sql_w_float64_bindings
and test_execute_sql_w_float_bindings_transfinite
We should register the same merge method for
|
@ankiaga This query should give you a good indication of what changes are needed for |
@@ -90,5 +90,8 @@ def _check_cell_data(found_cell, expected_cell, recurse_into_lists=True): | |||
for found_item, expected_item in zip(found_cell, expected_cell): | |||
_check_cell_data(found_item, expected_item) | |||
|
|||
elif isinstance(found_cell, float): | |||
assert abs(found_cell - expected_cell) < 0.00001 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕