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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
