You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix: DH-23758: Close pushdown resources on error and result paths - #8727
Fixes the resource-leak and error-path findings grouped under T-14 of the pushdown review (DH-23367). Most of them share one cause: resources were released only on the success path, or never. They are fixed together to simplify back-porting, with one commit per finding.
Scheduler (PD-061, PD-063)
JobScheduler: if submit throws (for example, a pool that is shutting down rejects the task), the iteration is now closed and failed through onError. Previously the per-task reference was never released, so onComplete, onError and cleanup never ran, and the task's context leaked. A scheduler that runs tasks inline (ImmediateJobScheduler) can also throw from submit after the task ran. That task has already closed itself, so only a task that never started is treated as rejected.
JobScheduler: when several tasks fail, the later failures are now kept as suppressed exceptions on the one that is delivered, instead of being dropped.
ColumnRegion and AbstractTableLocation: a failing pushdown action or estimate now goes to onError instead of being thrown synchronously.
ImmediateJobScheduler: documents its drain order (most recently submitted first) and that it rejects submissions from another thread during a drain.
Filter driver and OR (PD-035, PD-036)
AbstractFilterExecution:
filter inputs are closed when replaced;
a fully resolving pushdown result is closed;
the deferred path's final filter output is closed;
the parallel filter result is closed if a segment fails.
A partially resolving pushdown result is now owned by its StatelessFilter as soon as it arrives. This closes it on every failure path, including a failed re-sort or final filter, which the finding did not list.
DataIndexPushdownManager: the wrapped matcher's intermediate result is closed on both branches. When the index is skipped, it is closed exactly once, because a throwing consumer can route back into onError.
DisjunctiveFilter.orImpl: returns its accumulator instead of leaking it behind a copy, and closes it if a component throws. A disjunction with no components now matches nothing instead of everything.
Regioned, union and parquet (PD-018, PD-046, PD-019, PD-071, PD-024)
RegionedColumnSourceManager and UnionSourceManager: per-region and per-constituent row sets are also closed from onError, because the iteration's cleanup runs only after onComplete succeeds. cleanup itself is unchanged, since several callers already release the same resources in both cleanup and onError. Union constituents now keep their shifted selections in an array that both paths close, which also removes a copy. The union cost estimate records each constituent's cost in its own slot and takes the minimum on completion, instead of updating a shared minimum under a lock. Both union paths return early when no constituent that overlaps the selection supports pushdown.
PageStorePushdownHelper and ParquetTableLocation: close the RowSequence.asRowSet() intermediate. loadDataIndex closes the location row set copy when the location is flat and the copy goes unused.
Column regions build exact-match results with PushdownResult.exactMatch instead of passing an inline RowSetFactory.empty() that was never closed. ColumnRegionObject's lazily built dictionary regions are now volatile.
Python table data service (PD-063)
PythonTableDataService.readChunkPage closes the chunks it reads.
Not in this PR: PD-071(c) (IO during the dictionary estimate) and PD-071(d) (a data index cached under two names) are left to DH-23755, which replaces both code paths. PD-063(a), WritableBooleanChunk.sort() throwing for every input, is also left out: nothing calls it, and it is not a resource leak.
…submission
A throwing submit left the per-task reference unreleased, so onComplete, onError
and cleanup never ran, and the task's context leaked (PD-061).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…er's result paths
AbstractFilterExecution replaced filter inputs without closing them, never closed
a fully-resolving pushdown result or the deferred path's final filter output, and
dropped its parallel filter result on error. DataIndexPushdownManager abandoned the
wrapped matcher's intermediate result on both branches (PD-035).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
orImpl returned a copy of its accumulator and never closed it, on every OR
evaluation and when a component threw; a disjunction of no filters returned the
whole selection rather than nothing (PD-036).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…wn fails
The regioned pushdown closed its per-region results only in the iteration's
cleanup, which runs after onComplete succeeds, so a failed region or a throwing
consumer abandoned the results already produced (PD-018).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…constituent fails
UnionSourceManager closed each constituent's selection copy only in its success
callback, and its per-constituent results only in the iteration's success
cleanup, so a failing constituent leaked both (PD-046).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The result of RowSequence.asRowSet() must be closed, but only its shifted copy
was, leaking one row set per overlapping region per pushdown action (PD-019).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… and index loading
The dictionary pushdown never closed its RowSequence.asRowSet() intermediate,
and loadDataIndex never closed the location row set copy when the location is
flat and the copy goes unused (PD-071).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nclosed empty row set
Column regions passed an inline RowSetFactory.empty() to PushdownResult.of, which
copies it, leaving the temporary unclosed; use PushdownResult.exactMatch. Make the
object regions' lazily built dictionary regions volatile, so their unlocked
initialization publishes them safely (PD-024).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The replicator removed the boolean sort body, so the public sort() fell through
to the throwing default and failed for every input (PD-063).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ervice
readChunkPage never closed the chunks returned by getColumnValues, on success or
when it rejected them (PD-063).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ColumnRegion and AbstractTableLocation took an onError consumer but let a
failing pushdown action or estimate throw synchronously instead (PD-063).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Its depth-first drain order, and its refusal of submissions from a second thread
during a drain, were undocumented traps for asynchronous completers (PD-063).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
223 executable lines added or changed — 159/223 (71%) covered
5 of these files are replicated (ColumnRegionDouble, ColumnRegionFloat, ColumnRegionInt, ColumnRegionLong, ColumnRegionShort); coverage fixes belong at the template.
Gaps
The most serious gap is a capability no test drives end to end: loading a Parquet data index for a location whose row set is not flat. Every Parquet file with more than one row group produces such a location, because each row group gets its own block of row keys, but the indexed files in the tests all have a single row group. So neither the position-to-row-key translation nor this change's cleanup of the location row set when building that index fails is exercised. Next is the Python table data service's chunk-closing fix, which is never entered from Java: that service is driven only from the Python server tests, which these numbers do not measure, so the fix is verified by inspection alone. The rest is mostly error handling this change added. A failed pushdown action is shown to reach onError for a column region, but nothing injects a failure into a column region's estimate, into either table-location path, into a context that fails to close, or into the data index manager's second round when its matcher throws instead of calling onError or the merge of its results fails. The union manager's early returns, taken when no constituent that overlaps the selection supports pushdown, are also never reached. The replicated constant-region pushdowns are covered only for int, and their byte, char, short, long, float and double siblings are never entered, though they run identical logic. One defensive branch remains: the scheduler rethrowing an exception from an inline submit after its task already ran.
Measured at d47acdc against upstream/main, over test, testOutOfBand, testParallel and testSerial in :engine-table (with the :extensions-parquet-table execution data folded into its report), test and testOutOfBand in :extensions-parquet-table, and test in :extensions-barrage.
This reverts commit 074926d. No caller sorts a boolean chunk, and the fix is
unrelated to the pushdown resource leaks this branch addresses.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
If updateView or StandaloneDataIndex.from throws (for example, while validating a malformed index table), the copied locationRowSet has not acquired a successful lifetime owner and is never closed. Wrap construction of the adjusted index in a RuntimeException | Error catch that closes locationRowSet with SafeCloseable.closeAllDuringFailure before rethrowing.
This branch still leaks owned results if the wrapped matcher fails synchronously: result is released only by the callbacks. It also owns nextResult when that callback starts, so an exception while merging the earlier matches leaks nextResult. Guard the invocation and close nextResult if the merge fails; ownership transfers onward only when onComplete is called.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
lbooker42
added
NoDocumentationNeeded
NoReleaseNotesNeeded
Fixes the resource-leak and error-path findings grouped under T-14 of the pushdown review (DH-23367). Most of them share one cause: resources were released only on the success path, or never. They are fixed together to simplify back-porting, with one commit per finding.
Scheduler (PD-061, PD-063)
JobScheduler: ifsubmitthrows (for example, a pool that is shutting down rejects the task), the iteration is now closed and failed throughonError. Previously the per-task reference was never released, soonComplete,onErrorandcleanupnever ran, and the task's context leaked. A scheduler that runs tasks inline (ImmediateJobScheduler) can also throw fromsubmitafter the task ran. That task has already closed itself, so only a task that never started is treated as rejected.JobScheduler: when several tasks fail, the later failures are now kept as suppressed exceptions on the one that is delivered, instead of being dropped.ColumnRegionandAbstractTableLocation: a failing pushdown action or estimate now goes toonErrorinstead of being thrown synchronously.ImmediateJobScheduler: documents its drain order (most recently submitted first) and that it rejects submissions from another thread during a drain.Filter driver and OR (PD-035, PD-036)
AbstractFilterExecution:A partially resolving pushdown result is now owned by its
StatelessFilteras soon as it arrives. This closes it on every failure path, including a failed re-sort or final filter, which the finding did not list.DataIndexPushdownManager: the wrapped matcher's intermediate result is closed on both branches. When the index is skipped, it is closed exactly once, because a throwing consumer can route back intoonError.DisjunctiveFilter.orImpl: returns its accumulator instead of leaking it behind a copy, and closes it if a component throws. A disjunction with no components now matches nothing instead of everything.Regioned, union and parquet (PD-018, PD-046, PD-019, PD-071, PD-024)
RegionedColumnSourceManagerandUnionSourceManager: per-region and per-constituent row sets are also closed fromonError, because the iteration'scleanupruns only afteronCompletesucceeds.cleanupitself is unchanged, since several callers already release the same resources in bothcleanupandonError. Union constituents now keep their shifted selections in an array that both paths close, which also removes a copy. The union cost estimate records each constituent's cost in its own slot and takes the minimum on completion, instead of updating a shared minimum under a lock. Both union paths return early when no constituent that overlaps the selection supports pushdown.PageStorePushdownHelperandParquetTableLocation: close theRowSequence.asRowSet()intermediate.loadDataIndexcloses the location row set copy when the location is flat and the copy goes unused.PushdownResult.exactMatchinstead of passing an inlineRowSetFactory.empty()that was never closed.ColumnRegionObject's lazily built dictionary regions are nowvolatile.Python table data service (PD-063)
PythonTableDataService.readChunkPagecloses the chunks it reads.Not in this PR: PD-071(c) (IO during the dictionary estimate) and PD-071(d) (a data index cached under two names) are left to DH-23755, which replaces both code paths. PD-063(a),
WritableBooleanChunk.sort()throwing for every input, is also left out: nothing calls it, and it is not a resource leak.