You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/06/03 15:12:40 UTC
[GitHub] [iceberg] jun-he opened a new pull request #2672: [Python] Update to_byte_buff_mapping and from_byte_buff_mapping
jun-he opened a new pull request #2672:
URL: https://github.com/apache/iceberg/pull/2672
Update to_byte_buff_mapping and from_byte_buff_mapping to be consistent and add additional tests.
--
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.
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
[GitHub] [iceberg] rymurr commented on a change in pull request #2672: [Python] Update to_byte_buff_mapping and from_byte_buff_mapping
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #2672:
URL: https://github.com/apache/iceberg/pull/2672#discussion_r645443513
##########
File path: python/iceberg/api/types/conversions.py
##########
@@ -42,34 +42,34 @@ class Conversions(object):
to_byte_buff_mapping = {TypeID.BOOLEAN: lambda type_id, value: struct.pack("<h", 1 if value else 0),
TypeID.INTEGER: lambda type_id, value: struct.pack("<i", value),
TypeID.DATE: lambda type_id, value: struct.pack("<i", value),
- TypeID.LONG: lambda type_id, value: struct.pack("<l", value),
- TypeID.TIME: lambda type_id, value: struct.pack("<l", value),
- TypeID.TIMESTAMP: lambda type_id, value: struct.pack("<l", value),
+ TypeID.LONG: lambda type_id, value: struct.pack("<q", value),
Review comment:
why the change here?
--
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.
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
[GitHub] [iceberg] rymurr commented on a change in pull request #2672: [Python] Update to_byte_buff_mapping and from_byte_buff_mapping
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #2672:
URL: https://github.com/apache/iceberg/pull/2672#discussion_r646357655
##########
File path: python/iceberg/api/types/conversions.py
##########
@@ -42,34 +42,34 @@ class Conversions(object):
to_byte_buff_mapping = {TypeID.BOOLEAN: lambda type_id, value: struct.pack("<h", 1 if value else 0),
TypeID.INTEGER: lambda type_id, value: struct.pack("<i", value),
TypeID.DATE: lambda type_id, value: struct.pack("<i", value),
- TypeID.LONG: lambda type_id, value: struct.pack("<l", value),
- TypeID.TIME: lambda type_id, value: struct.pack("<l", value),
- TypeID.TIMESTAMP: lambda type_id, value: struct.pack("<l", value),
+ TypeID.LONG: lambda type_id, value: struct.pack("<q", value),
Review comment:
ahhh, got it. I thought `long long` meant `double long` not `long` and `l` meant `long`. How bizarre of python...
--
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.
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
[GitHub] [iceberg] jun-he commented on a change in pull request #2672: [Python] Update to_byte_buff_mapping and from_byte_buff_mapping
Posted by GitBox <gi...@apache.org>.
jun-he commented on a change in pull request #2672:
URL: https://github.com/apache/iceberg/pull/2672#discussion_r645813781
##########
File path: python/iceberg/api/types/conversions.py
##########
@@ -42,34 +42,34 @@ class Conversions(object):
to_byte_buff_mapping = {TypeID.BOOLEAN: lambda type_id, value: struct.pack("<h", 1 if value else 0),
TypeID.INTEGER: lambda type_id, value: struct.pack("<i", value),
TypeID.DATE: lambda type_id, value: struct.pack("<i", value),
- TypeID.LONG: lambda type_id, value: struct.pack("<l", value),
- TypeID.TIME: lambda type_id, value: struct.pack("<l", value),
- TypeID.TIMESTAMP: lambda type_id, value: struct.pack("<l", value),
+ TypeID.LONG: lambda type_id, value: struct.pack("<q", value),
Review comment:
@rymurr Here, `<q` is for `long long` with 8 bytes and `<i` is for `integer` with 4 bytes.
I think here we need 8 bytes to match Java. Also `from_byte_buff_mapping` uses `<q`.
--
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.
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
[GitHub] [iceberg] rymurr merged pull request #2672: [Python] Update to_byte_buff_mapping and from_byte_buff_mapping
Posted by GitBox <gi...@apache.org>.
rymurr merged pull request #2672:
URL: https://github.com/apache/iceberg/pull/2672
--
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.
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
[GitHub] [iceberg] jun-he commented on pull request #2672: [Python] Update to_byte_buff_mapping and from_byte_buff_mapping
Posted by GitBox <gi...@apache.org>.
jun-he commented on pull request #2672:
URL: https://github.com/apache/iceberg/pull/2672#issuecomment-855000716
thanks @TGooch44 @rymurr for the review. I will check the decimal type and put it in a different PR.
--
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.
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
[GitHub] [iceberg] TGooch44 commented on pull request #2672: [Python] Update to_byte_buff_mapping and from_byte_buff_mapping
Posted by GitBox <gi...@apache.org>.
TGooch44 commented on pull request #2672:
URL: https://github.com/apache/iceberg/pull/2672#issuecomment-854981706
> @rymurr @TGooch44 Here is the PR to update conversions.
> BTW, looks like decimal type is not supported and is commented out in the code. I am wondering if there is any issue with it?
It was just never fully tested/implemented.
--
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.
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
[GitHub] [iceberg] jun-he commented on pull request #2672: [Python] Update to_byte_buff_mapping and from_byte_buff_mapping
Posted by GitBox <gi...@apache.org>.
jun-he commented on pull request #2672:
URL: https://github.com/apache/iceberg/pull/2672#issuecomment-853948176
@rymurr @TGooch44 Here is the PR to update conversions.
BTW, looks like decimal type is not supported and is commented out in the code. I am wondering if there is any issue with it?
--
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.
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
[GitHub] [iceberg] jun-he commented on pull request #2672: [Python] Update to_byte_buff_mapping and from_byte_buff_mapping
Posted by GitBox <gi...@apache.org>.
jun-he commented on pull request #2672:
URL: https://github.com/apache/iceberg/pull/2672#issuecomment-853948176
@rymurr @TGooch44 Here is the PR to update conversions.
BTW, looks like decimal type is not supported and is commented out in the code. I am wondering if there is any issue with it?
--
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.
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