cryptus-neoxys opened a new pull request, #12189:
URL: https://github.com/apache/arrow-rs/pull/12189
# Which issue does this PR close?
- Closes #12129
> Note for reviewers (duplicate-work check): the issue has no assignee. A
search for PRs
> referencing #12129 shows a single prior attempt, #12139, which a
maintainer closed on
> Nov 9, 2026 because its AI usage was not disclosed. This branch is an
independent
> implementation and discloses AI usage below, as CONTRIBUTING.md requires.
If the
> maintainer who closed #12139 prefers to wait until its author reopens it,
this PR can
> be closed and resubmitted later; the branch is here either way.
# Rationale for this change
A primitive column (for example `INT32`) whose data page claims
`RLE_DICTIONARY`
encoding while the column chunk has no dictionary page panics the reader:
```
thread 'main' panicked at parquet/src/column/reader/decoder.rs:361:18:
Decoder for dict should have been set
```
The issue's reproducer writes a dictionary-encoded `Int32` column and
mutates the
dictionary page header's type from `DICTIONARY_PAGE` (2) to `INDEX_PAGE`
(1). The page
reader skips the page as an index page, the following data page still claims
dictionary
encoding, and `ColumnValueDecoderImpl::set_data` expects a dictionary
decoder that was
never installed. Because a truncated or otherwise malformed file can produce
this state,
any application reading untrusted Parquet files can be crashed.
The byte-array readers already handle the equivalent case
(`parquet/src/arrow/array_reader/byte_array.rs`, `missing dictionary page
for column`),
and empirically they return that error on every read attempt. This PR
mirrors that
handling for the generic primitive decoder.
One detail beyond the one-line swap: with only `expect` replaced by
`ok_or_else`, the
arrow-level reproducer returns the expected error on the first poll and then
panics on
the second poll at `read()` (`current_encoding should be set`), because
`read_new_page`
claims the page's values before `set_data` runs and `current_encoding` is
only written
on success. The guards described below close that second panic so the
primitive decoder
behaves like the byte-array readers, which error on every attempt instead of
panicking
or ending iteration.
# What changes are included in this PR?
All in `parquet/src/column/reader/decoder.rs`, the primitive value decoder:
- `set_data` returns `missing dictionary page for column` (the byte-array
readers'
message) instead of `expect`ing the dictionary decoder slot to be
populated.
- `set_data` clears `current_encoding` before configuring the new page, so a
failed
page cannot be retried with the previous page's decoder state.
- `read` and `skip_values` return `the current data page failed to decode`
when no data
page has been decoded, so a retry after a failed page errors instead of
panicking.
Tests:
- `parquet/src/file/serialized_reader.rs`:
`dict_encoded_data_page_without_dictionary_page_errors` exercises the
column reader
with the issue's mutated header and asserts the first attempt and a retry
both error.
- `parquet/tests/arrow_reader/bad_data.rs`:
`test_arrow_rs_gh_12129_missing_dictionary_page`
generates the file in memory, mutates the header, and asserts through
`ArrowReaderBuilder` that the first two polls return errors rather than
panicking.
Both regression tests panic at `decoder.rs:361` when the fix is reverted
(verified by
reapplying the pre-fix `expect` locally and running them).
# Are these changes tested?
Yes, by the two tests above plus the existing suites. Commands and results
(rustc/cargo
1.99.1, macOS aarch64):
- `cargo test -p parquet`: all suites green, 0 failures: lib 1508 passed (1
ignored),
`tests/arrow_reader` 147 passed (1 ignored), `tests/arrow_writer` 114
passed,
`tests/encryption` 2 passed, `tests/ieee754_nan_interop` 2 passed,
`tests/variant_integration` 80 passed (7 ignored), `tests/geospatial` 0
(feature
gated).
- `cargo test -p parquet --test arrow_reader bad_data::`: 11 passed,
including the new
test.
- `cargo test -p parquet --lib file::serialized_reader::tests`: 48 passed,
including
the new test.
- `cargo clippy -p parquet --all-targets --all-features -- -D warnings`:
clean.
- `cargo fmt --all -- --check`: clean.
- `typos --config typos.toml` on the three changed files: clean.
Not run here: the full workspace suite (`cargo test --workspace`) and `cargo
test
-p arrow`; this PR only touches the parquet crate, and the affected crate
was tested in
full. Miri was not run (no unsafe changes).
# Are there any significant user-facing changes?
Behavior on malformed input changes: a dictionary-encoded data page without a
dictionary page now returns `Err(ParquetError::General("missing dictionary
page for
column"))` instead of panicking. This matches the byte-array readers and is
not a
breaking API change.
# AI usage disclosure
This PR was implemented with AI assistance: an AI coding agent (OpenCode
running the
GLM model from Z.ai) produced the patch and tests under direction from the
submitter.
The submitter reviewed the diff in full, confirmed the reproducer from the
issue panics
before the fix and returns errors after it, and takes responsibility for the
changes.
This disclosure is included because a prior PR for the same issue (#12139)
was closed
for an undisclosed AI contribution.
--
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]
