gortiz opened a new pull request, #19462: URL: https://github.com/apache/pinot/pull/19462
## The invariant A multi-stage operator should drop its buffered row and hash state on **every** termination path — end of stream, error, and cancellation alike. Today most operators release only on the success path, via `onEosProduced()`. The base `MultiStageOperator.close()` and `cancel(Throwable)` recurse to child operators and release nothing themselves, so an operator that fails or is cancelled keeps its buffers for as long as anything holds the operator tree. Those buffers are live references, so no GC — not even a full one — can reclaim them. `SortedMailboxReceiveOperator`, `BaseMailboxReceiveOperator` and `LeafOperator` already did the right thing; the rest did not. ## What this changes Adds a `protected MultiStageOperator#releaseBuffers()` hook that both `close()` and `cancel()` invoke after recursing to children. An exception from it is logged and swallowed so it can never break the rest of the teardown. The default is a no-op, so operators that buffer nothing are unaffected. Implemented where per-query state is buffered: | Operator | State released | Was it released before? | |---|---|---| | `SortOperator` | `_priorityQueue`, `_rows` | Never, on any path | | `BinarySetOperator` (INTERSECT / EXCEPT) | `_rightRowSet` | Never, on any path | | `UnionOperator` | `_seenRecords` | Never, on any path | | `NonEquiJoinOperator` | `_rightTable` — the whole right side | No; only the much smaller `_matchedRightRows` | | `HashJoinOperator` | `_rightTable`, `_matchedRightRows`, `_nullKeyRightRows` | Success path only | | `AsofJoinOperator` | `_rightTable` | Success path only | | `AggregateOperator` | group-by / aggregation executors | Success path only — the error return sits two lines above where they were nulled | | `RepeatOperator` | `_currentRows` | Never | `BaseJoinOperator#onEosProduced()` now delegates to `releaseBuffers()`, so the prompt success-path release stays and the hook is the backstop rather than a replacement. That also picks up a case `onEosProduced()` missed entirely: a RIGHT/FULL join whose last output is the non-matched right rows sets `_eos` inside `buildJoinedDataBlock()` and returns via the early `if (_eos != null)` branch, so `onEosProduced()` never ran. ### Design note I went with one hook on the base class rather than a `close()`/`cancel()` pair in each operator. It keeps the per-operator diff to a single method, and it makes it impossible to add a release that forgets to `super`-recurse to children — which is exactly the bug `SortOperator` had (below). The trade-off is a new (no-op default) method on the shared base class. ## Two related fixes found on the way **`SortOperator#cancel(Throwable)` was an empty override.** Not "released nothing" — it did not call `super`, so cancelling a sort never reached the operators feeding it. Removed, so the base implementation recurses again. Covered by `sortOperatorCancelReachesChildren`. **`SortedMailboxReceiveOperator#close()` cleared a list it had already handed downstream.** It emits `new RowHeapDataBlock(_rows, …)` and then `close()` called `_rows.clear()`. `InMemorySendingMailbox` passes blocks to the receiving mailbox by reference, and the receiving op chain reads them after the sending one has been closed, so emptying that list could silently drop rows rather than fail loudly. It now drops the reference instead of mutating the list. `SortOperator._rows` had the same shape (it is handed out as `_rows.subList(...)`, a view backed by the same list) and is likewise nulled rather than cleared. The `releaseBuffers()` javadoc states the rule: `clear()` only a collection whose identity never leaves the operator; otherwise drop the reference. Both are covered by regression tests asserting the emitted block survives `close()`. `LeafOperator#close()`/`cancel()` now call `super` so the hook runs for every operator, not only the ones that already chained up. ## Deliberately not touched - `WindowAggregateOperator` buffers into a local variable, so nothing is retained on the operator. - `LookupJoinOperator#_rightTable` is a shared `DimensionTableDataManager`, not per-query state. - `PipelineBreakerOperator#_resultMap` is handed to the main op chain through `getResultMap()` and must outlive the operator — releasing it would be a correctness bug. ## Testing New `OperatorBufferReleaseTest` (14 cases) asserts the buffers are gone after **error** and after **cancel**, and that emitted blocks survive `close()`, for `SortOperator`, `HashJoinOperator`, `NonEquiJoinOperator`, `AggregateOperator`, `RepeatOperator`, `IntersectOperator` and `UnionOperator`. Two cases added to `SortedMailboxReceiveOperatorTest` for the block-emptying regression and the error-path release. The buffers are private, so the tests read them reflectively rather than widening the production API. Full `pinot-query-runtime` suite: 4616 tests, 0 failures, 0 errors. ## Labels `bugfix` -- 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]
