adriangb opened a new pull request, #10777: URL: https://github.com/apache/arrow-rs/pull/10777
# Which issue does this PR close? - Closes #9739. # Rationale for this change When the dictionary encoder overflows `dictionary_page_size_limit` part-way through a column chunk, the writer used to flush the values buffered for the in-progress page as one more dictionary-encoded data page and then always write the dictionary page — even when no other data page referenced it. This has two problems, both described in #9739: 1. The dictionary page can vastly exceed the configured `dictionary_page_size_limit`. The limit is only checked once per mini-batch, and when values are large and distinct the dictionary blows through it before the first data page ever completes: `tests/arrow_writer/layout.rs::test_dictionary_spill_large_values` previously pinned a 65540-byte dictionary page against a 1024-byte configured limit — 64x over — which the writer emitted solely so that a single 2-byte data page could reference it. Readers must fully decompress and materialize that page. 2. The prefix of the chunk keeps an encoding the writer has just decided is not good enough for the chunk, and the dictionary duplicates every value the prefix references. parquet-java's `FallbackValuesWriter` handles fallback by re-encoding: `DictionaryValuesWriter#fallBackAllValuesTo` walks the buffered ids, resolves them through the dictionary, and writes the values into the fallback writer; the dictionary page is only produced if previously flushed pages still need it. This PR brings the Rust writer to the same behavior. # What changes are included in this PR? `GenericColumnWriter::dict_fallback` no longer force-flushes the buffered values as a final dictionary-encoded page. Instead: - A new `ColumnValueEncoder::fall_back_from_dictionary(retain_dictionary)` (crate-private trait) re-encodes the values buffered for the in-progress page — held as dictionary ids — through the fallback encoder, resolving them against the in-memory dictionary. No original input values are needed. Implemented for both the generic typed encoder and the arrow byte-array encoder. - The dictionary page is written only when dictionary-encoded data pages were already flushed for the chunk (tracked by the writer). Those pages are left as-is, exactly as parquet-java leaves them; on the `ArrowWriter` path they have already been serialized into the page store, so this is also the only behavior compatible with the streaming design. - When no data page was flushed yet — the common case, since RLE ids grow far more slowly than the dictionary itself, and the only case where the dictionary page can grossly overshoot its limit — the dictionary is discarded: the chunk contains no dictionary page, `dictionary_page_offset` is unset, and `RLE_DICTIONARY` does not appear in the chunk encodings. - After re-encoding, the writer immediately checks the data page size limit, since values that fit comfortably as dictionary ids may exceed it once re-encoded. Behavioral changes visible in existing tests (all layouts strictly improve): - `layout.rs::test_primitive` (dict limit 1000): RLE_DICTIONARY page (250 rows) + PLAIN page (1750 rows) + 1000 B dictionary page → a single 2000-row PLAIN page, no dictionary page. - `layout.rs::test_string` (dict limit 1000): 1008 B dictionary page (already over its limit) + 126-row RLE_DICTIONARY page + 2 PLAIN pages → 2 PLAIN pages, and the chunk shrinks (16000 B vs 16114 B of page payload). - `layout.rs::test_dictionary_spill_large_values` (dict limit 1024): 65540 B dictionary page + 2 B RLE_DICTIONARY page + 31 PLAIN pages → 32 PLAIN pages, no dictionary page. - `column::writer::tests::test_column_writer_caps_dictionary_page_size` now sets a small `data_page_row_count_limit` so that dictionary-encoded pages exist before the spill and a dictionary page is still produced for it to measure. New tests cover both encoder paths: fallback before the first data page (no dictionary page, fallback encoding only, chunk size bounds — including a sorted-int column whose re-encoded DELTA_BINARY_PACKED chunk is less than half the plain size), fallback after data pages were flushed (dictionary page still written, mixed encodings), and roundtrip equality across the fallback boundary in every scenario. This also composes with the fallback-policy discussion in #9699 and the `DictionaryFallback` policy proposed in #10775: any policy that decides to fall back early now produces a clean, uniformly encoded chunk instead of writing out the rejected prefix. # Are these changes tested? Yes — 7 new tests as described above, 4 existing tests updated to the new layouts, and the full `cargo test -p parquet --all-features` suite passes (1776 tests). # Are there any user-facing changes? No API changes (`ColumnValueEncoder` is crate-private). File output changes for chunks that hit dictionary fallback: the dictionary page is omitted when nothing references it, and the values buffered at the fallback point are written in the fallback encoding. Any downstream test asserting exact page layouts of such chunks will need updating, as this crate's own layout tests did. 🤖 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]
