adriangb opened a new pull request, #11682: URL: https://github.com/apache/arrow-rs/pull/11682
# Which issue does this PR close? - Closes https://github.com/apache/arrow-rs/issues/11659 # Rationale for this change Since 59.0.0, `ArrowWriter` writes the dictionary page's `encoding_stats` entry after the data page entries. The file layout did not change: the dictionary page is still written first. Arrow C++ ([`IsColumnChunkFullyDictionaryEncoded`](https://github.com/apache/arrow/blob/9f0341314ca96eb58081d4132c77b460d880b2b0/cpp/src/parquet/file_reader.cc#L63-L88)) requires `encoding_stats[0]` to be the dictionary page. With the 59.x order, Arrow C++ does not see any parquet-rs column chunk as fully dictionary encoded, so it cannot use its dictionary fast path. | | `encoding_stats` (file order) | |---|---| | before this PR | `DATA_PAGE RLE_DICTIONARY`, `DICTIONARY_PAGE PLAIN` | | after this PR (and 58.x) | `DICTIONARY_PAGE PLAIN`, `DATA_PAGE RLE_DICTIONARY` | The new order was not a design goal of https://github.com/apache/arrow-rs/pull/10020. It was a side effect: the deferred-ordering page writer gets the dictionary page last, and the stats were recorded in the order the pages were produced. Review accepted this because the spec defines the list as a set (https://github.com/apache/arrow-rs/pull/10051 was closed for that reason). But a known reader depends on the order, and restoring it changes only metadata, so the change has no memory or throughput cost. # What changes are included in this PR? `GenericColumnWriter::write_dictionary_page` now inserts the dictionary entry at the front of `encoding_stats` instead of appending it. The dictionary page is always laid out first, on both the deferred (Arrow) path and the `SerializedFileWriter` path, so the stats now follow file order. On the non-deferred path the list is still empty when the dictionary is written, so its output does not change. When the writer falls back from dictionary to plain encoding, the result is `DICTIONARY, RLE_DICTIONARY data, PLAIN data`, which also matches the file layout. # Are these changes tested? Yes. The new `dictionary_page_encoding_stats_lists_dictionary_first` test (adapted from https://github.com/apache/arrow-rs/pull/10051) checks that `stats[0]` is the dictionary page. It fails on `main` and passes with this change. The existing column writer and arrow writer tests pass. # Are there any user-facing changes? Only the order of `page_encoding_stats` in files written by `ArrowWriter`. It goes back to the order used before 59.0.0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
