geruh commented on code in PR #3264:
URL: https://github.com/apache/iceberg-python/pull/3264#discussion_r3120972742
##########
tests/table/test_init.py:
##########
@@ -1753,3 +1753,19 @@ def test_check_uuid_passes_when_match(table_v2: Table,
example_table_metadata_v2
new_metadata = TableMetadataV2(**example_table_metadata_v2)
# Should not raise with same uuid
Table._check_uuid(table_v2.metadata, new_metadata)
+
+
+def test_build_large_partition_predicate(table_v2: Table) -> None:
+ """A left-folded Or chain over 5000 records would be depth-5000 and crash
bind()
+ (Python's default recursion limit is ~1000). The balanced tree has depth
~14."""
+ from pyiceberg.expressions.visitors import bind
Review Comment:
nit: can we import these at the class level?
##########
pyiceberg/table/__init__.py:
##########
@@ -366,21 +366,19 @@ def _build_partition_predicate(
Returns:
A predicate matching any of the input partition records.
"""
- partition_fields = [schema.find_field(field.source_id).name for field
in spec.fields]
-
- expr: BooleanExpression = AlwaysFalse()
- for partition_record in partition_records:
- match_partition_expression: BooleanExpression = AlwaysTrue()
-
- for pos, partition_field in enumerate(partition_fields):
- predicate = (
- EqualTo(Reference(partition_field), partition_record[pos])
- if partition_record[pos] is not None
- else IsNull(Reference(partition_field))
- )
- match_partition_expression = And(match_partition_expression,
predicate)
- expr = Or(expr, match_partition_expression)
- return expr
+ partition_fields = [schema.find_field(f.source_id).name for f in
spec.fields]
Review Comment:
nit: don't need to make this style diff so we don't lose blame history.
##########
tests/table/test_init.py:
##########
@@ -1753,3 +1753,19 @@ def test_check_uuid_passes_when_match(table_v2: Table,
example_table_metadata_v2
new_metadata = TableMetadataV2(**example_table_metadata_v2)
# Should not raise with same uuid
Table._check_uuid(table_v2.metadata, new_metadata)
+
+
+def test_build_large_partition_predicate(table_v2: Table) -> None:
+ """A left-folded Or chain over 5000 records would be depth-5000 and crash
bind()
Review Comment:
Nit: idk how I feel about describing the old behavior in the docstring. I
think it would be better to describe the current situation, i.e.
```python
"""Partition predicates over many records must bind without
RecursionError."""
```
But at that point the test name already says that and makes me wonder if the
docstring is needed maybe we could get rid of it. Again non-blocking and
thinkning out loud.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
