From 69af7ae7bbe9220145f1c0b2db1fd23402d14659 Mon Sep 17 00:00:00 2001 From: Sylwester Lachiewicz Date: Sun, 23 Aug 2026 11:35:51 +0200 Subject: [PATCH 1/2] Fix null list of structs being read and written as an empty list from_arrays() takes the offsets buffer alone, which cannot express a null list, so rebuilding the array without a mask collapsed every null list> into an empty one. On write that is unrecoverable: the Parquet file itself then holds the empty list. The rebuild still works around apache/arrow#38809 and stays; only the mask is added. That restores the assertion test_null_list_and_map has carried commented out since #252. Closes #3833 --- pyiceberg/io/pyarrow.py | 5 ++++- tests/integration/test_reads.py | 5 +---- tests/io/test_pyarrow.py | 37 +++++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 5 deletions(-) diff --git a/pyiceberg/io/pyarrow.py b/pyiceberg/io/pyarrow.py index c36f1639d9..cb4caa247f 100644 --- a/pyiceberg/io/pyarrow.py +++ b/pyiceberg/io/pyarrow.py @@ -2075,7 +2075,10 @@ def list(self, list_type: ListType, list_array: pa.Array | None, value_array: pa if isinstance(value_array, pa.StructArray): # This can be removed once this has been fixed: # https://github.com/apache/arrow/issues/38809 - list_array = pa.LargeListArray.from_arrays(list_array.offsets, value_array) + # The mask must be carried over explicitly: from_arrays() takes the offsets + # buffer alone, which cannot express a null list, so without it every null + # list is rebuilt as an empty one. + list_array = pa.LargeListArray.from_arrays(list_array.offsets, value_array, mask=list_array.is_null()) value_array = self._cast_if_needed(list_type.element_field, value_array) arrow_field = list_initializer(self._construct_field(list_type.element_field, value_array.type)) return list_array.cast(arrow_field) diff --git a/tests/integration/test_reads.py b/tests/integration/test_reads.py index 3ee94b751f..ef81add078 100644 --- a/tests/integration/test_reads.py +++ b/tests/integration/test_reads.py @@ -994,10 +994,7 @@ def test_null_list_and_map(catalog: Catalog) -> None: arrow_table = table_test_empty_list_and_map.scan().to_arrow() assert arrow_table["col_list"].to_pylist() == [None, []] assert arrow_table["col_map"].to_pylist() == [None, []] - # This should be: - # assert arrow_table["col_list_with_struct"].to_pylist() == [None, [{'test': 1}]] - # Once https://github.com/apache/arrow/issues/38809 has been fixed - assert arrow_table["col_list_with_struct"].to_pylist() == [[], [{"test": 1}]] + assert arrow_table["col_list_with_struct"].to_pylist() == [None, [{"test": 1}]] @pytest.mark.integration diff --git a/tests/io/test_pyarrow.py b/tests/io/test_pyarrow.py index b31c18949b..b6f13fadde 100644 --- a/tests/io/test_pyarrow.py +++ b/tests/io/test_pyarrow.py @@ -3210,6 +3210,43 @@ def test__to_requested_schema_float_promotion( assert result.column(0).to_pylist() == [1.5, 2.25, 3.0, None] +def test__to_requested_schema_null_list_of_structs() -> None: + """Test that a null list survives the write path when its element is a struct.""" + requested_schema = Schema( + NestedField( + 1, + "col_list_with_struct", + ListType(11, StructType(NestedField(111, "test", IntegerType(), required=False)), element_required=False), + required=False, + ), + NestedField(2, "col_list", ListType(21, IntegerType(), element_required=False), required=False), + ) + file_schema = requested_schema + + arrow_schema = pa.schema( + [ + pa.field("col_list_with_struct", pa.list_(pa.struct([pa.field("test", pa.int32())]))), + pa.field("col_list", pa.list_(pa.int32())), + ] + ) + batch = pa.RecordBatch.from_arrays( + [ + pa.array([[{"test": 1}], [], None], type=arrow_schema.field(0).type), + pa.array([[1], [], None], type=arrow_schema.field(1).type), + ], + schema=arrow_schema, + ) + + result = _to_requested_schema( + requested_schema, file_schema, batch, downcast_ns_timestamp_to_us=False, include_field_ids=False + ) + + # A null list and an empty list are different values, and only the struct-element + # case ever collapsed the former into the latter. + assert result.column(0).to_pylist() == [[{"test": 1}], [], None] + assert result.column(1).to_pylist() == [[1], [], None] + + def test_pyarrow_file_io_fs_by_scheme_cache() -> None: # It's better to set up multi-region minio servers for an integration test once `endpoint_url` argument # becomes available for `resolve_s3_region` From 1ee2f40f087541c6a811a9c722c98136db53d5f0 Mon Sep 17 00:00:00 2001 From: Sylwester Lachiewicz Date: Sun, 23 Aug 2026 12:37:47 +0200 Subject: [PATCH 2/2] Keep the list rebuild for renamed fields, not for arrow#38809 The comment claimed the branch could go once apache/arrow#38809 was fixed. That is fixed in pyarrow 19, but the branch must stay: Iceberg renames by field-id while cast matches element fields by name, so dropping it nulls out any field renamed inside a list element. Verified: the new test passes, and fails with the rebuild removed. --- pyiceberg/io/pyarrow.py | 10 +++++----- tests/io/test_pyarrow.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/pyiceberg/io/pyarrow.py b/pyiceberg/io/pyarrow.py index cb4caa247f..974dcf1023 100644 --- a/pyiceberg/io/pyarrow.py +++ b/pyiceberg/io/pyarrow.py @@ -2073,11 +2073,11 @@ def list(self, list_type: ListType, list_array: pa.Array | None, value_array: pa if isinstance(list_array, (pa.ListArray, pa.LargeListArray, pa.FixedSizeListArray)) and value_array is not None: list_initializer = pa.large_list if isinstance(list_array, pa.LargeListArray) else pa.list_ if isinstance(value_array, pa.StructArray): - # This can be removed once this has been fixed: - # https://github.com/apache/arrow/issues/38809 - # The mask must be carried over explicitly: from_arrays() takes the offsets - # buffer alone, which cannot express a null list, so without it every null - # list is rebuilt as an empty one. + # Rebuilding is what applies the projection to the element struct. A plain + # cast would not do: Iceberg renames by field-id, while cast matches the + # element fields by name and would silently null out any renamed one. The + # mask has to be carried over because the offsets buffer alone cannot + # express a null list, only an empty one. list_array = pa.LargeListArray.from_arrays(list_array.offsets, value_array, mask=list_array.is_null()) value_array = self._cast_if_needed(list_type.element_field, value_array) arrow_field = list_initializer(self._construct_field(list_type.element_field, value_array.type)) diff --git a/tests/io/test_pyarrow.py b/tests/io/test_pyarrow.py index b6f13fadde..98ccb02ebd 100644 --- a/tests/io/test_pyarrow.py +++ b/tests/io/test_pyarrow.py @@ -3247,6 +3247,40 @@ def test__to_requested_schema_null_list_of_structs() -> None: assert result.column(1).to_pylist() == [[1], [], None] +def test__to_requested_schema_renamed_field_in_list_of_structs() -> None: + """Test that a field renamed inside a list element keeps its values.""" + file_schema = Schema( + NestedField( + 1, + "col", + ListType(11, StructType(NestedField(111, "before", IntegerType(), required=False)), element_required=False), + required=False, + ), + ) + requested_schema = Schema( + NestedField( + 1, + "col", + ListType(11, StructType(NestedField(111, "after", IntegerType(), required=False)), element_required=False), + required=False, + ), + ) + + arrow_schema = pa.schema([pa.field("col", pa.list_(pa.struct([pa.field("before", pa.int32())])))]) + batch = pa.RecordBatch.from_arrays( + [pa.array([[{"before": 1}], [], None, [{"before": 3}]], type=arrow_schema.field(0).type)], + schema=arrow_schema, + ) + + result = _to_requested_schema( + requested_schema, file_schema, batch, downcast_ns_timestamp_to_us=False, include_field_ids=False + ) + + # Rebuilding the list from the projected element array is what carries the rename. + # Casting instead would match the element fields by name and yield nulls throughout. + assert result.column(0).to_pylist() == [[{"after": 1}], [], None, [{"after": 3}]] + + def test_pyarrow_file_io_fs_by_scheme_cache() -> None: # It's better to set up multi-region minio servers for an integration test once `endpoint_url` argument # becomes available for `resolve_s3_region`