From a1d7fb7e4902708c3011eb9f469d7854a7b95039 Mon Sep 17 00:00:00 2001 From: Vishnu Prakash Date: Wed, 17 Jun 2026 18:52:32 +0530 Subject: [PATCH 1/3] fix(decimal): use minimal byte length for negative powers of two --- pyiceberg/utils/decimal.py | 13 +++++++++---- tests/utils/test_decimal.py | 23 ++++++++++++++++++++++- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/pyiceberg/utils/decimal.py b/pyiceberg/utils/decimal.py index 5ef82640d9..83b9bda498 100644 --- a/pyiceberg/utils/decimal.py +++ b/pyiceberg/utils/decimal.py @@ -58,11 +58,16 @@ def bytes_required(value: int | Decimal) -> int: int: the minimum number of bytes needed to serialize the value. """ if isinstance(value, int): - return (value.bit_length() + 8) // 8 + unscaled = value elif isinstance(value, Decimal): - return (decimal_to_unscaled(value).bit_length() + 8) // 8 - - raise ValueError(f"Unsupported value: {value}") + unscaled = decimal_to_unscaled(value) + else: + raise ValueError(f"Unsupported value: {value}") + + # bit_length() overcounts negatives equal to -2**(8k-1) (e.g. -128, -32768) by one byte; + # using (unscaled + 1) for negatives yields the true minimum, matching the Iceberg spec. + n_bits = unscaled.bit_length() if unscaled >= 0 else (unscaled + 1).bit_length() + return (n_bits + 8) // 8 def decimal_to_bytes(value: Decimal, byte_length: int | None = None) -> bytes: diff --git a/tests/utils/test_decimal.py b/tests/utils/test_decimal.py index 3e67bf691a..c08b3e6414 100644 --- a/tests/utils/test_decimal.py +++ b/tests/utils/test_decimal.py @@ -18,7 +18,7 @@ import pytest -from pyiceberg.utils.decimal import decimal_required_bytes, decimal_to_bytes +from pyiceberg.utils.decimal import bytes_required, decimal_required_bytes, decimal_to_bytes def test_decimal_required_bytes() -> None: @@ -42,8 +42,29 @@ def test_decimal_required_bytes() -> None: assert "(0, 40]" in str(exc_info.value) +def test_bytes_required() -> None: + # Positive values and the negative values just past a byte boundary are unaffected. + assert bytes_required(0) == 1 + assert bytes_required(127) == 1 + assert bytes_required(128) == 2 + assert bytes_required(-127) == 1 + assert bytes_required(-129) == 2 + # The most-negative value that fits in N bytes (-2**(8N-1)) must require exactly N bytes, + # not N + 1. These are the cases the previous (value.bit_length() + 8) // 8 formula overcounted. + assert bytes_required(-128) == 1 + assert bytes_required(-32768) == 2 + assert bytes_required(-8388608) == 3 + # The same applies when the unscaled value comes from a Decimal. + assert bytes_required(Decimal("-1.28")) == 1 + assert bytes_required(Decimal("-327.68")) == 2 + + def test_decimal_to_bytes() -> None: # Check the boundary between 2 and 3 bytes. # 2 bytes has a minimum of -32,768 and a maximum value of 32,767 (inclusive). assert decimal_to_bytes(Decimal("32767.")) == b"\x7f\xff" assert decimal_to_bytes(Decimal("32768.")) == b"\x00\x80\x00" + # The most-negative value for a given width must serialize to the minimum number of bytes + # (matching the Iceberg spec / Java BigInteger.toByteArray), not one byte longer. + assert decimal_to_bytes(Decimal("-1.28")) == b"\x80" + assert decimal_to_bytes(Decimal("-327.68")) == b"\x80\x00" From b2f06b901a71a08a5cf2fc0edf0443fdd604dc10 Mon Sep 17 00:00:00 2001 From: Vishnu Prakash Date: Thu, 18 Jun 2026 21:58:42 +0530 Subject: [PATCH 2/3] test(decimal): add positive Decimal case to bytes_required --- tests/utils/test_decimal.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/utils/test_decimal.py b/tests/utils/test_decimal.py index c08b3e6414..370d767870 100644 --- a/tests/utils/test_decimal.py +++ b/tests/utils/test_decimal.py @@ -55,6 +55,7 @@ def test_bytes_required() -> None: assert bytes_required(-32768) == 2 assert bytes_required(-8388608) == 3 # The same applies when the unscaled value comes from a Decimal. + assert bytes_required(Decimal("1.27")) == 1 assert bytes_required(Decimal("-1.28")) == 1 assert bytes_required(Decimal("-327.68")) == 2 From ae51a03f54216a0ca8d7ca4cc633bb0ab0a14730 Mon Sep 17 00:00:00 2001 From: Kevin Liu Date: Thu, 20 Aug 2026 11:06:43 -0700 Subject: [PATCH 3/3] test(decimal): cover negative boundary bucketing --- pyiceberg/utils/decimal.py | 8 ++++---- tests/test_transforms.py | 1 + tests/utils/test_decimal.py | 10 +++------- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/pyiceberg/utils/decimal.py b/pyiceberg/utils/decimal.py index 83b9bda498..0ed2de1d5b 100644 --- a/pyiceberg/utils/decimal.py +++ b/pyiceberg/utils/decimal.py @@ -64,10 +64,10 @@ def bytes_required(value: int | Decimal) -> int: else: raise ValueError(f"Unsupported value: {value}") - # bit_length() overcounts negatives equal to -2**(8k-1) (e.g. -128, -32768) by one byte; - # using (unscaled + 1) for negatives yields the true minimum, matching the Iceberg spec. - n_bits = unscaled.bit_length() if unscaled >= 0 else (unscaled + 1).bit_length() - return (n_bits + 8) // 8 + # bit_length() ignores the sign, so -128 appears to need 9 signed bits instead of 8. + # Adding 1 before counting avoids the extra byte at negative boundaries. + num_bits = unscaled.bit_length() if unscaled >= 0 else (unscaled + 1).bit_length() + return (num_bits + 8) // 8 def decimal_to_bytes(value: Decimal, byte_length: int | None = None) -> bytes: diff --git a/tests/test_transforms.py b/tests/test_transforms.py index d296fcdb21..c977fcea14 100644 --- a/tests/test_transforms.py +++ b/tests/test_transforms.py @@ -187,6 +187,7 @@ def test_bucket_hash_values(test_input: Any, test_type: PrimitiveType, expected: (BucketTransform(100).transform(TimeType()), 81068000000, 59), (BucketTransform(100).transform(TimestampType()), 1510871468000000, 7), (BucketTransform(100).transform(DecimalType(9, 2)), Decimal("14.20"), 59), + (BucketTransform(16).transform(DecimalType(5, 2)), Decimal("-1.28"), 13), (BucketTransform(100).transform(StringType()), "iceberg", 89), ( BucketTransform(100).transform(UUIDType()), diff --git a/tests/utils/test_decimal.py b/tests/utils/test_decimal.py index 370d767870..d50f7c5707 100644 --- a/tests/utils/test_decimal.py +++ b/tests/utils/test_decimal.py @@ -43,18 +43,15 @@ def test_decimal_required_bytes() -> None: def test_bytes_required() -> None: - # Positive values and the negative values just past a byte boundary are unaffected. assert bytes_required(0) == 1 assert bytes_required(127) == 1 assert bytes_required(128) == 2 + # Check negative signed-byte boundaries and their neighbors. assert bytes_required(-127) == 1 - assert bytes_required(-129) == 2 - # The most-negative value that fits in N bytes (-2**(8N-1)) must require exactly N bytes, - # not N + 1. These are the cases the previous (value.bit_length() + 8) // 8 formula overcounted. assert bytes_required(-128) == 1 + assert bytes_required(-129) == 2 assert bytes_required(-32768) == 2 assert bytes_required(-8388608) == 3 - # The same applies when the unscaled value comes from a Decimal. assert bytes_required(Decimal("1.27")) == 1 assert bytes_required(Decimal("-1.28")) == 1 assert bytes_required(Decimal("-327.68")) == 2 @@ -65,7 +62,6 @@ def test_decimal_to_bytes() -> None: # 2 bytes has a minimum of -32,768 and a maximum value of 32,767 (inclusive). assert decimal_to_bytes(Decimal("32767.")) == b"\x7f\xff" assert decimal_to_bytes(Decimal("32768.")) == b"\x00\x80\x00" - # The most-negative value for a given width must serialize to the minimum number of bytes - # (matching the Iceberg spec / Java BigInteger.toByteArray), not one byte longer. + # Unscaled values -128 and -32768 require no sign padding. assert decimal_to_bytes(Decimal("-1.28")) == b"\x80" assert decimal_to_bytes(Decimal("-327.68")) == b"\x80\x00"