yew1eb commented on PR #3873: URL: https://github.com/apache/celeborn/pull/3873#issuecomment-6183606116
Thanks for the PR — clear motivation and I like that the test asserts the actual pushed batch row counts. No blocking issues; a few minor points: 1. The threshold caps *estimated* row bytes, not actual heap usage: builders still pre-size for `batch.size` rows, so for narrow-row/many-column schemas the pre-allocation dominates. Also, with dictionary encoding the estimate overcounts, so batches push earlier than the threshold implies (conservative, but worth knowing). Is there a follow-up for sizing the builders from the threshold? 2. With a separate `enabled` flag, setting only the threshold silently does nothing. Consider a WARN when `threshold` is explicitly set but `enabled` is false. 3. Since `fastColumnarWrite0` is the hottest loop, consider guarding the accumulation with the enabled flag so the disabled path stays identical to before. Tiny either way — nit. 4. The PartitionIdPassthrough reflection + handle setup in the new test duplicates `checkWithIntegrity` in `CelebornShuffleWriterSuiteBase` across the three modules. Extracting a protected helper in the base class would help future tests. 5. In the docs, one sentence on the trade-off (more frequent pushes, slightly worse compression) would help — the numbers are only in the PR description. -- 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]
