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
`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.
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.
2 participants
Rationale for this change
CythonBinaryDecoder.read_doubleis declaredcpdef float read_double(self). In Cython,floatis the C single-precision type, so the correctly decoded 64-bit value returned bySTRUCT_DOUBLE.unpackis narrowed to 32 bits on the way out. Every Avrodoubleread through the fast decoder is silently rounded, and values outside the single-precision range collapse entirely.new_decoderreturnsCythonBinaryDecoderwhenever the extension is built, so this is the default read path; the pure-PythonStreamingBinaryDecoderfallback was always correct.The user-visible effect is on any
doubleread out of a manifest, most directly an identity partition value on afloat/doublecolumn. Writing a manifest with a partition value of429496729622.314and reading it back throughManifestFile.fetch_manifest_entryreturns429496729600.0onmain, and the exact value with this change.read_floatis left ascpdef 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_doublebecause19.25is exactly representable in single precision.tests/avro/test_file.py::test_all_primitive_typesdoes round-trip adoublethat is not, but its assertion loop iteratesenumerate(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.pygainstest_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:6 failed, 8 passed— every failure is theCythonBinaryDecoderparametrization, e.g.assert 0.0 == 5e-32456 passedintests/avro/test_decoder.pyFull local run on macOS/arm64, Python 3.13:
make lintall 12 hooks pass,make testgives3957 passed, 3 skipped, 1570 deselected.Integration tests (Spark/Docker) were not run locally.
Are there any user-facing changes?
Yes —
doublevalues 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.