andygrove commented on PR #6279: URL: https://github.com/apache/datafusion-comet/pull/6279#issuecomment-6235269446
Thanks, these were all useful. Pushed in 766e0dea10, which also merges main to clear the conflicts. The consumer now registers when the first batch is buffered, so it no longer takes a share from the sort while the sort is still reading its input. A new test checks that it registers then and not for empty input. The error hint and the tuning doc now say that a window partition needs up to twice its size within the operator's share, and both suggest `greedy_unified` before `spark.comet.exec.window.enabled=false`, with a note that the latter also moves the bounded window functions back to Spark. I left the pool itself alone (#5465). `evaluate` now counts the copy with the counter that already holds the buffered batches, so the second counter and the `batches.len() > 1` branch are gone. `push_batch` and the final evaluation are timed in `poll_next_inner` and the inner timer is removed, so batches that only extend the open partition record time too. The last row's keys are kept from the previous batch, as a copy so they don't pin the batch's buffers, instead of being re-evaluated from the last buffered batch every time. For the tests, the first Scala test is now `windows/window_whole_partition_batches.sql` without the `OVER ()` query. The failure test asserts the `CometWindowAggExec cannot spill` hint, and it passes, so the hint does reach Spark. In the Rust tests I dropped the interleaved empty-batch block and the `peak` assertions. This is now `Part of #6253`, and the description says why `BoundedWindowAggExec` is still open. The follow-up for `BoundedWindowAggExec` is #7092. The window is added to the pool list in `tuning/memory.md`, to the imported-batches paragraph in `memory_management.md` (an `OVER ()` window has no sort below it), and to the cannot-spill item in its checklist. Not yet on offering the stream upstream. I'll leave the removal condition as it is until there is upstream work to point at. `run-spark-4.1-tests` is on the PR. I haven't run the TPC-DS comparison under `fair_unified` yet. -- 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]
