Fix dynamic_partition_overwrite with partition spec evolution (#3148) - #3863
hedger9487 wants to merge 3 commits into
Conversation
…#3148) * Identify evolved partition fields added in historical partitioned specs * Extend _build_partition_predicate to match IS NULL for evolved fields * Add unit and regression tests for dynamic partition overwrite with spec evolution
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes dynamic_partition_overwrite behavior when partition specs evolve by ensuring delete predicates also match files written under older specs (where newly added partition fields are NULL).
Changes:
- Extend
_build_partition_predicatewith optional handling for evolved partition fields via(... OR IS NULL). - Add logic to detect evolved partition fields across historical partition specs.
- Add unit + regression tests covering evolved-spec predicate building and end-to-end overwrite behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/table/test_init.py | Adds unit/regression tests for evolved partition fields and dynamic partition overwrite behavior. |
| pyiceberg/table/init.py | Implements evolved-field detection and broadens delete predicate construction for dynamic partition overwrite. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| from pyiceberg.catalog.sql import SqlCatalog | ||
|
|
||
| catalog = SqlCatalog(name="test", uri=f"sqlite:///{warehouse.as_posix()}/test_dpo_evolve.db", warehouse=f"file://{warehouse}") |
| 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.""" |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
Closes #3148
Rationale for this change
dynamic_partition_overwritepreviously constructed the delete predicate using only the current partition spec. When a table underwent partition spec evolution (e.g., adding a new partition field), files written under older specs lacked partition values for the newly added field (where the values areNULL).As a result, the
_StrictMetricsEvaluatorwould fail to match older spec files against the new spec's predicate (e.g.,category = 'A' AND region = 'us'), silently skipping them and leaving stale data files behind.This PR:
_build_partition_predicateto includeIS NULLfor evolved fields (e.g.category = 'A' AND (region = 'us' OR region IS NULL)).Are these changes tested?
Yes:
test_build_partition_predicate_with_evolved_fieldstest_dynamic_partition_overwrite_with_partition_spec_evolution(regression test verified to fail without the fix when stale spec-0 rows remain, and pass with this fix).table/,expressions/, andtransforms/pass cleanly.Are there any user-facing changes?
No.