EmilyMatt commented on PR #25853: URL: https://github.com/apache/datafusion/pull/25853#issuecomment-6248566015
What I see: 1. During the actual emit, for a brief instance, we are holding both the output arrays, and the state, meaning peak is still double, we need to have an actual estimation for the emitted size, I have a POC for this, can open a PR if you want, then you'll be able to use it to calculate the expected peak. 2. remaining_groups.get_array_memory_size() only calculates the *data*, not the capacity, and since you add another spill trigger, we could be doing more of these emits where the the Vec's capacities are larger than data and so we emit more than we actually reserved and are releaseing, unfortunately I don't see any solution for this other than calling shrink_to_fit() *for each array* after emitting it(so we peak at each array's 2x size, rather than the whole batch's), adds an extra copy, but greatly reduces memory footprint. 3. try_reserve_hash_table_growth still lets a batch through on failed reservation, instead we should slice it and try again, since our estimation depends on the the input array sizes, but also on the sliced data sizes. so it's possible that we can safely process the batch by simply doing the extra spill :woman_shrugging: -- 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]
