[karubian]

Closes #4323

Rationale for this change

PartitionFieldStats.update takes bounds with Python's min and max, which keep whichever of -0.0 and 0.0 came first. The spec says the lower bound must not be +0.0 when -0.0 is a value, and the upper bound must not be -0.0 when +0.0 is a value: https://iceberg.apache.org/spec/#manifest-lists

After min and max, a zero now replaces a zero bound of the other sign when it belongs there, so -0.0 sorts below 0.0. That is the order Iceberg Java's PartitionSummary gets from Double.compareTo. Nonzero floats and other types take the same path as before.

StatsAggregator in pyiceberg/io/pyarrow.py aggregates data file column bounds with the same min and max. For files PyIceberg writes this is already correct, because Arrow's Parquet writer stores a zero minimum as -0.0 and a zero maximum as +0.0, already in pyarrow 18.0.0, the oldest version PyIceberg supports: https://github.com/apache/arrow/blob/apache-arrow-18.0.0/cpp/src/parquet/statistics.cc#L338-L367. Files from other writers added with add_files may not follow that rule, which I'll raise separately.

#4274 is a different signed zero bug, on the write path, and doesn't overlap with this.

Are these changes tested?

Yes. test_construct_partition_summaries_orders_negative_zero_below_zero builds the summary for [0.0, -0.0] and [-0.0, 0.0], for float and double fields, and fails on main. make lint passes. make test passes apart from four Kerberos tests in tests/catalog/test_hive.py, which need puresasl, and that isn't installed here.

Are there any user-facing changes?

Yes. Manifest lists for float and double partition fields that hold both zeros now get the bounds the spec requires. There is no API change. Please add the changelog label.

AI disclosure: the code, the test and this description were drafted with Claude Code at my direction. I reviewed them and ran lint and the tests.

[dannycjones]

This change makes sense in isolation, LGTM.

I've worked very little with PyIceberg so regular contributor / maintainer feedback is very much desired :)

I have concerns over if this problem is isolated only to creation of partition field summary bounds, but that's broader than this PR.

[dannycjones]

I think we could do better readability here, but I'm not sure on any impact to performance in Python. Not suggesting we make this change, just noting a possible alternative.

Suggested change

	self._max = max(self._max, value)
	self._min = min(self._min, value)
	if isinstance(value, float) and value == 0.0:
	# min and max keep whichever zero came first, but bounds must order -0.0 below 0.0
	if math.copysign(1.0, value) < 0 and self._min == 0.0:
	self._min = value
	elif math.copysign(1.0, value) > 0 and self._max == 0.0:
	self._max = value
	def _bounds_order(value: Any) -> Any:
	# Spec requires IEEE 754 totalOrder: https://iceberg.apache.org/spec/#bounds-for-float-and-double
	# For float, use (value, sign) as the key to compare.
	return (value, math.copysign(1.0, value)) if isinstance(value, float) else value
	self._max = max(self._max, value, key=_bounds_order)
	self._min = min(self._min, value, key=_bounds_order)

[Fokko]

Ran a quick benchmark on this (CPython 3.14, best of 7 runs, 100k updates):

variant	ns/update
key= as written here	~397
key=, key fn defined once outside	~290
current PR (early exit for nonzero)	~73

So the key= form costs ~4x on the hot path — the tuple allocation + key call per comparison adds up — but both are sub-microsecond: for a realistic commit (10k files, 2 partition fields) that's ~0.2 ms difference. Correctness is identical between the two (verified both bounds on the zero edge cases). Either works, the current form just keeps the common path allocation-free. Thanks @dannycjones!

[Fokko]

Nice catch! 🙌 The signed-zero canonicalization looks correct, and the test covers both orderings (and the float/double variants) — verified locally that it passes. Thanks @karubian!