andygrove commented on PR #6279:
URL:
https://github.com/apache/datafusion-comet/pull/6279#issuecomment-6264145733
Thanks again for the reviews. I've pushed `35eac17a70`, which merges `main`
and is ready for another look.
Your earlier approvals were on `bde4aa0f76`, and the code has changed since
then in three commits:
- `2a81483e91` and `63f51fa22c` address @comphead's review.
- The consumer now registers when the first batch is buffered, so it no
longer takes a share from a sort that is still reading its input.
- `evaluate` counts the concatenated copy with the counter that already
holds the buffered batches. 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.
- The last row's keys are kept from the previous batch instead of being
re-evaluated each time.
- The failure test asserts the `CometWindowAggExec cannot spill` hint.
- The first Scala test is now `windows/window_whole_partition_batches.sql`.
- The docs and the upgrade guide are updated.
- `766e0dea10` is a small simplification that shares the grow-and-wrap
helper and the test pool lookup.
- `35eac17a70` is the latest merge of `main`. #7014 moved window planning
into `planner/window.rs`, so the swap to `CometWindowAggExec` now lives there.
`planner.rs` is back to `main`'s version. That swap is the only change to
existing code in the PR.
Locally, the 7 `window_agg` tests pass, and `cargo fmt` and `cargo clippy -D
warnings` are clean. The Spark 4.1 jobs passed on `766e0dea10` with
`run-spark-4.1-tests`, and I'll check them again on the new head.
One thing is still open. @comphead asked for a TPC-DS window query under
`fair_unified`, with a pool small enough to make the sort spill, compared
against `main`. I haven't run that yet. The standalone bench doesn't cover it,
so I can't say yet whether the share dilution or the per-batch pool calls add
cost there. Do you want that run before this merges?
`BoundedWindowAggExec` is still untracked. That is #7092, and this PR is
`Part of #6253`.
--
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]
