siddubakka opened a new pull request, #12217: URL: https://github.com/apache/arrow-rs/pull/12217
# Which issue does this PR close? - Closes #10931 # Rationale for this change `set_write_batch_size` documents `write_batch_size` as the granularity at which limits such as `set_data_page_row_count_limit` are checked. Columns with compact level data did not follow that rule. Their mini-batch size was `data_page_row_count_limit`, so `set_write_batch_size` had no effect on a nullable column whose batch happened to contain no nulls. The condition was on the level data rather than the schema, so the same column with the same writer properties produced different page boundaries depending on whether a batch contained a null. Page boundaries should follow the properties that were set, not the contents of the batch. # What changes are included in this PR? `write_batch_internal` now always batches at `write_batch_size`. The special case for compact level data added in #9831 and the two condition variables behind it are removed, and a short comment explains why the batch size is what it is. The plan comment on the issue proposed taking the smaller of `write_batch_size` and `data_page_row_count_limit`. I built that version first. It still leaves the compact path different from the other two when `write_batch_size` is larger than the row count limit, which is the `write_batch_size = 100000` case in the issue's reproduction. Batching at `write_batch_size` directly matches the documentation and makes all three level shapes produce identical page boundaries at every `write_batch_size` in the reproduction. # Are these changes tested? Added `test_write_batch_size_bounds_compact_level_batches` in `parquet/tests/arrow_writer/layout.rs`. It repeats the issue's reproduction: 12000 Int64 rows, row count limit 3000, dictionary disabled, for absent, uniform and materialized level data at `write_batch_size` 8, 64, 1024 and 100000. It asserts the page row counts from the issue's table and that the three shapes agree. I also ran `cargo bench -p parquet --bench arrow_writer` before and after the change for the groups that take this path. This machine cannot compare runs that are separated by a rebuild. Repeating the same commit in a later session moved those benches by 20 to 40 percent, while two runs of the same binary back to back stayed within about 6 percent, so I am not quoting a number for this change because it would not be honest. What does change is the batch size itself. At default settings the compact path used to write 20000 rows per mini-batch, which is the data page row count limit, and it now writes `write_batch_size` rows, which is 1024 by default. The larger batch was an optimization from #9831. This PR gives it back in exchange for the behavior the documentation describes. I can run a cleaner comparison if reviewers want numbers. # Are there any significant user-facing changes? Page layout changes for columns with compact level data. With the same writer properties, pages now end at `write_batch_size` boundaries like every other column instead of at `data_page_row_count_limit` boundaries, so files written before and after this change can have different page boundaries for such columns. There are no API changes. # AI usage disclosure I used an AI assistant to draft the code change, the test and this description. The parquet test suite and clippy were run locally before opening this PR. -- 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]
