Fix dynamic_partition_overwrite with partition spec evolution (#3148) - #3863
Open
hedger9487 wants to merge 3 commits into
Open
Fix dynamic_partition_overwrite with partition spec evolution (#3148)#3863hedger9487 wants to merge 3 commits into
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
Contributor
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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.