Weijun-H commented on PR #25910: URL: https://github.com/apache/datafusion/pull/25910#issuecomment-6115306696
> They overlap quite a bit. We should probably discuss them together. Each PR has two parts: what gets written to the spill file, and what the merge builds from it. > > * **[Limit spill merge output batches by byte target #25565](https://github.com/apache/datafusion/pull/25565)** fixes only the merge output. It reserves the output memory in the pool before building each batch. > > * **[fix: size aggregate spill batches by bytes as well as rows #25853](https://github.com/apache/datafusion/pull/25853)** bounds aggregate spill batches through the sort iterator, and also caps merge output batches. > > * **This PR** bounds spill batches in `SpillManager`, so any operator can opt in. Only the sort does in this PR. It also caps merge output batches, but by an estimate outside the pool. > > > The merge-output parts of all three do the same thing, and #25565 does it best. I propose we remove the merge cap from this PR so it covers only the write side, with #25565 as the merge-output fix. We need both: this PR alone removes the error but leaves the merge output outside the pool, and #25565 alone cannot start the merge because the spill batches are too large. They touch different code, so they can be reviewed in parallel and merged in either order. > > Then we can change #25853 so the aggregate uses the `SpillManager` bound and does not have its own mechanism. That one has to come after this PR, because it needs the new API. > > Wdyt? The split makes sense, but #25565 already passes the Utf8 repro and doesn't cover List/Utf8View output accounting. Before removing the other merge caps, we need that coverage; moving aggregate batching into SpillManager also needs to preserve the bound before take builds the batch and track the actual written batch sizes. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
