Arrow: Infer the types when reading #1669
Conversation
When reading a Parquet file using PyArrow, there is some metadata stored in the Parquet file to either make it a large type (eg `large_string`, or a normal type (`string`). The difference is that the large types use a 64 bit offset to encode their arrays. This is not always needed, and we can could first check all the in the types of which it is stored, and let PyArrow decide here: https://github.com/apache/iceberg-python/blob/300b8405a0fe7d0111321e5644d704026af9266b/pyiceberg/io/pyarrow.py#L1579 In PyArrow today we just bump everything to a large type, which might lead to additional memory consumption because it allocates a int64 array to allocate the offsets, instead of an int32. I thought we would be good to go for this now with the new lower bound of PyArrow to 17. But, it looks like we still have to wait for Arrow 18 to fix the issue with the `date` types: apache/arrow#43183 Fixes: apache#1049
| @@ -1750,7 +1750,7 @@ def to_arrow_batch_reader(self) -> pa.RecordBatchReader: | |||
| return pa.RecordBatchReader.from_batches( | |||
| target_schema, | |||
| batches, | |||
| ) | |||
| ).cast(target_schema) | |||
There was a problem hiding this comment.
This will still return large types if you stream the batches because we don't want to fetch all the schemas upfront.
| if property_as_bool(self._io.properties, PYARROW_USE_LARGE_TYPES_ON_READ, False): | ||
| result = result.cast(arrow_schema) |
There was a problem hiding this comment.
I left this in, but I would be leaning toward deprecating this, since I don't think we want to trouble the user. I think it should be an implementation detail based on how large the buffers are.
|
@sungwy Thoughts? :D |
There was a problem hiding this comment.
Hi @Fokko - thank you for pinging me for review! The change looks good to me, but I have a reservation about introducing this change without a deprecation warning.
Firstly, without the PyIceberg code base having a properly defined list of public classes, we assume all our classes to be public facing unless they start with an underscore. I'd argue that removing an input parameter to the ArrowProjectionVisitor __init__ method is an API change.
Secondly, changing the default value of PYARROW_USE_LARGE_TYPES_ON_READ to True for to_table method also seems like a breaking change for users reading Iceberg tables through PyIceberg. their large_string columns will change to a string column on upgrade without a warning.
Would it make sense to introduce this change in two stages:
- First by introducing a new config variable like:
PYICEBERG_INFER_LARGE_TYPES_ON_READand set it toFalseon default, and raise a deprecation warning when the flag is set toFalse? - Then remove PYICEBERG_INFER_LARGE_TYPES_ON_READ and PYARROW_USE_LARGE_TYPES_ON_READ in the next major version?
|
|
||
| def __init__( | ||
| self, | ||
| file_schema: Schema, | ||
| downcast_ns_timestamp_to_us: bool = False, | ||
| include_field_ids: bool = False, | ||
| use_large_types: bool = True, |
There was a problem hiding this comment.
I've always dreaded the process of updating our code base in these internal classes and functions because we do not yet have a properly defined list of public classes 😞
Would this change require a deprecation notice first?
|
|
||
| result = pa.concat_tables(tables, promote_options="permissive") | ||
|
|
||
| if property_as_bool(self._io.properties, PYARROW_USE_LARGE_TYPES_ON_READ, False): |
There was a problem hiding this comment.
Should we update this to align with the current default value?
| if property_as_bool(self._io.properties, PYARROW_USE_LARGE_TYPES_ON_READ, False): | |
| if property_as_bool(self._io.properties, PYARROW_USE_LARGE_TYPES_ON_READ, True): |
Time to give this another go 😆
When reading a Parquet file using PyArrow, there is some metadata stored in the Parquet file to either make it a large type (eg
large_string, or a normal type (string). The difference is that the large types use a 64 bit offset to encode their arrays. This is not always needed, and we can could first check all the in the types of which it is stored, and let PyArrow decide here:iceberg-python/pyiceberg/io/pyarrow.py
Line 1579 in 300b840
In PyArrow today we just bump everything to a large type, which might lead to additional memory consumption because it allocates an int64 array to allocate the offsets, instead of an int32.
I thought we would be good to go for this now with the new lower bound of PyArrow to 17. But, it looks like we still have to wait for Arrow 18 to fix the issue with the
datetypes:apache/arrow#43183
Fixes: #1049