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/07/03 02:42:36 UTC
[PR] Forward Compatible large_* type support: read as large, write as small [iceberg-python]
syun64 opened a new pull request, #890:
URL: https://github.com/apache/iceberg-python/pull/890
Solves: https://github.com/apache/iceberg-python/issues/887
--
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] Forward Compatible large_* type support: read as large, write as small [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on PR #890:
URL: https://github.com/apache/iceberg-python/pull/890#issuecomment-2207040653
This is almost ready - I'm going to introduce the flag to all read API as well, so that users can control which schema they are using to read their pyarrow tables, as they may be using the pyarrow schema for consistency checks, etc
--
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] Forward Compatible large_* type support: read as large, write as small [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #890:
URL: https://github.com/apache/iceberg-python/pull/890#discussion_r1666835301
##########
pyiceberg/table/__init__.py:
##########
@@ -1866,7 +1866,7 @@ def plan_files(self) -> Iterable[FileScanTask]:
for data_entry in data_entries
]
- def to_arrow(self) -> pa.Table:
+ def to_arrow(self, with_large_types: bool = True) -> pa.Table:
Review Comment:
> I felt like we needed a way for the user to control which type they would be using for their arrow table or RecordBatchReader
I don't think we should expose this in the public API. Do people want to control this? In an ideal world:
- When writing you want to take the type that's being handed to PyIceberg from the user
- When reading you want to take this information from what comes out of the Parquet files
My first assumption was to go with the large one since that seems what most libraries seem to be using. But unfortunately, that doesn't seem to be the case.
--
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] Forward Compatible large_* type support: read as large, write as small [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 closed pull request #890: Forward Compatible large_* type support: read as large, write as small
URL: https://github.com/apache/iceberg-python/pull/890
--
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] Forward Compatible large_* type support: read as large, write as small [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #890:
URL: https://github.com/apache/iceberg-python/pull/890#discussion_r1666094679
##########
pyiceberg/table/__init__.py:
##########
@@ -1866,7 +1866,7 @@ def plan_files(self) -> Iterable[FileScanTask]:
for data_entry in data_entries
]
- def to_arrow(self) -> pa.Table:
+ def to_arrow(self, with_large_types: bool = True) -> pa.Table:
Review Comment:
Thank you very much for taking the time to review @Fokko .
It’s great you brought this up because I didn’t feel great about introducing a flag either… but I felt like we needed a way for the user to control which type they would be using for their arrow table or RecordBatchReader.
Do you have a preference for which type (large or small) should be the common type for the schema? The reason I’ve introduced a flag here is because we would still need to choose to which type to use in the pyarrow schema we infer based on the Iceberg table schema. As we’ve discussed in this [issue](https://github.com/apache/iceberg-python/issues/791), I thought being intentional about which type we are choosing to represent our table or RecordBatchReader would make the behavior feel more consistent and error prone for the end user, than the alternative of rendering the type that PyArrow infers based on the parquet file.
If this does not sound like a great candidate for an API argument, would having a configuration to control this behavior be a better option? I think that was an idea that was discussed in a [previous discussion here](https://github.com/apache/iceberg-python/pull/807#pullrequestreview-2119199017). Please let me know!
--
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] Forward Compatible large_* type support: read as large, write as small [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #890:
URL: https://github.com/apache/iceberg-python/pull/890#discussion_r1666027762
##########
pyiceberg/table/__init__.py:
##########
@@ -1866,7 +1866,7 @@ def plan_files(self) -> Iterable[FileScanTask]:
for data_entry in data_entries
]
- def to_arrow(self) -> pa.Table:
+ def to_arrow(self, with_large_types: bool = True) -> pa.Table:
Review Comment:
Hey @syun64 Thanks again for jumping on this issue. It is a very nasty one, so thanks for doing the hard work here.
Can I suggest one more direction? My first thoughts are that we should not bother the user with having to set this kind of flags. Instead, I think we can solve it when we concatenate the table:
When we do `to_requested_schema`, we can allow both a normal and a large string when we request a string type. When doing the concatenation of the batches into a table, we let Arrow coerce to a common type. WDYT?
--
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] Forward Compatible large_* type support: read as large, write as small [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on PR #890:
URL: https://github.com/apache/iceberg-python/pull/890#issuecomment-2221501265
close in favor of https://github.com/apache/iceberg-python/pull/902
--
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] Forward Compatible large_* type support: read as large, write as small [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #890:
URL: https://github.com/apache/iceberg-python/pull/890#discussion_r1670709039
##########
pyiceberg/table/__init__.py:
##########
@@ -1866,7 +1866,7 @@ def plan_files(self) -> Iterable[FileScanTask]:
for data_entry in data_entries
]
- def to_arrow(self) -> pa.Table:
+ def to_arrow(self, with_large_types: bool = True) -> pa.Table:
Review Comment:
> I don't think we should expose this in the public API. Do people want to control this? In an ideal world:
>
> * When writing you want to take the type that's being handed to PyIceberg from the user
> * When reading you want to take this information from what comes out of the Parquet files
I agree with this in the ideal world. However, PyArrow API cannot handle both large_* and normal types in its APIs without the us manually casting the type to one or the other. For example, the RecordBatchReader will fail to produce the next RecordBatch if the schema doesn't align completely, and requires us to choose one and always cast the types.
If the concern is in exposing this option in the public API, I think we can walk back on this change and remove it from:
- `to_arrow_batch_reader()`
- `to_arrow_table()`
- `to_requested_schema()`
But we may still need it in `schema_to_pyarrow` because here, we are making an opinionated decision about the type we are choosing to represent the data as, for when we write and for when we read.
> My first assumption was to go with the large one since that seems what most libraries seem to be using. But unfortunately, that doesn't seem to be the case.
- I think this is the case for daft and polars:
Daft:
```
>>> import pyarrow as pa
>>> import pyarrow.parquet as pq
>>> import daft
>>> daft.read_parquet("strings.parquet").to_arrow()
pyarrow.Table
strings: large_string
----
strings: [["a","b"]]
>>> daft.read_parquet("strings.parquet").to_arrow()
pyarrow.Table
strings: large_string
----
strings: [["a","b"]]
>>> daft.read_parquet("strings.parquet").to_arrow().cast(pa.schema([("strings", pa.string())])).write_parquet("small-strings.parquet")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: 'pyarrow.lib.Table' object has no attribute 'write_parquet'
>>> daft.from_arrow(daft.read_parquet("strings.parquet").to_arrow().cast(pa.schema([("strings", pa.string())]))).write_parquet("small-strings.parquet")
╭────────────────────────────────╮
│ path │
│ --- │
│ Utf8 │
╞════════════════════════════════╡
│ small-strings.parquet/74515f6… │
╰────────────────────────────────╯
(Showing first 1 of 1 rows)
>>> daft.read_parquet("small-strings.parquet").to_arrow()
pyarrow.Table
strings: large_string
----
strings: [["a","b"]]
>>> pq.read_table("small-strings.parquet")
pyarrow.Table
strings: large_string
----
strings: [["a","b"]]
```
--
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] Forward Compatible large_* type support: read as large, write as small [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #890:
URL: https://github.com/apache/iceberg-python/pull/890#discussion_r1668666127
##########
pyiceberg/table/__init__.py:
##########
@@ -1866,7 +1866,7 @@ def plan_files(self) -> Iterable[FileScanTask]:
for data_entry in data_entries
]
- def to_arrow(self) -> pa.Table:
+ def to_arrow(self, with_large_types: bool = True) -> pa.Table:
Review Comment:
@syun64 @HonahX I've played around with this, and I think we can Arrow decide on the types: https://github.com/apache/iceberg-python/pull/902
##########
pyiceberg/table/__init__.py:
##########
@@ -1866,7 +1866,7 @@ def plan_files(self) -> Iterable[FileScanTask]:
for data_entry in data_entries
]
- def to_arrow(self) -> pa.Table:
+ def to_arrow(self, with_large_types: bool = True) -> pa.Table:
Review Comment:
@syun64 @HonahX I've played around with this, and I think we can let Arrow decide on the types: https://github.com/apache/iceberg-python/pull/902
--
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