andygrove commented on PR #25853: URL: https://github.com/apache/datafusion/pull/25853#issuecomment-5981949254
@EmilyMatt I agree the aggregate's estimates are worth improving, since they decide when it spills and whether it keeps room to sort and write the spill. But they don't fix this issue, which happens after the spill, when the spilled state is read back. A spill file is read back one batch at a time, and spilled batches are bounded only by `batch_size` rows. With fewer groups than `batch_size`, all the groups of a spill land in one batch, however early the aggregate spills, and the replay's ordered aggregate, which cannot spill, has to hold that whole batch. In the reproducer in #25851, each final partition receives one input batch that already holds all of its state, so the final aggregate spills on that first batch and writes its 8 groups as one 184 MB batch. Reading it back needs 225 MB in a 128 MB pool, although any one group fits. No reservation changes the size of that batch. With several spill files, the merge has the same problem: it combines rows until it has `batch_size` of them, so it rebuilds batches of large groups even from small spilled batches. On the other points: - Performance: the TPC-H and TPC-DS runs above show no slowdowns, `external_aggr` is not slower, and merges without a byte limit only check that none is set. - Maintenance: the byte limit is crate-private and opt-in, and only aggregate spilling sets it. - Accuracy: spilling cuts batches by the size of each row (`SpilledRowSizes`), not by averages. Only the merge estimates rows by the average row size of their batch. It never takes more rows from a batch than the batch holds, so it can underestimate an output batch by at most the input batches it holds, which are about 1 MiB each unless they hold a single larger row. -- 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]
