samredai commented on a change in pull request #3988:
URL: https://github.com/apache/iceberg/pull/3988#discussion_r793193421
##########
File path: python_legacy/iceberg/api/types/conversions.py
##########
@@ -23,6 +23,14 @@
from .type import TypeID
+def decimal_to_bytes(type_id, value):
Review comment:
There's a
[type_util.py](https://github.com/apache/iceberg/blob/master/python_legacy/iceberg/api/types/type_util.py)
file. It makes sense to put this in there and import it here.
##########
File path: python_legacy/iceberg/api/types/conversions.py
##########
@@ -23,6 +23,14 @@
from .type import TypeID
+def decimal_to_bytes(type_id, value):
Review comment:
I see that you added `type_id` since you needed to match the other
mappings, but since it's unused it might be helpful to signify that with an
underscore. `def decimal_to_bytes(_, value):`.
##########
File path: python_legacy/iceberg/api/types/conversions.py
##########
@@ -90,11 +99,7 @@ def to_byte_buffer(type_id, value):
@staticmethod
def from_byte_buffer(type_var, buffer_var):
- return Conversions.internal_from_byte_buffer(type_var.type_id,
buffer_var)
-
- @staticmethod
- def internal_from_byte_buffer(type_id, buffer_var):
try:
- return Conversions.from_byte_buff_mapping.get(type_id)(type_id,
buffer_var)
+ return
Conversions.from_byte_buff_mapping.get(type_var.type_id)(type_var, buffer_var)
Review comment:
This will never hit the `KeyError` and will instead hit a similar error
to what you originally saw that won't be caught by the except block. Instead of
using `get`, I think it should be a normal dictionary lookup using square
brackets:
```py
return Conversions.from_byte_buff_mapping[type_var.type_id](type_var,
buffer_var)
```
##########
File path: python_legacy/iceberg/api/types/conversions.py
##########
@@ -90,11 +99,7 @@ def to_byte_buffer(type_id, value):
@staticmethod
def from_byte_buffer(type_var, buffer_var):
- return Conversions.internal_from_byte_buffer(type_var.type_id,
buffer_var)
-
- @staticmethod
- def internal_from_byte_buffer(type_id, buffer_var):
try:
- return Conversions.from_byte_buff_mapping.get(type_id)(type_id,
buffer_var)
+ return
Conversions.from_byte_buff_mapping.get(type_var.type_id)(type_var, buffer_var)
except KeyError:
- raise TypeError("Cannot deserialize Type: %s" % type_id)
+ raise TypeError("Cannot deserialize Type: %s" % type_var)
Review comment:
Should this be `type_var.type_id` instead of `type_var`? And maybe an
f-string: `f"Cannot deserialize Type: {type_var.type_id}"`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
