You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Rodrigo-Palma (via GitHub)" <gi...@apache.org> on 2026/09/23 02:25:22 UTC
[PR] fix(avro): advance the decoder when skipping an enum [iceberg-python]
Rodrigo-Palma opened a new pull request, #4009:
URL: https://github.com/apache/iceberg-python/pull/4009
Closes #4006
# Rationale for this change
`EnumReader.skip()` was `pass`, so skipping an enum field left its bytes in the stream and every field after it in the record was decoded from the wrong offset. `read()` already delegates to the wrapped reader; `skip()` now does the same.
This is reachable from ordinary reads: the manifest entry schema starts with the `status` enum (field id 0), so any read that projects `status` out decodes the rest of the entry from the enum's bytes.
```python
decoder = CythonBinaryDecoder(b"\x02\x18") # enum ordinal 1, then 12
reader = EnumReader(ManifestEntryStatus, IntegerReader())
reader.skip(decoder)
IntegerReader().read(decoder)
# before: 1 (the enum's own bytes, re-read)
# after: 12
```
# Are these changes tested?
Yes, two tests in `tests/avro/test_resolver.py`, each parametrized over `StreamingBinaryDecoder` and `CythonBinaryDecoder` (4 cases, all failing before this change):
- `test_enum_reader_skip_advances_the_decoder`: after skipping, the next field reads its own value.
- `test_enum_reader_skip_matches_read`: reading and skipping leave the decoder at the same position.
`pytest tests/ -m unit --ignore=tests/integration` gives 4215 passed. The 9 failures in `tests/avro/test_decoder.py` are present identically on an unmodified `main` in this checkout (a stale compiled Cython extension) and are unrelated to this change.
# Are there any user-facing changes?
Yes: reads that skip an enum field now return correct values instead of misaligned ones. No API 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): advance the decoder when skipping an enum [iceberg-python]
Posted by "Rodrigo-Palma (via GitHub)" <gi...@apache.org>.
Rodrigo-Palma commented on code in PR #4009:
URL: https://github.com/apache/iceberg-python/pull/4009#discussion_r4085287773
##########
tests/avro/test_resolver.py:
##########
@@ -418,3 +421,30 @@ def test_writer_missing_optional_in_read_schema() -> None:
expected = StructWriter(field_writers=((None, OptionWriter(option=StringWriter())),))
assert actual == expected
+
+
+@pytest.mark.parametrize("decoder_class", [StreamingBinaryDecoder, CythonBinaryDecoder])
Review Comment:
Moved the list to `tests/avro/conftest.py`, with a `decoder_class` fixture that parametrizes it, so these tests no longer carry their own copy.
I went with a fixture instead of importing the constant. CI runs `pytest tests/avro/test_decoder.py` on its own (python-ci.yml, and the cibuildwheel test command in the artifact workflows), and in that invocation the repository root is not on `sys.path`, so `from tests.avro.conftest import AVAILABLE_DECODERS` fails with `ModuleNotFoundError: No module named 'tests'`. I hit it locally before settling on the fixture. The existing `from tests.conftest import ...` imports have the same limitation and only work under `python -m pytest`. A fixture needs no import, so it works under either invocation.
`test_decoder.py` and `test_reader.py` still define their own copies. Direct parametrization takes precedence over a fixture of the same name, so they keep working untouched. Happy to migrate them in a follow-up if you would like: it is 22 decorators plus one `itertools.product` call, which felt like a separate change from this bug fix.
--
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): advance the decoder when skipping an enum [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #4009:
URL: https://github.com/apache/iceberg-python/pull/4009
--
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): advance the decoder when skipping an enum [iceberg-python]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on code in PR #4009:
URL: https://github.com/apache/iceberg-python/pull/4009#discussion_r4084724549
##########
tests/avro/test_resolver.py:
##########
@@ -418,3 +421,30 @@ def test_writer_missing_optional_in_read_schema() -> None:
expected = StructWriter(field_writers=((None, OptionWriter(option=StringWriter())),))
assert actual == expected
+
+
+@pytest.mark.parametrize("decoder_class", [StreamingBinaryDecoder, CythonBinaryDecoder])
Review Comment:
There's a list of AVAILABLE_DECODERS in `tests/avro/test_reader.py` that we should try to reuse
That'll involve moving that list to some shared file, but that way we can have future tests all share the same list of decoders.
--
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