You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Fokko (via GitHub)" <gi...@apache.org> on 2025/01/21 14:30:44 UTC
[PR] PyArrow: Avoid buffer-overflow by avoid doing a sort [iceberg-python]
Fokko opened a new pull request, #1555:
URL: https://github.com/apache/iceberg-python/pull/1555
Second attempt of https://github.com/apache/iceberg-python/pull/1539
--
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] PyArrow: Avoid buffer-overflow by avoid doing a sort [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #1555:
URL: https://github.com/apache/iceberg-python/pull/1555#discussion_r1925454227
##########
pyiceberg/partitioning.py:
##########
@@ -413,8 +413,10 @@ def partition_record_value(partition_field: PartitionField, value: Any, schema:
the final partition record value.
"""
iceberg_type = schema.find_field(name_or_id=partition_field.source_id).field_type
- iceberg_typed_value = _to_partition_representation(iceberg_type, value)
- transformed_value = partition_field.transform.transform(iceberg_type)(iceberg_typed_value)
+ if not isinstance(value, int):
+ # When adding files, it can be that we still need to convert from logical types to physical types
+ value = _to_partition_representation(iceberg_type, value)
+ transformed_value = partition_field.transform.transform(iceberg_type)(value)
Review Comment:
This is causing bugs, I'm going to revisit this to fix it properly
--
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] PyArrow: Avoid buffer-overflow by avoid doing a sort [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #1555:
URL: https://github.com/apache/iceberg-python/pull/1555#discussion_r1924080871
##########
tests/integration/test_writes/test_partitioned_writes.py:
##########
@@ -1126,3 +1127,25 @@ def test_append_multiple_partitions(
"""
)
assert files_df.count() == 6
+
+
+@pytest.mark.integration
+def test_pyarrow_overflow(session_catalog: Catalog) -> None:
+ """Test what happens when the offset is beyond 32 bits"""
+ identifier = "default.arrow_table_overflow"
+ try:
+ session_catalog.drop_table(identifier=identifier)
+ except NoSuchTableError:
+ pass
+
+ x = pa.array([random.randint(0, 999) for _ in range(30_000)])
Review Comment:
i wonder if random here is making the test non-deterministic. since we're testing the overflow, what if we just use `999` or do `random.randint(900, 999)`
--
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] PyArrow: Avoid buffer-overflow by avoid doing a sort [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #1555:
URL: https://github.com/apache/iceberg-python/pull/1555#discussion_r1924222167
##########
tests/integration/test_writes/test_partitioned_writes.py:
##########
@@ -1126,3 +1127,25 @@ def test_append_multiple_partitions(
"""
)
assert files_df.count() == 6
+
+
+@pytest.mark.integration
+def test_pyarrow_overflow(session_catalog: Catalog) -> None:
+ """Test what happens when the offset is beyond 32 bits"""
+ identifier = "default.arrow_table_overflow"
+ try:
+ session_catalog.drop_table(identifier=identifier)
+ except NoSuchTableError:
+ pass
+
+ x = pa.array([random.randint(0, 999) for _ in range(30_000)])
Review Comment:
I would expect the overflow to come from the `StringType` column, but I agree to make this test deterministic 👍
--
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] PyArrow: Avoid buffer-overflow by avoid doing a sort [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #1555:
URL: https://github.com/apache/iceberg-python/pull/1555#discussion_r1925983653
##########
pyiceberg/partitioning.py:
##########
@@ -413,8 +413,10 @@ def partition_record_value(partition_field: PartitionField, value: Any, schema:
the final partition record value.
"""
iceberg_type = schema.find_field(name_or_id=partition_field.source_id).field_type
- iceberg_typed_value = _to_partition_representation(iceberg_type, value)
- transformed_value = partition_field.transform.transform(iceberg_type)(iceberg_typed_value)
+ if not isinstance(value, int):
+ # When adding files, it can be that we still need to convert from logical types to physical types
+ value = _to_partition_representation(iceberg_type, value)
+ transformed_value = partition_field.transform.transform(iceberg_type)(value)
Review Comment:
also if its just for adding files, perhaps we can do something special just for that path
--
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] PyArrow: Avoid buffer-overflow by avoid doing a sort [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #1555:
URL: https://github.com/apache/iceberg-python/pull/1555#issuecomment-2605603737
@kevinjqliu I think the test is a bit too much, according to your comment here https://github.com/apache/iceberg-python/pull/1539#discussion_r1922705843 the test allocates almost 5gb 😀
--
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] PyArrow: Avoid buffer-overflow by avoid doing a sort [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #1555:
URL: https://github.com/apache/iceberg-python/pull/1555#issuecomment-2605648387
2^32 (4_294_967_296) is around 4GB, we just need to test a scenario greater than that
--
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] PyArrow: Avoid buffer-overflow by avoid doing a sort [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #1555:
URL: https://github.com/apache/iceberg-python/pull/1555#discussion_r1925902812
##########
pyiceberg/partitioning.py:
##########
@@ -413,8 +413,10 @@ def partition_record_value(partition_field: PartitionField, value: Any, schema:
the final partition record value.
"""
iceberg_type = schema.find_field(name_or_id=partition_field.source_id).field_type
- iceberg_typed_value = _to_partition_representation(iceberg_type, value)
- transformed_value = partition_field.transform.transform(iceberg_type)(iceberg_typed_value)
+ if not isinstance(value, int):
+ # When adding files, it can be that we still need to convert from logical types to physical types
+ value = _to_partition_representation(iceberg_type, value)
+ transformed_value = partition_field.transform.transform(iceberg_type)(value)
Review Comment:
Yes, so I got to the bottom of it. It has to do with the return types of the transforms. eg. When we apply the `bucket` transform, the result is always an int, which is great. The problem is with the identity transform where the destination type is equal to the source type. So when a date comes in, it also comes out.
I think in the end it is better to remove the `_to_partition_representation` and see if we can consolidate this somewhere, but that's a different PR.
--
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] PyArrow: Avoid buffer-overflow by avoid doing a sort [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #1555:
URL: https://github.com/apache/iceberg-python/pull/1555#discussion_r1924223584
##########
tests/integration/test_writes/test_partitioned_writes.py:
##########
@@ -1126,3 +1127,25 @@ def test_append_multiple_partitions(
"""
)
assert files_df.count() == 6
+
+
+@pytest.mark.integration
+def test_pyarrow_overflow(session_catalog: Catalog) -> None:
+ """Test what happens when the offset is beyond 32 bits"""
+ identifier = "default.arrow_table_overflow"
+ try:
+ session_catalog.drop_table(identifier=identifier)
+ except NoSuchTableError:
+ pass
+
+ x = pa.array([random.randint(0, 999) for _ in range(30_000)])
Review Comment:
```suggestion
x = pa.array([1925 for _ in range(30_000)])
```
##########
tests/integration/test_writes/test_partitioned_writes.py:
##########
@@ -17,6 +17,7 @@
# pylint:disable=redefined-outer-name
+import random
Review Comment:
```suggestion
```
--
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] PyArrow: Avoid buffer-overflow by avoid doing a sort [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #1555:
URL: https://github.com/apache/iceberg-python/pull/1555#issuecomment-2605266133
> make: *** [Makefile:55: test-integration] Aborted (core dumped)
uh oh
--
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] PyArrow: Avoid buffer-overflow by avoid doing a sort [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #1555:
URL: https://github.com/apache/iceberg-python/pull/1555#discussion_r1925983134
##########
pyiceberg/partitioning.py:
##########
@@ -413,8 +413,10 @@ def partition_record_value(partition_field: PartitionField, value: Any, schema:
the final partition record value.
"""
iceberg_type = schema.find_field(name_or_id=partition_field.source_id).field_type
- iceberg_typed_value = _to_partition_representation(iceberg_type, value)
- transformed_value = partition_field.transform.transform(iceberg_type)(iceberg_typed_value)
+ if not isinstance(value, int):
+ # When adding files, it can be that we still need to convert from logical types to physical types
+ value = _to_partition_representation(iceberg_type, value)
+ transformed_value = partition_field.transform.transform(iceberg_type)(value)
Review Comment:
> So when a date comes in, it also comes out.
is it due to not having support for `datetime` literal? #1542
--
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] PyArrow: Avoid buffer-overflow by avoid doing a sort [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #1555:
URL: https://github.com/apache/iceberg-python/pull/1555#discussion_r1926067731
##########
pyiceberg/partitioning.py:
##########
@@ -413,8 +413,10 @@ def partition_record_value(partition_field: PartitionField, value: Any, schema:
the final partition record value.
"""
iceberg_type = schema.find_field(name_or_id=partition_field.source_id).field_type
- iceberg_typed_value = _to_partition_representation(iceberg_type, value)
- transformed_value = partition_field.transform.transform(iceberg_type)(iceberg_typed_value)
+ if not isinstance(value, int):
+ # When adding files, it can be that we still need to convert from logical types to physical types
+ value = _to_partition_representation(iceberg_type, value)
+ transformed_value = partition_field.transform.transform(iceberg_type)(value)
Review Comment:
> also if its just for adding files, perhaps we can do something special just for that path
Yes, that's exactly what I went for. I think we can simplify the logic in subsequent PRs :)
--
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] PyArrow: Avoid buffer-overflow by avoid doing a sort [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #1555:
URL: https://github.com/apache/iceberg-python/pull/1555
--
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