Arrow: Don't copy the list/map when not needed

[Fokko]

Wrapping the list seems to introduce an odd behavior where null values are converted to an empty list [].

Because the arrays in Arrow have a counter for the null values. When we create a new list by copying, the counter is not taken into account, and just empty lists/maps are injected instead.

Resolves#251

[HonahX]

Overall LGTM! Thanks for the great work @Fokko.

I noticed one edge case that could still lead to the issue. I've found one workaround but not sure if it works for other edge cases

[HonahX]

Just to confirm my understanding, another reason that we have to copy the array: Arrow also does not allow field-mismatch, which happens when we have an optional schema field and no values for that field in the file.

[HonahX]

And this limitation will make the issue remaining in this edge case (col_list array<struct<test:int>>):

spark.sql(
f""" CREATE TABLE {catalog_name}.default.test_table_empty_list_and_map ( col_list array<struct<test:int>>, col_map map<int, int> ) USING iceberg TBLPROPERTIES ( 'format-version'='1' ); """
)
spark.sql(
f""" INSERT INTO {catalog_name}.default.test_table_empty_list_and_map VALUES (null, null) """
)
deftest_null_list_and_map(catalog: Catalog) ->None:
table_test_empty_list_and_map=catalog.load_table("default.test_table_empty_list_and_map")
arrow_table=table_test_empty_list_and_map.scan().to_arrow()
>assertarrow_table["col_list"].to_pylist() == [None]
Eassert [[]] == [None]

[HonahX]

It seems adding the following before line 1167

iflist_array.is_null():
returnNone

can let me pass the above test.

[Fokko]

Thanks for the test-case, let me add that one 👍 I don't think the last fix is going to work, since not all values might be null 🤔

[Fokko]

I've pushed a fix, but I don't think we can solve this yet without a fix in Arrow upstream 😢 apache/arrow#38809

[HonahX]

Thanks for fixing this issue! @Fokko. I think we can merge this first and leave that issue open until the upstream fix release.

[Fokko]

@HonahX I agree, thanks for merging. This PR is already an improvement over the previous situation, so I think it is good to have it in.

[Fokko]

Wrapping the list seems to introduce an odd behavior where null values are converted to an empty list [].

Because the arrays in Arrow have a counter for the null values. When we create a new list by copying, the counter is not taken into account, and just empty lists/maps are injected instead.

Resolves#251

[HonahX]

Overall LGTM! Thanks for the great work @Fokko.

I noticed one edge case that could still lead to the issue. I've found one workaround but not sure if it works for other edge cases

[HonahX]

Just to confirm my understanding, another reason that we have to copy the array: Arrow also does not allow field-mismatch, which happens when we have an optional schema field and no values for that field in the file.

[HonahX]

And this limitation will make the issue remaining in this edge case (col_list array<struct<test:int>>):

spark.sql(
f""" CREATE TABLE {catalog_name}.default.test_table_empty_list_and_map ( col_list array<struct<test:int>>, col_map map<int, int> ) USING iceberg TBLPROPERTIES ( 'format-version'='1' ); """
)
spark.sql(
f""" INSERT INTO {catalog_name}.default.test_table_empty_list_and_map VALUES (null, null) """
)
deftest_null_list_and_map(catalog: Catalog) ->None:
table_test_empty_list_and_map=catalog.load_table("default.test_table_empty_list_and_map")
arrow_table=table_test_empty_list_and_map.scan().to_arrow()
>assertarrow_table["col_list"].to_pylist() == [None]
Eassert [[]] == [None]

[HonahX]

It seems adding the following before line 1167

iflist_array.is_null():
returnNone

can let me pass the above test.

[Fokko]

Thanks for the test-case, let me add that one 👍 I don't think the last fix is going to work, since not all values might be null 🤔

[Fokko]

I've pushed a fix, but I don't think we can solve this yet without a fix in Arrow upstream 😢 apache/arrow#38809

[HonahX]

Thanks for fixing this issue! @Fokko. I think we can merge this first and leave that issue open until the upstream fix release.

[Fokko]

@HonahX I agree, thanks for merging. This PR is already an improvement over the previous situation, so I think it is good to have it in.

[Fokko]

Wrapping the list seems to introduce an odd behavior where null values are converted to an empty list [].

Because the arrays in Arrow have a counter for the null values. When we create a new list by copying, the counter is not taken into account, and just empty lists/maps are injected instead.

Resolves#251

[HonahX]

Overall LGTM! Thanks for the great work @Fokko.

I noticed one edge case that could still lead to the issue. I've found one workaround but not sure if it works for other edge cases

[HonahX]

Just to confirm my understanding, another reason that we have to copy the array: Arrow also does not allow field-mismatch, which happens when we have an optional schema field and no values for that field in the file.

[HonahX]

And this limitation will make the issue remaining in this edge case (col_list array<struct<test:int>>):

spark.sql(
f""" CREATE TABLE {catalog_name}.default.test_table_empty_list_and_map ( col_list array<struct<test:int>>, col_map map<int, int> ) USING iceberg TBLPROPERTIES ( 'format-version'='1' ); """
)
spark.sql(
f""" INSERT INTO {catalog_name}.default.test_table_empty_list_and_map VALUES (null, null) """
)
deftest_null_list_and_map(catalog: Catalog) ->None:
table_test_empty_list_and_map=catalog.load_table("default.test_table_empty_list_and_map")
arrow_table=table_test_empty_list_and_map.scan().to_arrow()
>assertarrow_table["col_list"].to_pylist() == [None]
Eassert [[]] == [None]

[HonahX]

It seems adding the following before line 1167

iflist_array.is_null():
returnNone

can let me pass the above test.

[Fokko]

Thanks for the test-case, let me add that one 👍 I don't think the last fix is going to work, since not all values might be null 🤔

[Fokko]

I've pushed a fix, but I don't think we can solve this yet without a fix in Arrow upstream 😢 apache/arrow#38809

[HonahX]

Thanks for fixing this issue! @Fokko. I think we can merge this first and leave that issue open until the upstream fix release.

[Fokko]

@HonahX I agree, thanks for merging. This PR is already an improvement over the previous situation, so I think it is good to have it in.

[Fokko]

Wrapping the list seems to introduce an odd behavior where null values are converted to an empty list [].

Because the arrays in Arrow have a counter for the null values. When we create a new list by copying, the counter is not taken into account, and just empty lists/maps are injected instead.

Resolves#251

[HonahX]

Overall LGTM! Thanks for the great work @Fokko.

I noticed one edge case that could still lead to the issue. I've found one workaround but not sure if it works for other edge cases

[HonahX]

Just to confirm my understanding, another reason that we have to copy the array: Arrow also does not allow field-mismatch, which happens when we have an optional schema field and no values for that field in the file.

[HonahX]

And this limitation will make the issue remaining in this edge case (col_list array<struct<test:int>>):

spark.sql(
f""" CREATE TABLE {catalog_name}.default.test_table_empty_list_and_map ( col_list array<struct<test:int>>, col_map map<int, int> ) USING iceberg TBLPROPERTIES ( 'format-version'='1' ); """
)
spark.sql(
f""" INSERT INTO {catalog_name}.default.test_table_empty_list_and_map VALUES (null, null) """
)
deftest_null_list_and_map(catalog: Catalog) ->None:
table_test_empty_list_and_map=catalog.load_table("default.test_table_empty_list_and_map")
arrow_table=table_test_empty_list_and_map.scan().to_arrow()
>assertarrow_table["col_list"].to_pylist() == [None]
Eassert [[]] == [None]

[HonahX]

It seems adding the following before line 1167

iflist_array.is_null():
returnNone

can let me pass the above test.

[Fokko]

Thanks for the test-case, let me add that one 👍 I don't think the last fix is going to work, since not all values might be null 🤔

[Fokko]

I've pushed a fix, but I don't think we can solve this yet without a fix in Arrow upstream 😢 apache/arrow#38809

[HonahX]

Thanks for fixing this issue! @Fokko. I think we can merge this first and leave that issue open until the upstream fix release.

[Fokko]

@HonahX I agree, thanks for merging. This PR is already an improvement over the previous situation, so I think it is good to have it in.

[Fokko]

Wrapping the list seems to introduce an odd behavior where null values are converted to an empty list [].

Because the arrays in Arrow have a counter for the null values. When we create a new list by copying, the counter is not taken into account, and just empty lists/maps are injected instead.

Resolves#251

[HonahX]

Overall LGTM! Thanks for the great work @Fokko.

I noticed one edge case that could still lead to the issue. I've found one workaround but not sure if it works for other edge cases

[HonahX]

Just to confirm my understanding, another reason that we have to copy the array: Arrow also does not allow field-mismatch, which happens when we have an optional schema field and no values for that field in the file.

[HonahX]

And this limitation will make the issue remaining in this edge case (col_list array<struct<test:int>>):

spark.sql(
f""" CREATE TABLE {catalog_name}.default.test_table_empty_list_and_map ( col_list array<struct<test:int>>, col_map map<int, int> ) USING iceberg TBLPROPERTIES ( 'format-version'='1' ); """
)
spark.sql(
f""" INSERT INTO {catalog_name}.default.test_table_empty_list_and_map VALUES (null, null) """
)
deftest_null_list_and_map(catalog: Catalog) ->None:
table_test_empty_list_and_map=catalog.load_table("default.test_table_empty_list_and_map")
arrow_table=table_test_empty_list_and_map.scan().to_arrow()
>assertarrow_table["col_list"].to_pylist() == [None]
Eassert [[]] == [None]

[HonahX]

It seems adding the following before line 1167

iflist_array.is_null():
returnNone

can let me pass the above test.

[Fokko]

Thanks for the test-case, let me add that one 👍 I don't think the last fix is going to work, since not all values might be null 🤔

[Fokko]

I've pushed a fix, but I don't think we can solve this yet without a fix in Arrow upstream 😢 apache/arrow#38809

[HonahX]

Thanks for fixing this issue! @Fokko. I think we can merge this first and leave that issue open until the upstream fix release.

[Fokko]

@HonahX I agree, thanks for merging. This PR is already an improvement over the previous situation, so I think it is good to have it in.

[Fokko]

Wrapping the list seems to introduce an odd behavior where null values are converted to an empty list [].

Because the arrays in Arrow have a counter for the null values. When we create a new list by copying, the counter is not taken into account, and just empty lists/maps are injected instead.

Resolves#251

[HonahX]

Overall LGTM! Thanks for the great work @Fokko.

I noticed one edge case that could still lead to the issue. I've found one workaround but not sure if it works for other edge cases

[HonahX]

Just to confirm my understanding, another reason that we have to copy the array: Arrow also does not allow field-mismatch, which happens when we have an optional schema field and no values for that field in the file.

[HonahX]

And this limitation will make the issue remaining in this edge case (col_list array<struct<test:int>>):

spark.sql(
f""" CREATE TABLE {catalog_name}.default.test_table_empty_list_and_map ( col_list array<struct<test:int>>, col_map map<int, int> ) USING iceberg TBLPROPERTIES ( 'format-version'='1' ); """
)
spark.sql(
f""" INSERT INTO {catalog_name}.default.test_table_empty_list_and_map VALUES (null, null) """
)
deftest_null_list_and_map(catalog: Catalog) ->None:
table_test_empty_list_and_map=catalog.load_table("default.test_table_empty_list_and_map")
arrow_table=table_test_empty_list_and_map.scan().to_arrow()
>assertarrow_table["col_list"].to_pylist() == [None]
Eassert [[]] == [None]

[HonahX]

It seems adding the following before line 1167

iflist_array.is_null():
returnNone

can let me pass the above test.

[Fokko]

Thanks for the test-case, let me add that one 👍 I don't think the last fix is going to work, since not all values might be null 🤔

[Fokko]

I've pushed a fix, but I don't think we can solve this yet without a fix in Arrow upstream 😢 apache/arrow#38809

[HonahX]

Thanks for fixing this issue! @Fokko. I think we can merge this first and leave that issue open until the upstream fix release.

[Fokko]

@HonahX I agree, thanks for merging. This PR is already an improvement over the previous situation, so I think it is good to have it in.

[Fokko]

Wrapping the list seems to introduce an odd behavior where null values are converted to an empty list [].

Because the arrays in Arrow have a counter for the null values. When we create a new list by copying, the counter is not taken into account, and just empty lists/maps are injected instead.

Resolves#251

[HonahX]

Overall LGTM! Thanks for the great work @Fokko.

I noticed one edge case that could still lead to the issue. I've found one workaround but not sure if it works for other edge cases

[HonahX]

Just to confirm my understanding, another reason that we have to copy the array: Arrow also does not allow field-mismatch, which happens when we have an optional schema field and no values for that field in the file.

[HonahX]

And this limitation will make the issue remaining in this edge case (col_list array<struct<test:int>>):

spark.sql(
f""" CREATE TABLE {catalog_name}.default.test_table_empty_list_and_map ( col_list array<struct<test:int>>, col_map map<int, int> ) USING iceberg TBLPROPERTIES ( 'format-version'='1' ); """
)
spark.sql(
f""" INSERT INTO {catalog_name}.default.test_table_empty_list_and_map VALUES (null, null) """
)
deftest_null_list_and_map(catalog: Catalog) ->None:
table_test_empty_list_and_map=catalog.load_table("default.test_table_empty_list_and_map")
arrow_table=table_test_empty_list_and_map.scan().to_arrow()
>assertarrow_table["col_list"].to_pylist() == [None]
Eassert [[]] == [None]

[HonahX]

It seems adding the following before line 1167

iflist_array.is_null():
returnNone

can let me pass the above test.

[Fokko]

Thanks for the test-case, let me add that one 👍 I don't think the last fix is going to work, since not all values might be null 🤔

[Fokko]

I've pushed a fix, but I don't think we can solve this yet without a fix in Arrow upstream 😢 apache/arrow#38809

[HonahX]

Thanks for fixing this issue! @Fokko. I think we can merge this first and leave that issue open until the upstream fix release.

[Fokko]

@HonahX I agree, thanks for merging. This PR is already an improvement over the previous situation, so I think it is good to have it in.

[Fokko]

Wrapping the list seems to introduce an odd behavior where null values are converted to an empty list [].

Because the arrays in Arrow have a counter for the null values. When we create a new list by copying, the counter is not taken into account, and just empty lists/maps are injected instead.

Resolves#251

[HonahX]

Overall LGTM! Thanks for the great work @Fokko.

I noticed one edge case that could still lead to the issue. I've found one workaround but not sure if it works for other edge cases

[HonahX]

Just to confirm my understanding, another reason that we have to copy the array: Arrow also does not allow field-mismatch, which happens when we have an optional schema field and no values for that field in the file.

[HonahX]

And this limitation will make the issue remaining in this edge case (col_list array<struct<test:int>>):

spark.sql(
f""" CREATE TABLE {catalog_name}.default.test_table_empty_list_and_map ( col_list array<struct<test:int>>, col_map map<int, int> ) USING iceberg TBLPROPERTIES ( 'format-version'='1' ); """
)
spark.sql(
f""" INSERT INTO {catalog_name}.default.test_table_empty_list_and_map VALUES (null, null) """
)
deftest_null_list_and_map(catalog: Catalog) ->None:
table_test_empty_list_and_map=catalog.load_table("default.test_table_empty_list_and_map")
arrow_table=table_test_empty_list_and_map.scan().to_arrow()
>assertarrow_table["col_list"].to_pylist() == [None]
Eassert [[]] == [None]

[HonahX]

It seems adding the following before line 1167

iflist_array.is_null():
returnNone

can let me pass the above test.

[Fokko]

Thanks for the test-case, let me add that one 👍 I don't think the last fix is going to work, since not all values might be null 🤔

[Fokko]

I've pushed a fix, but I don't think we can solve this yet without a fix in Arrow upstream 😢 apache/arrow#38809

[HonahX]

Thanks for fixing this issue! @Fokko. I think we can merge this first and leave that issue open until the upstream fix release.

[Fokko]

@HonahX I agree, thanks for merging. This PR is already an improvement over the previous situation, so I think it is good to have it in.