zml1206 commented on PR #57815: URL: https://github.com/apache/spark/pull/57815#issuecomment-5251782126
> I went through this in depth — traced the eligibility gate, the hash->sorter dedup and permanent-fallback path, the growing ROWS/RANGE event model, and the caller-owned sorter lifecycle, and cross-checked the two hardest surfaces separately. The result correctness holds up: dedup semantics are preserved across the hash->sorter fallback boundary (`switchToSorter` drains all existing map keys into the sorter, and `consumeDistinctRows` keeps the earliest event per key), RANGE ties enter the same output boundary consistently because the same `upperBound.compare` drives both event visibility and the frame boundary, `NormalizeFloatingNumbers` is applied to the distinct key so `-0.0`/NaN dedup correctly, binary-unstable collations route through the sorter's semantic ordering rather than the raw-byte map, and LISTAGG's own eval-time sort plus the `hasDistinctOrderAmbiguity` guard make the order-sensitive case correct. The lifecycle is also sound — synchronous `finally` cleanup per p artition plus a `TaskContext` completion listener backstop, with idempotent `cleanupResources()` so the overlapping cleanup calls are safe. > > One robustness gap worth fixing before merge, though (not a correctness issue — results are never wrong; the query just fails instead of spilling): > > **`BytesToBytesMap` construction OOM isn't covered by the fallback.** `FirstVisibleRowsBuilder` builds the map eagerly (`DistinctWindowFunctionFrame.scala:146`), and the `BytesToBytesMap` constructor immediately calls `allocate(...)` -> `MemoryConsumer.allocateArray(...)`, which throws `SparkOutOfMemoryError` on failure rather than returning. Only `Location.append` failure is caught and routed to `switchToSorter`. So on a partition that reaches this frame with execution memory already near-exhausted (can't get even the map's initial ~1KB pointer array even after `allocatePage` tries to spill other consumers), the query fails at map construction instead of degrading to the sorter — which contradicts the frame's own comment that "an append fails, the frame permanently falls back to an external sorter." > > To be fair about scope: the _documented_ `append`-failure fallback is genuinely OOM-safe (`acquireNewPage` returns `false` rather than throwing, and `growAndRehash` catches `SparkOutOfMemoryError` internally), so it's not that a claimed fallback is broken — it's that the constructor is an additional allocation site outside the fallback. The trigger is narrow, and this isn't a regression from released Spark (distinct windows previously failed at analysis). But the intent is clearly "OOM -> spill, don't fail," and the constructor is a hole in it. Suggest wrapping the map creation in a `catch` for `SparkOutOfMemoryError` (note: `SparkOutOfMemoryError` extends `Error`, so a `NonFatal` guard won't catch it) that falls straight to the sorter path, plus a test that forces the map's initial allocation to fail. > > Minor, non-blocking: `closeEventResources` reads `eventSorter.getSpillSize()` after the sorter may already be cleaned up; it's correct today only because `totalSpillBytes` survives `cleanupResources()` — fragile if that ever changes, but fine as-is. > > The config (`4.4.0`, internal, `NOT_APPLICABLE` binding policy) and the LISTAGG golden regeneration look right, and cloud-fan's comment-wording nit reads as already addressed in the current head. Good catch. I now catch SparkOutOfMemoryError from BytesToBytesMap construction and fall back directly to the sorter path. I also added tests covering both successful fallback and OOM propagation when the fallback sorter cannot allocate either. -- 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]
