adriangb commented on PR #10505: URL: https://github.com/apache/arrow-rs/pull/10505#issuecomment-5196588583
@alamb I've addressed the comment suggestions you had. Regarding the limitation with nulls: I think it's solvable, but I propose we do it as a followup (https://github.com/apache/arrow-rs/pull/10554) to keep the review units contained. Responding to your other questions > it seems like it may be just better behavior (aka the writer is now enforcing page limits more strictly I think the problem with this framing is that we are trading off best efforts here. Best effort to keep file sizes low, best effort to keep page sizes in check and best effort to be fast. None of the regimes are clearly bugs, they work without errors (aside from OOMs maybe). But I would argue that an 80MB file instead of an 8MB file is buggy behavior / a regression (which is what @jonasdedden is arguing as well I think). > I am worried about the complexity potentially introduced by this PR: not just the code, but I am not sure if it will really help @jonasdedden 's problem (maybe now the pages will have 2 rows rather than 1 ?) 🤔 I think it does address the root issue. I ran the reporter's exact repro on current main and on this PR:  | file | pages | max page | vs 1 MiB limit -- | -- | -- | -- | -- main | 80 MiB | 10 | 8 MiB | 8.0× #10505 | 8 MiB | 1 | 8 MiB | 8.0× We can't do better than the 8.0x page limit: a single value is already 8MiB so we are forced to exceed the limit. Parquet requires at least one value per page, so an 8 MiB value against a 1 MiB limit overshoots it by 8× no matter what. What changes is only that `main` emits ten of those pages instead of one, and 80 MiB instead of 8 MiB. So I don't think there's page-limit adherence being traded away; the one-value-per-page cut isn't buying smaller pages, because smaller pages aren't reachable. > maybe now the pages will have 2 rows rather than 1? For the reported case it's 10 rows on 1 page. Two-per-page is the _worst_ case, and only for values that share no prefix. In that case deduplication has nothing to save and the file size is identical either way, so the bound is acting as a safeguard rather than as the result. That's what `test_column_writer_delta_byte_array_bounds_pages_without_shared_prefix` promises. Regarding "just raise the page size limit": it works, but only if you know your largest value up front, and it's all-or-nothing for the column. In the table above, raising the limit to 4 MiB still gives 80 MiB, because the values are 8 MiB; you have to clear the largest value you'll ever write. On complexity: the mechanism is 30 lines. One trait method w/ a default, one `usize` on `PageMetrics`, and one `saturating_sub` in `should_add_data_page`. The remaining ~250 lines are tests and doc comments. The "only check page sizes at batch boundaries" option is genuinely simpler but it would regress the work that #9972 was added to prevent, i.e. reverts to huge pages for the case being reported here. -- 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]
