bug: fix Upsert duplicated rows on partitioned tables - #3780
Conversation
|
The test cases we had which showed the #3758 issue, in upsert and delete, all pass cleanly with this. Additional tests - no package code change - which cover the code paths not hit by the upsert test. Cherry pick into this PR if you want, or I can open an issue to contribute them. These show RED in 0.12rc1 and GREEN with this PR. |
|
|
||
| def _build_partition_predicate( | ||
| self, partition_records: set[Record], spec: PartitionSpec, schema: Schema | ||
| self, partition_records: set[Record], spec: PartitionSpec, schema: Schema, is_projected: bool = False |
There was a problem hiding this comment.
what do you think about https://github.com/apache/iceberg-python/compare/main...kevinjqliu:iceberg-python:kevinjqliu/codex-overwrite-pruning-regression-tests?expand=1?
I like that we can use _OverwriteFiles.partition_filters as the api boundary. And we can skip the new is_projected state
There was a problem hiding this comment.
I like the idea of reusing Transaction._build_partition_predicate here, but I agree the is_projected flag isn't the cleanest way to express that. Looking at the link build_partition_record_filter ends up duplicating most of the same logic.
We could have a single shared _build_partition_predicate, add two thin wrapper methods that call it with different field sets (schema-based vs. spec-based) so we dont have duplicate logic
wdyt?
There was a problem hiding this comment.
I personally like that a lot. I'm wondering if we actually need the wrapper methods and couldn't just pass in the field sets?
There was a problem hiding this comment.
oh yes, they do look similar. lets reuse it if we can. codex mentioned something about this, but i forgot. lets give it a try
There was a problem hiding this comment.
sounds good Ill make the changes will push again in a bit
12fd8eb to
f1a5464
Compare
f1a5464 to
e14a8dc
Compare
4 participants
Closes #3758
Rationale for this change
_build_delete_files_partition_predicatecollectsdata_file.partitionrecords from the deleted files. Those records hold already transformed values, but it passed them toTransaction._build_partition_predicate, which names fields by the source column. So the values gets transformed a second time.Are these changes tested?
Yes added a regression test for upsert.
Are there any user-facing changes?
Yes, it fixes the undefined behavior in upsert