[GitHub] [iceberg] samredai commented on a change in pull request #3988: Python: Add Decimal type conversion

[GitBox <gi...@apache.org>]

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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org