adriangb opened a new pull request, #10505: URL: https://github.com/apache/arrow-rs/pull/10505
- closes https://github.com/apache/arrow-rs/issues/10489 ## The cause is the page flush, not the mini-batch splitting The issue attributes the regression to #9972's byte-budget sub-batching. It's actually the page flush; the sub-batching only exposes it. `should_add_data_page` fires when `estimated_data_page_size() >= data_page_size_limit`. For `DELTA_BYTE_ARRAY` that estimate is the real encoded size, and the first value on a page is stored in full — so a single 8 MiB value against a 1 MiB limit puts the page over the limit by itself and triggers a flush. Flushing clears the encoder's `previous`, so the next value gets `prefix_length = 0`, and so on. The encoding degenerates to exactly `PLAIN`. Before #9972 a 1024-row mini-batch meant the check simply didn't run until 1024 values had been written, so the dedup survived. That was an accident of `write_batch_size`, not a designed property — the same column at 2000 rows already lost the prefix at the 1024-value boundary. This matters for choosing a fix: **making the byte budget encoding-aware (suggestion 2 in the issue) does not fix the reported bug.** However precise the budget, the first value alone still exceeds the limit and still triggers the flush. ## The fix Parquet requires at least one value per data page, so a value larger than the limit cannot be split out — the limit is unsatisfiable for it. Counting those bytes against the limit is what forces the pathological one-value-per-page cut. Record that first value's encoded size in `PageMetrics::page_size_floor` and apply the limit to what follows it, i.e. the bytes that *can* still go on another page. The exemption is gated on a new `ColumnValueEncoder::compresses_against_previous_value` (default `false`, `true` only for `DELTA_BYTE_ARRAY` in both the generic and arrow encoders). `PLAIN` and `DELTA_LENGTH_BYTE_ARRAY` cost the same wherever a value lands, so there is nothing to preserve by keeping values together and they keep their existing tighter one-value page bound. All three regression tests from #9972 pass unmodified. ## Why not "don't split when the split cannot help" Suggestion 1 in the issue restores the dedup, but by removing the bound that #9972 added, for exactly the workload it was added for. Measured with `Some(1) => chunk_size` in `byte_budget_chunker.rs`, 2048 rows × 128 KiB distinct values against a 64 KiB page limit — the same regime as a 10 MB value against the 1 MiB default, scaled to fit in RAM: | | pages | max page | |---|---|---| | 59.1.0 | 2048 | 128 KiB | | suggestion 1 | 2 | **128 MiB** | | this PR | 1024 | 256 KiB | Page size under suggestion 1 is `write_batch_size × value_size`. At 1024 × 10 MB that is a 10 GB page, which is the failure #9972 fixed. ## Results The reporter's table, reproduced verbatim (10 identical 8 MiB values, raw input 80 MiB): | `data_page_size_limit` | 58.4.0 | 59.1.0 | this PR | | --- | --- | --- | --- | | default (1 MiB) | 8.00 MiB | 80.00 MiB | **8.00 MiB** | | 4 MiB | 8.00 MiB | 80.00 MiB | **8.00 MiB** | | 8 MiB | 8.00 MiB | 80.00 MiB | **8.00 MiB** | | 16 MiB | 8.00 MiB | 8.00 MiB | **8.00 MiB** | Values that merely share a long prefix behave the same. Values sharing *no* prefix are unaffected in file size and stay bounded at up to two values per page — the exempt one plus the one that trips the budget. ## Tests - `test_column_writer_delta_byte_array_dedups_large_shared_prefix_values` — 16 identical 64 KiB values, 16 KiB page limit. Fails on `main` with 1 MiB across 16 pages (byte for byte what `PLAIN` produces); passes here at ~one value's worth. - `test_column_writer_delta_byte_array_bounds_pages_without_shared_prefix` — the same column with values differing from byte 0, asserting pages stay bounded by two values. Guards against fixing this by dropping the bound. - `test_large_string_delta_byte_array_shared_prefix` — the `ArrowWriter` path from the report, as an exact page-layout assertion. ## Notes A follow-up worth considering separately: the byte budget in `count_within_budget_*` still measures raw payload length, so `DELTA_BYTE_ARRAY` columns sub-batch more eagerly than the encoded size warrants. That is a throughput question rather than a correctness one, and it is not what caused this regression. 🤖 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]
