pavan51 commented on PR #57346:
URL: https://github.com/apache/spark/pull/57346#issuecomment-5199996815
Hi @cloud-fan and @peter-toth,
Thank you for the detailed reviews! All of the blocking issues, non-blocking
findings, and nits have been completely addressed in the latest commits.
**@cloud-fan (Blocking & Nits)**
* **Shrinking-frame memory footprint:** Addressed. I removed shrinking-frame
deque routing entirely. The `isMinMaxOnly` logic now properly skips shrinking
frames (falling back to the `eligibleForSegTree` path or
`UnboundedFollowingWindowFunctionFrame`). We can revisit shrinking frames once
task-memory accounting and partition-size fallbacks are added.
* **Benchmark contamination (Finding 2):** Addressed. I explicitly pinned
`SQLConf.WINDOW_MONOTONIC_DEQUE_ENABLED.key -> "false"` in `runSpillGuard` and
`runSectionB` so they won't drift again. The benchmarks were then fully
regenerated via the standard GitHub Actions runner (Linux / AMD EPYC 9V74). The
new results confirm the spill-guard now properly tests the Segment Tree
(showing a ~18.6X
speedup vs the 50-second naive string run).
* **Hot-path allocations:** Replaced all `deques.foreach(...)` calls in
`prepare()` and `write()` with indexed `while (di < deques.length)` loops to
eliminate the per-row closure allocation on the
hot path.
* **Nits:** Fixed the "guaranteed to be" missing word, corrected the
Scaladoc grammar ("consists"/"implementations"), and removed the orphaned T5/T8
stub sentence in `WindowSegmentTreeMemorySuite`.
**@peter-toth (Correctness & Suite findings)**
* **Finding 1 (Stale PR Description & Numbers):** Addressed. I updated the
main PR description to reflect the current code (defaulting to `false`). I also
updated the benchmark numbers to reflect the newly generated results on the
standard CI runner, which proves an incredible **~40X speedup** (4414ms naive
-> 109ms deque) for MAX sliding window `W=1001`.
* **Finding 3 (Tie-breaking semantics):** Updated `admit` to use strict
comparisons so the deque retains the *first* tied value rather than the last,
perfectly matching the semantics of the existing naive and segment-tree paths.
* **Finding 5 (Routing assertions):** Added routing assertions and metric
checks to `MonotonicDequeWindowFunctionSuite` to verify the deque path is
strictly taken, along with mixed-aggregate and
`FILTER` negative cases.
* **Finding 6 & 7 (Copy path & `evaluateAndCopy`):** Removed the custom copy
logic, per-deque `tempRow`, and `isPrimitive` allowlist. `evaluateAndCopy` now
directly defers to `InternalRow.
copyValue()`. We also lowered the test thresholds to guarantee it triggers
spills and exercises the `UnsafeRow` recycling condition.
* **Findings 9, 10, & 11 (Benchmark coverage):** Restored the
`blockSize=256` column in Section A, added `Section H` (W=1, 3, 11) to ensure
the deque is beneficial even at minimal window widths,
fixed case names, and restored Section B in smoke mode.
* **Findings 8, 12, & 13:** Documented the `AggregateProcessor.evaluate`
contract, removed the unused `MinMaxDeque.bufferIndex`, and removed the no-op
`withSQLConf` class-level override in the
Segment Tree suites.
Please let me know if anything is missing.
--
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]
