mbasmanova commented on PR #48468: URL: https://github.com/apache/arrow/pull/48468#issuecomment-4961101944
@yonipeleg33 Thanks — let me restate our position more clearly, since it came across narrower than intended. Our first preference is exactly what this PR does: Arrow supporting a byte-size row-group limit directly. arrow-rs already does that with per-batch estimation (#9357), and that model is fine by us — so we're in favor of #48468 landing, not asking for a separate feature. Our earlier note leaned on "keep the contract exact," which made the adjustable row-count target look like the main ask. That was meant as a *fallback* — a way to still get byte-sized row groups if estimating inside the writer is considered a no-go. It's contingent on the direct byte limit not being feasible — which we'd assumed was the case, given the PR hasn't moved in months — not a competing feature, so I don't think it's a separate-FR thing. On "the wrapper is fine": it works, but it means each engine reimplements the row-group splitting that really belongs in the writer — arrow-rs having the byte limit avoids exactly that duplication. That's the upstream value here. So the open question is really for the maintainers: is the per-batch estimate acceptable in the C++ writer, as it was in Rust? That looks like the crux. @wgtmac @pitrou — could one of you weigh in? If it's a yes, this PR is what we want. If it's a no, would the adjustable exact-row target be acceptable instead — since it keeps the estimation in the caller and leaves the writer cutting only at exact rows? -- 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]
