Repository navigation
Order -0.0 below 0.0 in data file metrics bounds - #4331
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>
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Good one @AP24110011695, I think we can simplify a bit. Let me know what you think
Flip the min/max argument order so a tie keeps the existing value (the same shape as PartitionFieldStats.update in manifest.py), which lets the guard only handle an incoming zero of the wrong sign and drops the previous-value capture. Behavior verified unchanged: the regression tests and all zero-arrival interleavings still produce the canonical -0.0-below-0.0 bounds. Signed-off-by: Ayush Kumar Saha <225407166+AP24110011695@users.noreply.github.com>
|
Applied both suggestions as-is (2e7bd68) — flipping the argument order is clearly better: the tie now keeps the existing value exactly like Verified unchanged behavior: all 22 StatsAggregator tests pass, and I re-probed the zero interleavings (both two-value orders plus |
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Thanks @AP24110011695 for the quick follow up, LGTM 🚀
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.