andygrove commented on PR #5370: URL: https://github.com/apache/datafusion-comet/pull/5370#issuecomment-5496959557
Sorry, I missed the merge window on this one. Reviewing after the fact, the disk-side fix and the completion-listener move both look right to me, and I confirmed the listener ordering holds because `releasePlan` publishes final metrics and Spark runs completion listeners in reverse registration order. One thing I think is worth revisiting though. The shared-allocation dedup only covers slices produced inside a single `insert_batch` call, so it misses the case the `count_new_buffers` doc comment calls out, where a partial `HashAggregate` hands the writer many chunks that all share one group-values allocation. Those arrive as separate `insert_batch` calls, and I measured the same 16 chunks of one 128 KiB allocation reporting 2,228,224 bytes when delivered separately versus 262,144 when sliced from one batch. I know the PR treats cumulative-per-input-batch as intended, but that makes the number depend on how the producer chunked its output rather than on how much memory was actually resident. I filed https://github.com/apache/datafusion-comet/issues/5617 with the details. Would you mind taking a look? -- 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]
