fix(avro): read doubles at full precision in the Cython decoder - #3808
fix(avro): read doubles at full precision in the Cython decoder#3808m0g3r wants to merge 2 commits into
Conversation
Sorry, something went wrong.
`CythonBinaryDecoder.read_double` was declared `cpdef float`, which in Cython is the C single-precision type, so every Avro double decoded by the fast decoder was silently rounded to 32-bit precision. Values outside the single-precision range collapse entirely: 1e308 becomes inf and 5e-324 becomes 0.0. `new_decoder` returns the Cython decoder whenever the extension is built, so this is the default read path. It affects any double read from a manifest, most visibly identity partition values on a float/double column: writing a partition value of 429496729622.314 and reading the manifest back returns 429496729600.0. The pure-Python `StreamingBinaryDecoder` was always correct, and `read_float` is unaffected because a value decoded from four bytes is already representable as a C float. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T2GWEoizz8ZGQbjatT8aZy
| 5e-324, # underflows to 0.0 in single precision | ||
| ], | ||
| ) | ||
| def test_read_double_keeps_full_precision(decoder_class: Callable[[bytes], BinaryDecoder], value: float) -> None: |
There was a problem hiding this comment.
I don't think this is a valid regression test. It passes even if we revert decoder_fast.pyx's change.
Sorry, something went wrong.
There was a problem hiding this 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:
$ 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 deselectedAll 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.
Sorry, something went wrong.
Reviewers
1 more reviewerAt least 1 approving review is required to merge this pull request.
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.
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:
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.