Skip to content

Order -0.0 below 0.0 in partition field summary bounds #4324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Fokko merged 1 commit into from
Nov 10, 2026
Merged

Order -0.0 below 0.0 in partition field summary bounds #4324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pyiceberg/manifest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -736,6 +736,12 @@ def update(self, value: Any) -> None:
else:
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
Comment on lines 737 to +744

@dannycjones dannycjones Nov 10, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Fokko Nov 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!



def construct_partition_summaries(spec: PartitionSpec, schema: Schema, partitions: list[Record]) -> list[PartitionFieldSummary]:
Expand Down
22 changes: 20 additions & 2 deletions tests/utils/test_manifest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import pyiceberg.manifest as manifest_module
from pyiceberg.avro.codecs import AvroCompressionCodec
from pyiceberg.avro.file import DEFAULT_SYNC_INTERVAL, AvroFile, AvroOutputFile
from pyiceberg.conversions import to_bytes
from pyiceberg.encryption.key_metadata import StandardKeyMetadata
from pyiceberg.encryption.manager import EncryptionManager
from pyiceberg.encryption.stream import _encrypt_ags1
Expand All @@ -47,15 +48,17 @@
_inherit_from_manifest,
_manifests,
clear_manifest_cache,
construct_partition_summaries,
read_manifest_list,
write_manifest,
write_manifest_list,
)
from pyiceberg.partitioning import UNPARTITIONED_PARTITION_SPEC, PartitionSpec
from pyiceberg.partitioning import UNPARTITIONED_PARTITION_SPEC, PartitionField, PartitionSpec
from pyiceberg.schema import Schema
from pyiceberg.table.snapshots import Operation, Snapshot, Summary
from pyiceberg.transforms import IdentityTransform
from pyiceberg.typedef import Record, TableVersion
from pyiceberg.types import IntegerType, ListType, LongType, NestedField, StructType
from pyiceberg.types import DoubleType, FloatType, IntegerType, ListType, LongType, NestedField, StructType
from tests.encryption.memory_kms import MemoryKeyManagementClient


Expand Down Expand Up @@ -1365,6 +1368,21 @@ def test_write_manifest_min_sequence_number_zero(format_version: TableVersion) -
assert manifest_file.min_sequence_number == 0


def test_construct_partition_summaries_orders_negative_zero_below_zero() -> None:
spec = PartitionSpec(PartitionField(source_id=1, field_id=1000, transform=IdentityTransform(), name="x"))
for field_type in (FloatType(), DoubleType()):
schema = Schema(NestedField(1, "x", field_type, required=False))
expected = PartitionFieldSummary.from_args(
contains_null=False,
contains_nan=False,
lower_bound=to_bytes(field_type, -0.0),
upper_bound=to_bytes(field_type, 0.0),
)
for values in ([0.0, -0.0], [-0.0, 0.0]):
summaries = construct_partition_summaries(spec, schema, [Record(value) for value in values])
assert summaries == [expected], (field_type, values)


def test_inherit_from_manifest_snapshot_id() -> None:
entry = ManifestEntry.from_args(
status=ManifestEntryStatus.ADDED,
Expand Down
Loading