hcrosse opened a new pull request, #4281: URL: https://github.com/apache/iceberg-python/pull/4281
Closes https://github.com/apache/iceberg-python/issues/4277 # Rationale for this change `delete()`, `overwrite()`, `upsert()` and `dynamic_partition_overwrite()` raise `ValueError: Cannot create expression literal from NaN` when they touch a NaN identity partition. `_build_partition_predicate` builds `EqualTo` for each non-NULL partition value, and expression literals reject NaN. I changed `_build_partition_predicate` to emit `IsNaN` for NaN partition values next to the existing `IsNull` branch. This PR depends on https://github.com/apache/iceberg-python/pull/4280. On main, `Record` compares two NaN partition values as unequal, so commit validation misses a concurrent position delete in a NaN partition. Today the replacement raises before validation runs. With only this change, a stale file replacement would commit over that delete and bring the deleted row back. This PR includes both commits, and only the second one belongs to it. Dynamic overwrites with NaN keys used to raise. They can now reach two existing bugs, both of which need a file written under an older partition spec. - With both NaN and NULL keys, a dynamic overwrite can drop a NULL row that matches none of the incoming keys. This is https://github.com/apache/iceberg-python/issues/4184. A hand-written `delete()` filter already loses the row on main. The open https://github.com/apache/iceberg-python/pull/4219 keeps the row for that filter. I did not test the dynamic-overwrite case against it. - With two partition columns, a dynamic overwrite can drop a whole old file, including rows that match none of the incoming keys, because PyIceberg writes no NaN counts. This is https://github.com/apache/iceberg-python/issues/4130. Finite keys already reach it on main. `upsert()` with NaN in a join column still raises, because [`create_match_filter`](https://github.com/apache/iceberg-python/blob/e3564a54c10960307147377c578680276865ed18/pyiceberg/table/upsert_util.py#L33-L48) builds literals from the join keys. That path is separate from this change. ## Are these changes tested? Yes. I added three tests. - `test_replace_files_in_nan_identity_partition` covers whole-file deletion, partial rewriting, overwrite and dynamic overwrite in a NaN identity partition, on the memory and SQL catalogs. - `test_build_partition_predicate_with_nan_and_null` checks the predicate for NaN, NULL and finite values. - `test_file_overwrite_validates_concurrent_position_delete_in_nan_partition` stages a file replacement, then commits a partition-scoped position delete from another table handle. It checks that the replacement fails validation and the deleted row stays gone, on the memory and SQL catalogs. PyIceberg cannot write delete files, so the helper commits the delete through `overwrite()` with `_OverwriteFiles.new_manifest_writer` patched to write a delete manifest. All three fail on main with the NaN literal error and pass with both commits. Without the dependency, the concurrent-delete test fails because the stale replacement commits. I also ran `upsert()` on a row in the NaN partition and `delete(AlwaysTrue())`. Both raise on main and succeed with the fix. These cases are not in the committed tests. The full unit suite with both commits gives 4482 passed, 4 failed and 5 skipped, versus 4472 passed, 4 failed and 5 skipped on main. Three of the ten new passes come from the dependency and seven from this PR. The same four Hive Kerberos tests fail on both because the `hive-kerberos` extra cannot be installed in the test environment. `prek run -a` passed. ## Are there any user-facing changes? Yes. `delete()`, `overwrite()`, `upsert()` and `dynamic_partition_overwrite()` now work on NaN identity partitions, apart from the cases above. 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]
