pavan51 commented on PR #57346: URL: https://github.com/apache/spark/pull/57346#issuecomment-5628528801
Thanks @cloud-fan for the thorough review! Here's a summary of everything addressed: **P1 (Blocking) — Close RANGE lower cursor at partition boundary:** Added `closeIterator()` calls in `prepare()` to release the prior partition's `lowerIterator` and `inputIterator` before replacing them. For spilled arrays, the underlying `SpillableArrayIterator` implements `Closeable`, so this releases the spill reader at partition boundary rather than deferring to task completion. Also added `AutoCloseable`/`close()` on the frame itself and registered a `TaskCompletionListener` to cover exceptional exits and task cancellation. **P2 — Random and tied-RANGE fixtures now match their claimed shapes:** - The "wide random rows frame" test now uses `rand(42)`-based values instead of the default monotone `id` data, so both admit and evict paths are exercised at each step. - The "range frame on tied order key" test now uses `id/6` for `ord` (creating genuine 2-row peer groups per partition key) with `rangeBetween(-1, 1)`, so the frame boundary actually spans a full peer group. **P2 — Added multi-partition RANGE + spill test to pin cursor cleanup:** Added `"multi-partition RANGE with spill closes cursors at partition boundary"` — 9 partition keys each forced to spill with low in-memory thresholds. This pins the `close()` at partition boundary contract and would fail (correctness first, then file-descriptor exhaustion) if cleanup broke. **P2 — Benchmark matrix and frame-width labels corrected:** Section B windows changed from `W=10/W=50` to `W=11/W=51` (odd widths = `2*halfW+1`) and the Scaladoc matrix description updated to match the actual 6-aggregate layout (MIN/MAX get 4 cells including deque; others get 3). **P2 — Pin precedence and exact cardinality for the routing metric:** Added `expectedDequeCount` parameter to `checkEquivalence` (default=3 matching `baseDF`'s 3 partitions). Added an explicit precedence test: when both deque and seg-tree flags are on, deque wins, seg-tree counter stays 0, and exactly 3 deque frames are counted. **P2 — Config doc updated:** The `WINDOW_MONOTONIC_DEQUE_ENABLED` doc now states the eligibility gate: "Applies only when the moving frame contains exclusively Min/Max aggregates with no FILTER clauses." Let me know if anything else needs attention before merge! -- 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]
