cccs-eric commented on a change in pull request #3988:
URL: https://github.com/apache/iceberg/pull/3988#discussion_r793647114
##########
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:
I'm not sure what we want to express here. Here's what I think:
```Python
raise TypeError("Cannot deserialize Type: %s" % type_var)
```
If we want to express that we could not deserialize a type, it makes sense
that the error specifies which type it is by echoing what was passed to the
function. It could not deserialize **this** type and not a TypeID. The dev
never asked for a TypeID to be deserialized...
```Python
raise TypeError("Cannot deserialize Type: %s" % type_var.type_id)
```
If we want to use `type_id` in the message, we should say that we couldn't
find a converter or a deserializer for this TypeID.
I think what is causing confusion is that `to_byte_buffer()` and
`from_byte_buffer()` don't align... But maybe it's just me 😄 That's my
2cents, let me know what you prefer.
--
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]
