Repository navigation
Conversation
_determine_partitions groups rows by partition value and then selects each group with an equality filter. group_by keeps -0.0 and 0.0 apart, but -0.0 == 0.0, so both partitions selected the rows of both, and every such row was written twice. Match the sign of zero as well, the same way NaN is already selected with is_nan.
This branch has not been deployed
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.
Rationale for this change
Appending to a table with an identity partition on a
doubleorfloatcolumn writes some rows twice when the data holds both-0.0and0.0. Signed zeros are easy to get, for example by rounding small negative values:On
main(and on 0.12.0), the three rows come back as five:With this change:
_determine_partitionsgroups the rows by partition value, then selects each group's rows withpc.field(name) == value.group_bykeeps-0.0and0.0as two groups, but-0.0 == 0.0is true, so each of the two filters selects the rows of both groups, and those rows are written into both data files. #4030 fixed the NaN case of this filter, where NaN never equals itself and its partition came out empty. This PR handles the opposite case: for a zero partition value, the filter also matches the sign of zero, using1 / x, which is-inffor-0.0andinffor0.0.The partitioning itself is unchanged.
-0.0and0.0are still two partitions, as before. Each row now lands in exactly one of them.overwrite()anddynamic_partition_overwrite()write through the same function and had the same problem (3 rows in, 5 read back). Both return 3 rows with this change.Are these changes tested?
Yes.
tests/io/test_pyarrow.py::test_determine_partitions_identity_signed_zerochecks, for bothdoubleandfloat, that the-0.0,0.0and1.0rows each land in exactly one partition. Onmain, both zero partitions hold rows[1, 2, 4].tests/catalog/test_catalog_behaviors.py::test_append_signed_zero_to_identity_partitioned_tableappends to a real table (memory and SQL catalogs). It checks that a full scan returns each row once and thatvalue = 0returns both zero rows. Onmain, the scan returns[1, 1, 2, 2, 3].All 4 new test cases fail on
mainand pass with the fix.What I ran locally:
tests/io/test_pyarrow.pyandtests/catalog/test_catalog_behaviors.py: 931 passed, 3 skipped;tests/table/test_partitioning.pyandtests/catalog/test_sql.py: 31 passed;I also checked the new filter expression on pyarrow 18.0.0, the minimum supported version. CI covers the rest of the suite and the integration tests.
Are there any user-facing changes?
Yes, a bug fix. Appending, overwriting or dynamically overwriting data that holds both
-0.0and0.0into an identity-partitionedfloatordoublecolumn no longer writes those rows twice. Rows that were already duplicated in existing tables stay as they are.