zhf999 commented on PR #48468: URL: https://github.com/apache/arrow/pull/48468#issuecomment-5128508504
Hi, we work on Paimon C++, a storage library that writes Parquet through the Arrow C++ writer. We have been carrying a local patch for byte-based row group limiting in production, so we'd like to share our perspective as another downstream user.We support landing this feature with in-writer estimation as the default behavior, for three reasons: 1. The alternative doesn't avoid estimation — it just relocates it. #49527 exposing estimated_buffered_stats() is genuinely useful, but any caller implementing their own row-group policy on top of it ends up doing essentially the same arithmetic this PR does: compressed-so-far + estimated-buffered against a byte budget. The estimate is no more accurate in application code than inside the writer. If anything, the writer is in a better position: it can split an incoming batch at a row boundary mid-write, which a caller of arrow::FileWriter::WriteRecordBatch() cannot do without reimplementing the whole write loop. 3. Every downstream is already reimplementing this. Velox (per @mbasmanova above) and the internal code @wecharyu mentioned have all independently built the same wrapper: track size, cut a row group / roll a file when a threshold is crossed. This is exactly the duplication a library default would eliminate — and both parquet-java (parquet.block.size) and arrow-rs (apache/arrow-rs#9357) have already accepted per-batch estimation as good enough for this purpose. 4. The use case is inherently tolerant of imprecision. **Users who set a byte limit are sizing row groups for I/O scheduling and storage-block alignment, not asking for an exact contract** — max_row_group_length remains the exact knob for those who need determinism. As a data point: our production patch uses an even cruder estimate than this PR (compressed bytes only, checked reactively between batches, so a row group can overshoot by up to one batch), and even that has been acceptable in practice for our workloads. The per-row estimation in this PR should only behave better. Documenting max_row_group_bytes as best-effort seems sufficient. One suggestion on the remaining concern: keeping the default as unlimited and making the byte limit opt-in (as arrow-rs did for backward compatibility) would address the behavior-change objection without weakening the feature. Thanks @wecharyu for pushing this forward. -- 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]
