Forward Compatible large_* type support: read as large, write as small #890
Conversation
|
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 |
| @@ -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: | |||
There was a problem hiding this 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?
There was a problem hiding this 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, 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. Please let me know!
There was a problem hiding this 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.
There was a problem hiding this 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"]]
|
close in favor of #902 |
Solves: #887