Repository navigation
Order -0.0 below 0.0 in data file metrics bounds - #4331
Open
AP24110011695 wants to merge 1 commit into
Open
AP24110011695 wants to merge 1 commit into
AP24110011695 wants to merge 1 commit into
Conversation
StatsAggregator.update_min/update_max feed DataFile lower/upper bounds, but min(val, current) and max(val, current) keep the first argument on a tie, so whichever zero arrives last wins: a late -0.0 lowers an existing +0.0 upper bound, and a late +0.0 raises an existing -0.0 lower bound. The serialized bounds then order above a value present in the file. Mirror the fix that apache#4324 applied to partition field summaries: restore the stored bound when a zero arrives that would invert the -0.0 < 0.0 ordering. The argument order here (val first) flips which direction needs the guard compared to manifest.py. Verified failing-first: the new tests fail on main for both arrival orders and both float types, and pass with the guard. Signed-off-by: Ayush Kumar Saha <225407166+AP24110011695@users.noreply.github.com>
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.
What's changed
StatsAggregator.update_min/update_maxaggregate parquet row-group statistics intoDataFile.lower_bounds/upper_bounds. On a tie between+0.0and-0.0,min(val, current)andmax(val, current)keep the first argument — so whichever zero arrives last wins:-0.0lowers an existing+0.0upper bound (e.g. row group 1 contains+0.0, a later row group's max is-0.0),+0.0raises an existing-0.0lower bound.The serialized bounds then order above a value that is present in the file, violating the two's-comparable bound ordering.
#4324 fixed the same class in the partition-field summary path (
PartitionFieldStats.updateinmanifest.py); this applies the equivalent guard to the data-file metrics path. The argument order here is flipped (min(val, current)vsmin(self._min, value)), so the guarded directions are mirrored, and the guard is split across the two update methods. Verified against #4324's pinned bar: for both arrival orders the bounds must serialize aslower = to_bytes(-0.0),upper = to_bytes(+0.0).NaN needs no handling here: pyarrow row-group statistics exclude NaN (
has_min_maxisFalsefor all-NaN chunks), so the aggregator never sees it on this path.Test
test_stats_aggregator_orders_negative_zero_below_zero— parametrized over both arrival orders andFloatType/DoubleType, asserting the serialized bounds match the #4324 canonical pair. Verified failing-first: 4/4 fail onmain(533a617) and pass with the guard; the existingStatsAggregatortests (18) still pass.AI disclosure: code and tests drafted with AI assistance, verified locally by running the full failing-first cycle.