You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "m0g3r (via GitHub)" <gi...@apache.org> on 2026/08/18 07:01:54 UTC
[PR] fix(avro): read doubles at full precision in the Cython decoder [iceberg-python]
m0g3r opened a new pull request, #3808:
URL: https://github.com/apache/iceberg-python/pull/3808
# Rationale for this change
`CythonBinaryDecoder.read_double` is declared `cpdef float read_double(self)`. In Cython, `float` is the C single-precision type, so the correctly decoded 64-bit value returned by `STRUCT_DOUBLE.unpack` is narrowed to 32 bits on the way out. Every Avro `double` read through the fast decoder is silently rounded, and values outside the single-precision range collapse entirely.
```python
>>> import struct
>>> from pyiceberg.avro.decoder import StreamingBinaryDecoder
>>> from pyiceberg.avro.decoder_fast import CythonBinaryDecoder
>>> b = struct.pack("<d", 3.141592653589793)
>>> StreamingBinaryDecoder(b).read_double()
3.141592653589793
>>> CythonBinaryDecoder(b).read_double() # before this PR
3.1415927410125732
>>> CythonBinaryDecoder(struct.pack("<d", 1e308)).read_double()
inf
>>> CythonBinaryDecoder(struct.pack("<d", 5e-324)).read_double()
0.0
```
`new_decoder` returns `CythonBinaryDecoder` whenever the extension is built, so this is the default read path; the pure-Python `StreamingBinaryDecoder` fallback was always correct.
The user-visible effect is on any `double` read out of a manifest, most directly an identity partition value on a `float`/`double` column. Writing a manifest with a partition value of `429496729622.314` and reading it back through `ManifestFile.fetch_manifest_entry` returns `429496729600.0` on `main`, and the exact value with this change.
`read_float` is left as `cpdef float`: a value decoded from four bytes is already exactly representable as a C float, so no precision is lost there.
The bug dates back to the original Cython decoder (#8134, 2023). It was not caught by `tests/avro/test_decoder.py::test_read_double` because `19.25` is exactly representable in single precision. `tests/avro/test_file.py::test_all_primitive_types` does round-trip a `double` that is not, but its assertion loop iterates `enumerate(all_primitives_schema.as_struct())` — iterating the pydantic model yields its two model fields (`type`, `fields`), not the 13 schema fields — so only positions 0 and 1 were ever compared. I kept that out of this PR to keep it to one concern, and am happy to send the test fix as a follow-up (or fold it in here if you would rather).
## Are these changes tested?
Yes. `tests/avro/test_decoder.py` gains `test_read_double_keeps_full_precision`, parametrized over both decoder implementations and six doubles that are not representable in single precision, including the 1e308 overflow and 5e-324 underflow cases.
Verified red/green by rebuilding the extension against the pre-change `decoder_fast.pyx`:
- before: `6 failed, 8 passed` — every failure is the `CythonBinaryDecoder` parametrization, e.g. `assert 0.0 == 5e-324`
- after: `56 passed` in `tests/avro/test_decoder.py`
Full local run on macOS/arm64, Python 3.13: `make lint` all 12 hooks pass, `make test` gives `3957 passed, 3 skipped, 1570 deselected`.
Integration tests (Spark/Docker) were not run locally.
## Are there any user-facing changes?
Yes — `double` values read from Avro are no longer rounded to single precision. This is a bug fix; existing manifests do not need to be rewritten, since the data on disk was always correct and only the decode was lossy.
---
Disclosure: this change was written with AI assistance (Claude Code). The bug was found by auditing the Cython decoder's C return types, then confirmed against the pure-Python decoder and end to end through a manifest write/read round trip.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(avro): read doubles at full precision in the Cython decoder [iceberg-python]
Posted by "ebyhr (via GitHub)" <gi...@apache.org>.
ebyhr commented on code in PR #3808:
URL: https://github.com/apache/iceberg-python/pull/3808#discussion_r3801916103
##########
tests/avro/test_decoder.py:
##########
@@ -160,6 +160,23 @@ def test_read_double(decoder_class: Callable[[bytes], BinaryDecoder]) -> None:
assert decoder.read_double() == 19.25
+@pytest.mark.parametrize("decoder_class", AVAILABLE_DECODERS)
+@pytest.mark.parametrize(
+ "value",
+ [
+ 3.141592653589793,
+ 429496729622.314,
+ 0.1,
+ 1.0000000000000002, # smallest double above 1.0
+ 1e308, # overflows to inf in single precision
+ 5e-324, # underflows to 0.0 in single precision
+ ],
+)
+def test_read_double_keeps_full_precision(decoder_class: Callable[[bytes], BinaryDecoder], value: float) -> None:
Review Comment:
I don't think this is a valid regression test. It passes even if we revert `decoder_fast.pyx`'s change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(avro): read doubles at full precision in the Cython decoder [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #3808:
URL: https://github.com/apache/iceberg-python/pull/3808#discussion_r3907787099
##########
tests/avro/test_decoder.py:
##########
@@ -160,6 +160,23 @@ def test_read_double(decoder_class: Callable[[bytes], BinaryDecoder]) -> None:
assert decoder.read_double() == 19.25
+@pytest.mark.parametrize("decoder_class", AVAILABLE_DECODERS)
+@pytest.mark.parametrize(
+ "value",
+ [
+ 3.141592653589793,
+ 429496729622.314,
+ 0.1,
+ 1.0000000000000002, # smallest double above 1.0
+ 1e308, # overflows to inf in single precision
+ 5e-324, # underflows to 0.0 in single precision
+ ],
+)
+def test_read_double_keeps_full_precision(decoder_class: Callable[[bytes], BinaryDecoder], value: float) -> None:
Review Comment:
I'm seeing the issue as well:
```
__________________________________________________ test_read_double_keeps_full_precision[3.141592653589793-CythonBinaryDecoder] ___________________________________________________
decoder_class = <class 'pyiceberg.avro.decoder_fast.CythonBinaryDecoder'>, value = 3.141592653589793
@pytest.mark.parametrize("decoder_class", AVAILABLE_DECODERS)
@pytest.mark.parametrize(
"value",
[
3.141592653589793,
429496729622.314,
0.1,
1.0000000000000002, # smallest double above 1.0
1e308, # overflows to inf in single precision
5e-324, # underflows to 0.0 in single precision
],
)
def test_read_double_keeps_full_precision(decoder_class: Callable[[bytes], BinaryDecoder], value: float) -> None:
decoder = decoder_class(struct.pack("<d", value))
> assert decoder.read_double() == value
E assert 3.1415927410125732 == 3.141592653589793
E + where 3.1415927410125732 = read_double()
E + where read_double = <pyiceberg.avro.decoder_fast.CythonBinaryDecoder object at 0x11d2abe70>.read_double
tests/avro/test_decoder.py:221: AssertionError
```
`make clean` should give you a clean state.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(avro): read doubles at full precision in the Cython decoder [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #3808:
URL: https://github.com/apache/iceberg-python/pull/3808
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(avro): read doubles at full precision in the Cython decoder [iceberg-python]
Posted by "m0g3r (via GitHub)" <gi...@apache.org>.
m0g3r commented on code in PR #3808:
URL: https://github.com/apache/iceberg-python/pull/3808#discussion_r3838118008
##########
tests/avro/test_decoder.py:
##########
@@ -160,6 +160,23 @@ def test_read_double(decoder_class: Callable[[bytes], BinaryDecoder]) -> None:
assert decoder.read_double() == 19.25
+@pytest.mark.parametrize("decoder_class", AVAILABLE_DECODERS)
+@pytest.mark.parametrize(
+ "value",
+ [
+ 3.141592653589793,
+ 429496729622.314,
+ 0.1,
+ 1.0000000000000002, # smallest double above 1.0
+ 1e308, # overflows to inf in single precision
+ 5e-324, # underflows to 0.0 in single precision
+ ],
+)
+def test_read_double_keeps_full_precision(decoder_class: Callable[[bytes], BinaryDecoder], value: float) -> None:
Review Comment:
Thanks for checking. The test does fail on the unfixed tree, but only if the Cython extension is actually rebuilt — and `setup.py` makes that easy to miss.
The `cythonize` call is wrapped in `try/except Exception` with `allowed_to_fail = os.environ.get("CIBUILDWHEEL", "0") != "1"`, so if the interpreter running it can't import Cython, `ext_modules` stays `[]` and `build_ext --inplace` prints `running build_ext`, exits 0, and builds nothing. The previously built `decoder_fast.*.so` stays on disk with the fix compiled into it, and the test passes against that stale artifact.
Reverting the source and forcing a real rebuild:
```console
$ git checkout HEAD~1 -- pyiceberg/avro/decoder_fast.pyx # back to cpdef float
$ .venv/bin/python setup.py build_ext --inplace
Compiling pyiceberg/avro/decoder_fast.pyx because it changed.
building 'pyiceberg.avro.decoder_fast' extension
$ .venv/bin/python -m pytest tests/avro/test_decoder.py -k full_precision -q
6 failed, 6 passed, 44 deselected
```
All six failures are the `CythonBinaryDecoder` parameters. The six `StreamingBinaryDecoder` ones pass, which is expected — the pure-Python decoder was always correct. Decoded values on the unfixed build:
```
3.141592653589793 -> 3.1415927410125732
1e+308 -> inf
5e-324 -> 0.0
```
With the fix restored and rebuilt, `tests/avro/test_decoder.py` is 56 passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org