You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "syun64 (via GitHub)" <gi...@apache.org> on 2024/06/11 01:56:35 UTC
[PR] Cast PyArrow schema to `large_*` types [iceberg-python]
syun64 opened a new pull request, #807:
URL: https://github.com/apache/iceberg-python/pull/807
Fixes #791
For consistency, we should always cast to large types when inferring pyarrow schema from Iceberg Schema, and when scanning using the physical_schema of the fragment
--
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
Re: [PR] Cast PyArrow schema to `large_*` types [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on PR #807:
URL: https://github.com/apache/iceberg-python/pull/807#issuecomment-2168510997
@HonahX could I ask for you to merge this in? It'll help unblock me in https://github.com/apache/iceberg-python/pull/786/files
--
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
Re: [PR] Cast PyArrow schema to `large_*` types [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on PR #807:
URL: https://github.com/apache/iceberg-python/pull/807#issuecomment-2163182848
> To summarize the discussion in https://github.com/apache/iceberg-python/issues/791, we could always benefit from reading data as `large_*` type since offset is 64-bit.
Yes, that's how I understand it too. There are benefits to using `large_*` types in memory, so we can decouple the motivation for storing data in memory as large types from that of writing large types, even if our only supported (in PyIceberg) file type doesn't support writing large data yet.
> For parquet, we will still write data in non large type due to parquet's 2GB data size limitation.
I think it actually won't matter either way because we will get an error when we either try to down cast the type to the smaller type, or try to write the parquet file when we have actually large data in the table. I've updated to_requested_schema function to always cast to large types even on write for simplicity.
> Just to confirm my understanding, since the major difference between `large_binary` and `binary` is the offset type (64-bit versus 32-bit), there will be no significant increase in memory usage when reading data as `large_binary`.
Yes that's right. I've added a comment as you've suggested 🙂
--
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
Re: [PR] Cast PyArrow schema to `large_*` types [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #807:
URL: https://github.com/apache/iceberg-python/pull/807
--
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
Re: [PR] Cast PyArrow schema to `large_*` types [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #807:
URL: https://github.com/apache/iceberg-python/pull/807#discussion_r1637685511
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1167,8 +1197,14 @@ def __init__(self, file_schema: Schema):
def _cast_if_needed(self, field: NestedField, values: pa.Array) -> pa.Array:
file_field = self.file_schema.find_field(field.field_id)
- if field.field_type.is_primitive and field.field_type != file_field.field_type:
- return values.cast(schema_to_pyarrow(promote(file_field.field_type, field.field_type), include_field_ids=False))
+ if field.field_type.is_primitive:
+ if field.field_type != file_field.field_type:
+ return values.cast(schema_to_pyarrow(promote(file_field.field_type, field.field_type), include_field_ids=False))
+ elif (target_type := schema_to_pyarrow(field.field_type, include_field_ids=False)) != values.type:
+ # if file_field and field type (e.g. String) are the same
Review Comment:
[super small nit] I think it is good to include underscore in both `file_field` and `field_type`, or remove it from both of them to ensure consistency.
--
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
Re: [PR] Cast PyArrow schema to `large_*` types [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #807:
URL: https://github.com/apache/iceberg-python/pull/807#discussion_r1635920344
##########
pyiceberg/io/pyarrow.py:
##########
@@ -680,6 +680,10 @@ def _pyarrow_to_schema_without_ids(schema: pa.Schema) -> Schema:
return visit_pyarrow(schema, _ConvertToIcebergWithoutIDs())
+def _pyarrow_with_large_types(schema: pa.Schema) -> pa.Schema:
Review Comment:
How about naming it `_pyarrow_ensure_large_types` to reflect that we directly convert types to `large_*` if applicable.
##########
pyiceberg/io/pyarrow.py:
##########
@@ -998,7 +1026,7 @@ def _task_to_table(
fragment_scanner = ds.Scanner.from_fragment(
fragment=fragment,
- schema=physical_schema,
+ schema=_pyarrow_with_large_types(physical_schema),
Review Comment:
It may be good to add a comment (either here or in the method body) to explain that we read data as `large_*` types to improve the performance
--
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
Re: [PR] Cast PyArrow schema to `large_*` types [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #807:
URL: https://github.com/apache/iceberg-python/pull/807#discussion_r1636324658
##########
pyiceberg/io/pyarrow.py:
##########
@@ -680,6 +680,10 @@ def _pyarrow_to_schema_without_ids(schema: pa.Schema) -> Schema:
return visit_pyarrow(schema, _ConvertToIcebergWithoutIDs())
+def _pyarrow_with_large_types(schema: pa.Schema) -> pa.Schema:
Review Comment:
I really like this name suggestion - thank you!
--
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