adriangb commented on PR #25910: URL: https://github.com/apache/datafusion/pull/25910#issuecomment-5923211281
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. - **#25565** fixes only the merge output. It reserves the output memory in the pool before building each batch. - **#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? -- 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]
