hcrosse opened a new pull request, #4280: URL: https://github.com/apache/iceberg-python/pull/4280
Closes https://github.com/apache/iceberg-python/issues/4279 # Rationale for this change Scans return deleted rows in NaN identity partitions when one position delete file covers several data files, as Java writers produce by default. Two separately decoded NaN partition values compare as unequal, so the delete index never matches the delete file to its data files. Commit validation misses conflicts in those partitions for the same reason. I kept the list comparison in `Record.__eq__` as a fast path. If it fails, an elementwise comparison treats float NaNs at the same position as equal. The hash is unchanged because NaN records already hash equal through `str`. Dictionary and set lookups now find those records, so the delete index and commit validation match the NaN partitions. The change is limited to NaN. `Record(0.0)` and `Record(-0.0)` still compare equal with different hashes, and so do `Record(1)` and `Record(1.0)`. https://github.com/apache/iceberg-python/pull/4274 covers signed zeros on the write path. `inspect.partitions()` and the residual cache key on plain tuples rather than `Record`, so they are unaffected. The open https://github.com/apache/iceberg-python/pull/3285 keys equality deletes by the same partition `Record`, so its lookup would match NaN partitions too. This change should land before or together with the fix for https://github.com/apache/iceberg-python/issues/4277. That fix lets NaN-partition rewrites reach commit validation. Without this change, a file replacement can commit over a concurrent partition-scoped position delete and permanently restore the deleted row. I measured `==` on three-field records with CPython 3.12, taking the best of seven runs of one million comparisons. Equal finite records go from 101.6 to 97.1 ns, unequal records of the same length go from 101.1 to 532.1 ns, and NaN records take 597.0 ns with the fix. Dictionary and set lookups compare only after a hash match, so partition lookups rarely take that slower path. ## Are these changes tested? I added three tests. - `test_record_equality_with_nan` checks separate and nested NaN records, set deduplication and unequal values. - `test_record_equality_for_nan_partition_lookup` checks that the delete index finds a partition-scoped delete using a separately constructed NaN partition. - `test_validate_data_files_exist_raises_on_conflict_in_nan_partition` uses mocked manifests and checks that `_validate_data_files_exist` rejects a concurrently deleted file in a NaN partition. All three fail on main and pass with the fix. The issue's reproduction now returns `[2, 4, 6, 8]` instead of `[2, 4, 5, 6, 7, 8]`. The full unit suite gives 4479 passed and 5 skipped, with no failures. `prek run -a` passed. Integration tests and an end-to-end concurrent commit were not run. ## Are there any user-facing changes? Yes. Partition-scoped position deletes now apply in NaN partitions, and commit validation recognizes conflicts in those partitions. There is no API change. Please add the `changelog` label. -- 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]
