Copilot commented on code in PR #3863:
URL: https://github.com/apache/iceberg-python/pull/3863#discussion_r3861774680
##########
tests/table/test_init.py:
##########
@@ -2036,3 +2040,71 @@ def _spy(*args: Any, **kwargs: Any) -> FileIO:
assert seen_locations, "expected at least one load_file_io call"
assert all(loc is not None for loc in seen_locations), f"load_file_io
called without a location: {seen_locations}"
+
+
+def test_build_partition_predicate_with_evolved_fields(table_v2: Table) ->
None:
+ tx = table_v2.transaction()
+ records = {Record("A", "us")}
+ fields = ["category", "region"]
+
+ # Without evolved fields
+ pred = tx._build_partition_predicate(records, fields)
+ assert pred == And(EqualTo(Reference("category"), "A"),
EqualTo(Reference("region"), "us"))
+
+ # With evolved fields
+ pred_evolved = tx._build_partition_predicate(records, fields,
evolved_fields={"region"})
+ assert pred_evolved == And(
+ EqualTo(Reference("category"), "A"),
+ Or(EqualTo(Reference("region"), "us"), IsNull(Reference("region"))),
+ )
+
+
+def test_dynamic_partition_overwrite_with_partition_spec_evolution(warehouse:
Path) -> None:
+ import pyarrow as pa
+
+ from pyiceberg.catalog.sql import SqlCatalog
+
+ catalog = SqlCatalog(name="test",
uri=f"sqlite:///{warehouse.as_posix()}/test_dpo_evolve.db",
warehouse=f"file://{warehouse}")
Review Comment:
`warehouse=f"file://{warehouse}"` produces an invalid/ambiguous file URI for
absolute paths (it becomes `file:////tmp/...` because `str(Path)` starts with
`/`). Use `warehouse.as_uri()` (or equivalent) to generate a correct
`file:///...` URI so the catalog consistently resolves the warehouse location
across platforms.
##########
pyiceberg/table/__init__.py:
##########
@@ -390,31 +395,61 @@ def _set_ref_snapshot(
return updates, requirements
- def _build_partition_predicate(self, partition_records: set[Record],
partition_fields: list[str]) -> BooleanExpression:
+ def _build_partition_predicate(
+ self,
+ partition_records: set[Record],
+ partition_fields: list[str],
+ evolved_fields: set[str] | None = None,
+ ) -> BooleanExpression:
"""Build a filter predicate matching any of the input partition
records.
Args:
partition_records: A set of partition records to match
partition_fields: The field names to reference for each position
in a partition record
+ evolved_fields: Optional set of field names added during partition
spec evolution
Returns:
A predicate matching any of the input partition records.
"""
if not partition_records or not partition_fields:
return AlwaysFalse()
+ evolved = evolved_fields or set()
per_record_exprs: list[BooleanExpression] = []
for partition_record in partition_records:
- predicates: list[BooleanExpression] = [
- EqualTo(Reference(partition_field), partition_record[pos])
- if partition_record[pos] is not None
- else IsNull(Reference(partition_field))
- for pos, partition_field in enumerate(partition_fields)
- ]
+ predicates: list[BooleanExpression] = []
+ for pos, field in enumerate(partition_fields):
+ ref = Reference(field)
+ val = partition_record[pos]
+ if val is None:
+ predicates.append(IsNull(ref))
+ elif field in evolved:
+ predicates.append(Or(EqualTo(ref, val), IsNull(ref)))
+ else:
+ predicates.append(EqualTo(ref, val))
+
per_record_exprs.append(And(*predicates) if len(predicates) > 1
else predicates[0])
return Or(*per_record_exprs) if len(per_record_exprs) > 1 else
per_record_exprs[0]
+ def _get_evolved_partition_fields(self, current_spec: PartitionSpec) ->
set[str]:
+ """Find partition fields in the current spec that were absent in any
historical partitioned spec."""
Review Comment:
The docstring is a bit ambiguous: “absent in any historical” can be read as
“absent in at least one historical spec” or “absent in all historical specs.”
Since the implementation uses an intersection across historical specs (i.e.,
treats fields as evolved if they’re missing from at least one historical spec),
consider clarifying the wording (e.g., “absent in at least one historical
partitioned spec” / “not present in all historical partitioned specs”) to match
the actual behavior.
--
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]
