feat: complex query thread pool

[Swiddis]

Description

Following discussion on #5608, this PR implements a dedicated thread pool for slow queries. This helps keep the cluster responsive in cases of specific expensive queries being run, as the main worker thread pool will still be available to process fast queries. This comes with a plugins.sql.slow_worker_pool.enabled setting (default true).

The pool exists as a separate resource that the worker can hand off to. So the normal fast query path is unchanged, we only hand off in known special cases. This also means we only pay rescheduling overhead for queries that are already slow.

I've tested this by saturating a cluster with >20sec queries (until all slow threads are active and requests are queued), and then verified that normal cheap queries are still responsive even if slow queries are at capacity.

Guide for reviewers:

• The detection rules live in ScriptDetector.java. We primary look for specific expensive UDFs (rex, parse), window functions, joins, or any script pushdown contexts.
  • The biggest class of false-negatives is aggregations with high cardinality, we can't determine from the query plan alone that an aggregation field is very large. We'd need some sort of statistics tracking on the index. I don't want to put all aggregations in the slow pool since they're very common for dashboarding and many aggregations are low-cardinality.
• Execution dispatch is in ThreadPoolExecutionDispatcher, if we hit the ScriptDetector check we do a handoff involving a lot of thread context propagation and cleanup. This is the riskiest part of the code, I've been carefully checking that the context propagation is working correctly and that we handle things like auth context (field level security) and cancellation.

Related Issues

Resolves #5608

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 1053561)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The cancellation poller schedules a recurring task that interrupts the execution thread when the task is cancelled. However, if the execution completes before the first poll interval (500ms), the poller continues running and may interrupt the thread after it has been returned to the pool and assigned to a different task. This can cause unrelated queries to fail with spurious interrupts. private Cancellable scheduleCancellationPoller( @Nullable CancellableTask cancellableTask, Thread executionThread) { if (cancellableTask == null) { return NOOP_CANCELLABLE; } return threadPool.scheduleWithFixedDelay( () -> { if (cancellableTask.isCancelled()) { LOG.debug("Task cancelled, interrupting complex pool execution thread"); executionThread.interrupt(); } }, CANCEL_POLL_INTERVAL, ThreadPool.Names.GENERIC); } Resource Leak If an exception is thrown before the timeoutHandle and cancelPoller are assigned (for example, during ThreadContext restoration or task setup), the scheduled timeout and cancellation poller are never cancelled. This leaves background tasks running indefinitely, consuming thread pool resources. threadPool.schedule( () -> { final Thread executionThread = Thread.currentThread(); Cancellable timeoutHandle = threadPool.schedule( () -> { LOG.warn( "Query execution timed out after {}. Interrupting execution thread.", timeout); executionThread.interrupt(); }, timeout, ThreadPool.Names.GENERIC); Cancellable cancelPoller = scheduleCancellationPoller(cancellableTask, executionThread); Hook.Closeable hookHandle = null; try { // Restore state from caller thread ThreadContext.putAll(ctx); OpenSearchQueryManager.setCancellableTask(cancellableTask); QueryProfiling.set(profileContext); CalcitePlanContext.restoreThreadLocals(snapshot); // Override execution pool to indicate complex pool CalcitePlanContext.executionPool.set(SQL_COMPLEX_WORKER_THREAD_POOL_NAME); if (metadataProvider != null) { RelMetadataQueryBase.THREAD_PROVIDERS.set(metadataProvider); } if (currentTime >= 0) { hookHandle = Hook.CURRENT_TIME.addThread((Consumer<Holder<Long>>) h -> h.set(currentTime)); } task.run(); } catch (Exception e) { LOG.error("Exception during task execution on complex pool", e); if (failureListener != null) { failureListener.onFailure(e); } } finally { timeoutHandle.cancel(); cancelPoller.cancel(); Thread.interrupted(); if (hookHandle != null) { hookHandle.close(); } OpenSearchQueryManager.clearCancellableTask(); RelMetadataQueryBase.THREAD_PROVIDERS.remove(); CalcitePlanContext.clearTimewrapSignals(); QueryProfiling.clear(); } }, Possible Issue The optimize call was removed from the run method. If any caller still invokes run directly without first calling optimize, the plan will execute unoptimized. This can lead to incorrect query results or performance degradation. The PR optimizes before dispatch in QueryService.executeCalcitePlan and RestUnifiedQueryAction.doExecute, but other call sites (e.g., tests, explain paths) may bypass optimization. public static PreparedStatement run(CalcitePlanContext context, RelNode rel) { ProfileMetric optimizeTime = QueryProfiling.current().getOrCreateMetric(OPTIMIZE); long startTime = System.nanoTime(); final RelShuttle shuttle = new RelHomogeneousShuttle() { @Override public RelNode visit(TableScan scan) { Possible Issue The timeout handler interrupts the execution thread after the configured timeout. However, if the execution completes just before the timeout fires, the interrupt may target the thread after it has been returned to the pool and assigned to a different task. This can cause unrelated queries to fail with spurious interrupts. Cancellable timeoutHandle = threadPool.schedule( () -> { LOG.warn( "Query execution timed out after {}. Interrupting execution thread.", timeout); executionThread.interrupt(); }, timeout, ThreadPool.Names.GENERIC);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 1053561

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Cancel future on interrupt When InterruptedException is caught, the thread's interrupt flag is restored but the nextBatchFuture may still be running. The future should be cancelled to prevent resource leaks and ensure the background task stops executing. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java [122-125] } catch (InterruptedException e) { Thread.currentThread().interrupt(); + nextBatchFuture.cancel(true); throw new TaskCancelledException("The task is cancelled."); } catch (ExecutionException e) { Suggestion importance[1-10]: 8 __ Why: This is a valid resource leak concern. When InterruptedException is caught, the nextBatchFuture should be cancelled to prevent the background task from continuing to execute and consume resources. This is important for proper cleanup.	Medium
	Cancel task on timeout The timeout handler interrupts the execution thread but doesn't cancel the underlying task. If the task is blocked on I/O or ignores interrupts, it will continue running. Consider cancelling the CancellableTask in addition to interrupting the thread to ensure proper cleanup. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [82-91] Cancellable timeoutHandle = threadPool.schedule( () -> { LOG.warn( - "Query execution timed out after {}. Interrupting execution thread.", + "Query execution timed out after {}. Interrupting execution thread and cancelling task.", timeout); + if (cancellableTask != null) { + cancellableTask.cancel("Query execution timeout"); + } executionThread.interrupt(); }, timeout, ThreadPool.Names.GENERIC); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that interrupting the thread alone may not stop tasks blocked on I/O. Cancelling the CancellableTask provides a more robust cleanup mechanism. However, the impact is moderate since the interrupt mechanism may already handle most cases.	Medium
General	Fix metric timing accuracy The analyzeMetric is set before dispatching to the complex worker pool. If the dispatcher routes to a different thread, the metric timing won't include the actual execution time. Consider moving the metric recording inside the dispatch callback or after execution completes. core/src/main/java/org/opensearch/sql/executor/QueryService.java [242-250] RelNode optimizedPlan = CalciteToolsHelper.optimize(calcitePlan, context); -analyzeMetric.set(System.nanoTime() - analyzeStart); // Wrap execution with EXECUTING stage tracking — dispatch via // ExecutionDispatcher which may route to a complex worker pool StageErrorHandler.executeStageVoid( QueryProcessingStage.EXECUTING, - () -> executionDispatcher.dispatch(optimizedPlan, context, listener, executionEngine), + () -> { + analyzeMetric.set(System.nanoTime() - analyzeStart); + executionDispatcher.dispatch(optimizedPlan, context, listener, executionEngine); + }, "while running the query"); Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid point about metric timing, but the current placement appears intentional—the analyzeMetric measures analysis/optimization time, not execution time. Moving it inside dispatch would change what's being measured. The impact is low since this is primarily a profiling concern.	Low
	Check interrupt status before clearing The Thread.interrupted() call clears the interrupt flag but doesn't check its value. If the thread was interrupted due to timeout or cancellation, the exception handler may have already been invoked. Verify the interrupt status before clearing to avoid masking the interrupt signal. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [110-118] } catch (Exception e) { LOG.error("Exception during task execution on complex pool", e); if (failureListener != null) { failureListener.onFailure(e); } } finally { timeoutHandle.cancel(); cancelPoller.cancel(); - Thread.interrupted(); + boolean wasInterrupted = Thread.interrupted(); + if (wasInterrupted) { + LOG.debug("Execution thread was interrupted, interrupt flag cleared"); + } Suggestion importance[1-10]: 3 __ Why: While checking the interrupt status before clearing provides better observability, the current code correctly clears the flag in the finally block. The suggested logging adds minimal value since interrupts are already handled by the exception handler and timeout/cancellation logic.	Low

---

Previous suggestions

Suggestions up to commit aaaa8e4

Category	Suggestion	Impact
Possible issue	Prevent thread leak on listener failure When an exception occurs, the timeout and cancellation polling threads continue running until the finally block executes. If the exception handler itself throws (e.g., failureListener.onFailure fails), these background threads leak. Move cleanup into a nested try-finally to guarantee cancellation. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [110-118] } catch (Exception e) { LOG.error("Exception during task execution on complex pool", e); - if (failureListener != null) { - failureListener.onFailure(e); + try { + if (failureListener != null) { + failureListener.onFailure(e); + } + } catch (Exception listenerException) { + LOG.error("Exception in failure listener", listenerException); } } finally { timeoutHandle.cancel(); cancelPoller.cancel(); Thread.interrupted(); Suggestion importance[1-10]: 8 __ Why: This is a valid concern about resource leaks. If failureListener.onFailure throws an exception, the finally block won't execute, causing timeoutHandle and cancelPoller to leak. The suggested nested try-catch ensures cleanup happens regardless of listener failures, preventing thread leaks.	Medium
	Cancel task on timeout The timeout handler interrupts the execution thread but doesn't cancel the underlying task. If the task is blocked on I/O or ignores interrupts, it will continue running indefinitely. Consider also cancelling the CancellableTask when the timeout fires to ensure proper cleanup. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [82-90] Cancellable timeoutHandle = threadPool.schedule( () -> { LOG.warn( - "Query execution timed out after {}. Interrupting execution thread.", + "Query execution timed out after {}. Interrupting execution thread and cancelling task.", timeout); executionThread.interrupt(); + if (cancellableTask != null) { + cancellableTask.cancel("Query execution timed out"); + } }, timeout, ThreadPool.Names.GENERIC); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the timeout handler only interrupts the thread but doesn't cancel the underlying CancellableTask. This could lead to tasks continuing to run if they ignore interrupts. However, the impact is moderate since thread interruption is already in place, and the suggestion assumes CancellableTask has a cancel method which needs verification.	Medium
	Cancel future on interrupt When InterruptedException is caught, the thread's interrupt flag is restored but the nextBatchFuture remains active. If the future is blocked on I/O, it won't be cancelled and may hold resources. Cancel the future before rethrowing to ensure proper cleanup. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java [122-125] } catch (InterruptedException e) { Thread.currentThread().interrupt(); + nextBatchFuture.cancel(true); throw new TaskCancelledException("The task is cancelled."); } catch (ExecutionException e) { Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that when InterruptedException is caught, the nextBatchFuture should be cancelled to release resources. However, the code already restores the interrupt flag with Thread.currentThread().interrupt(), which is good practice. Adding nextBatchFuture.cancel(true) would improve resource cleanup, though the existing code is not critically broken.	Medium
General	Measure analysis time accurately The analyzeMetric is set before dispatching to the complex worker pool, so it measures only the caller thread's work. If optimization is expensive and dispatch adds latency, the metric will be inaccurate. Consider measuring total analysis time including dispatch overhead. core/src/main/java/org/opensearch/sql/executor/QueryService.java [242-250] RelNode optimizedPlan = CalciteToolsHelper.optimize(calcitePlan, context); -analyzeMetric.set(System.nanoTime() - analyzeStart); // Wrap execution with EXECUTING stage tracking — dispatch via // ExecutionDispatcher which may route to a complex worker pool StageErrorHandler.executeStageVoid( QueryProcessingStage.EXECUTING, - () -> executionDispatcher.dispatch(optimizedPlan, context, listener, executionEngine), + () -> { + analyzeMetric.set(System.nanoTime() - analyzeStart); + executionDispatcher.dispatch(optimizedPlan, context, listener, executionEngine); + }, "while running the query"); Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid point about metric accuracy, but the impact is limited. The current implementation measures analysis time before dispatch, which is reasonable since dispatch overhead is part of execution, not analysis. Moving the metric inside the lambda would include dispatch scheduling time, which may or may not be desired depending on the metric's purpose.	Low

Suggestions up to commit aaaa8e4

Category	Suggestion	Impact
Possible issue	Cancel task on timeout The timeout handler interrupts the execution thread but doesn't cancel the underlying task. If the task is blocked on I/O or ignores interrupts, it will continue running. Consider also cancelling the CancellableTask to ensure proper cleanup and prevent resource leaks. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [82-91] Cancellable timeoutHandle = threadPool.schedule( () -> { LOG.warn( - "Query execution timed out after {}. Interrupting execution thread.", + "Query execution timed out after {}. Interrupting execution thread and cancelling task.", timeout); + if (cancellableTask != null) { + cancellableTask.cancel("Query execution timed out"); + } executionThread.interrupt(); }, timeout, ThreadPool.Names.GENERIC); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the timeout handler only interrupts the thread without cancelling the underlying CancellableTask. This could lead to resource leaks if the task ignores interrupts. However, the impact is moderate since the interrupt mechanism is already in place and may be sufficient for most cases.	Medium
General	Log interrupt status before clearing The finally block clears the interrupt flag with Thread.interrupted() but doesn't check if the thread was actually interrupted. If the thread was interrupted due to timeout or cancellation, this information is lost, potentially masking the root cause of failures. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [110-118] } catch (Exception e) { LOG.error("Exception during task execution on complex pool", e); if (failureListener != null) { failureListener.onFailure(e); } } finally { timeoutHandle.cancel(); cancelPoller.cancel(); - Thread.interrupted(); + if (Thread.interrupted()) { + LOG.debug("Execution thread was interrupted, likely due to timeout or cancellation"); + } Suggestion importance[1-10]: 4 __ Why: While logging the interrupt status before clearing could provide useful debugging information, the current code already logs errors during execution. The suggestion adds marginal value for debugging but doesn't address a critical issue. The interrupt flag is intentionally cleared to prevent it from affecting subsequent operations.	Low
	Capture optimization time accurately The analyzeMetric is set before dispatching to the execution pool, but if the query is routed to the complex worker pool, there's a delay before actual execution starts. This makes the analyze metric include queuing time, which should be tracked separately for accurate profiling. core/src/main/java/org/opensearch/sql/executor/QueryService.java [242-250] RelNode optimizedPlan = CalciteToolsHelper.optimize(calcitePlan, context); -analyzeMetric.set(System.nanoTime() - analyzeStart); +long optimizeEnd = System.nanoTime(); +analyzeMetric.set(optimizeEnd - analyzeStart); // Wrap execution with EXECUTING stage tracking — dispatch via // ExecutionDispatcher which may route to a complex worker pool StageErrorHandler.executeStageVoid( QueryProcessingStage.EXECUTING, () -> executionDispatcher.dispatch(optimizedPlan, context, listener, executionEngine), "while running the query"); Suggestion importance[1-10]: 3 __ Why: The suggestion adds an intermediate variable optimizeEnd but doesn't change the actual timing calculation. The analyzeMetric is already set immediately after optimization completes (line 243), so the timing is accurate. The suggestion doesn't improve accuracy and only adds an unnecessary variable.	Low

Suggestions up to commit 009300a

Category	Suggestion	Impact
General	Stop cancellation poller after interruption The cancellation poller should stop polling once it detects cancellation to avoid unnecessary overhead. After interrupting the execution thread, the poller continues running until the task completes. Add a check to cancel the scheduled task after interruption. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [143-157] private Cancellable scheduleCancellationPoller( @Nullable CancellableTask cancellableTask, Thread executionThread) { if (cancellableTask == null) { return NOOP_CANCELLABLE; } - return threadPool.scheduleWithFixedDelay( + final Cancellable[] holder = new Cancellable[1]; + holder[0] = threadPool.scheduleWithFixedDelay( () -> { if (cancellableTask.isCancelled()) { LOG.debug("Task cancelled, interrupting complex pool execution thread"); executionThread.interrupt(); + if (holder[0] != null) { + holder[0].cancel(); + } } }, CANCEL_POLL_INTERVAL, ThreadPool.Names.GENERIC); + return holder[0]; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the cancellation poller continues running after interrupting the execution thread, which is inefficient. However, the proposed solution has a potential race condition where holder[0] might be accessed before it's assigned. A better approach would be to return a wrapper Cancellable that cancels itself after interruption.	Medium
	Preserve original exception as cause When catching InterruptedException, the thread's interrupt status is restored but the original exception is lost. The TaskCancelledException should preserve the original cause for better debugging and error tracking. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java [122-125] } catch (InterruptedException e) { Thread.currentThread().interrupt(); - throw new TaskCancelledException("The task is cancelled."); + throw new TaskCancelledException("The task is cancelled.", e); } catch (ExecutionException e) { Suggestion importance[1-10]: 7 __ Why: Valid suggestion to preserve the original InterruptedException as the cause of TaskCancelledException for better debugging. This improves error tracking without changing the behavior, though the impact is moderate since the exception message already indicates cancellation.	Medium
	Handle InterruptedException explicitly before clearing flag The Thread.interrupted() call clears the interrupt flag but doesn't handle the case where the thread was interrupted during execution. If an InterruptedException occurs, it should be caught separately to ensure proper cleanup and listener notification before clearing the flag. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [99-107] +} catch (InterruptedException e) { + LOG.error("Task interrupted on complex pool", e); + Thread.currentThread().interrupt(); + if (failureListener != null) { + failureListener.onFailure(e); + } } catch (Exception e) { LOG.error("Exception during task execution on complex pool", e); if (failureListener != null) { failureListener.onFailure(e); } } finally { cancelPoller.cancel(); Thread.interrupted(); Suggestion importance[1-10]: 6 __ Why: The suggestion to handle InterruptedException separately is reasonable for clarity, but the current code already handles interruption correctly by clearing the flag in the finally block. The improvement is marginal since the generic Exception catch already covers InterruptedException and notifies the listener appropriately.	Low
	Cap background pool size limit The background thread pool size is doubled from the search pool size, which may cause resource contention on nodes with many processors. Consider using a fixed multiplier or capping the pool size to prevent excessive thread creation on large nodes. plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java [480-485] new FixedExecutorBuilder( settings, SQL_BACKGROUND_THREAD_POOL_NAME, - settings.getAsInt( - "thread_pool.search.size", 2 * OpenSearchExecutors.allocatedProcessors(settings)), + Math.min( + settings.getAsInt( + "thread_pool.search.size", 2 * OpenSearchExecutors.allocatedProcessors(settings)), + 32), 1000, "thread_pool." + SQL_BACKGROUND_THREAD_POOL_NAME)); Suggestion importance[1-10]: 5 __ Why: The suggestion to cap the background pool size is reasonable to prevent excessive thread creation on large nodes. However, the hardcoded limit of 32 is arbitrary and may not be appropriate for all deployments. A configurable setting or a more principled calculation would be better. The current implementation already uses 2 * allocatedProcessors, which is a reasonable default.	Low

Suggestions up to commit 9ba8eef

Category	Suggestion	Impact
General	Review background pool size increase The background thread pool size is set to 2 * allocatedProcessors, which doubles the previous default of allocatedProcessors. This significant increase in thread count may cause resource contention and excessive context switching under load. Verify this sizing is intentional or revert to the original value. plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java [482-485] new FixedExecutorBuilder( settings, SQL_BACKGROUND_THREAD_POOL_NAME, settings.getAsInt( - "thread_pool.search.size", 2 * OpenSearchExecutors.allocatedProcessors(settings)), + "thread_pool.search.size", OpenSearchExecutors.allocatedProcessors(settings)), 1000, "thread_pool." + SQL_BACKGROUND_THREAD_POOL_NAME)); Suggestion importance[1-10]: 8 __ Why: This is a significant change that doubles the background thread pool size from allocatedProcessors to 2 * allocatedProcessors. The suggestion correctly identifies this as a potentially impactful change that warrants verification. The increased pool size could lead to resource contention, though it may be intentional to handle increased async workload from the new complex worker pool. This deserves careful review.	Medium
	Stop polling after cancellation detected The cancellation poller should stop polling once it detects cancellation to avoid unnecessary overhead. After interrupting the execution thread, the poller continues running until the task completes. Add a mechanism to cancel the scheduled task after the first interrupt. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [148-157] private Cancellable scheduleCancellationPoller( @Nullable CancellableTask cancellableTask, Thread executionThread) { if (cancellableTask == null) { return NOOP_CANCELLABLE; } - return threadPool.scheduleWithFixedDelay( + final Cancellable[] holder = new Cancellable[1]; + holder[0] = threadPool.scheduleWithFixedDelay( () -> { if (cancellableTask.isCancelled()) { LOG.debug("Task cancelled, interrupting complex pool execution thread"); executionThread.interrupt(); + if (holder[0] != null) { + holder[0].cancel(); + } } }, CANCEL_POLL_INTERVAL, ThreadPool.Names.GENERIC); + return holder[0]; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the cancellation poller continues running after detecting cancellation. However, the implementation has a subtle issue: the poller tries to cancel itself from within its own execution, which may not work reliably. A better approach would be to return the Cancellable and let the caller manage it in the finally block, which already calls cancelPoller.cancel().	Medium
	Preserve thread interrupt status during cleanup Calling Thread.interrupted() clears the interrupt flag without checking it first. If the thread was interrupted due to cancellation, this information is lost before cleanup completes. Check and preserve the interrupt status to ensure proper cleanup behavior. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [99-106] } catch (Exception e) { LOG.error("Exception during task execution on complex pool", e); if (failureListener != null) { failureListener.onFailure(e); } } finally { cancelPoller.cancel(); - Thread.interrupted(); + boolean wasInterrupted = Thread.interrupted(); + try { + // cleanup code + } finally { + if (wasInterrupted) { + Thread.currentThread().interrupt(); + } + } Suggestion importance[1-10]: 6 __ Why: The suggestion raises a valid concern about clearing the interrupt flag with Thread.interrupted(). However, the current code appears intentional - it clears the flag to prevent it from affecting subsequent operations. The suggestion's approach of re-interrupting after cleanup could cause issues if the thread is reused from a pool. The current implementation may be acceptable depending on the threading model.	Low
	Preserve exception cause chain on interrupt When InterruptedException is caught, the thread's interrupt flag is restored but the original exception is replaced with TaskCancelledException. This loses the original exception's stack trace and cause chain, making debugging difficult. Preserve the original exception as the cause. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java [122-125] } catch (InterruptedException e) { Thread.currentThread().interrupt(); - throw new TaskCancelledException("The task is cancelled."); + throw new TaskCancelledException("The task is cancelled.", e); } catch (ExecutionException e) { Suggestion importance[1-10]: 5 __ Why: The suggestion correctly notes that the original InterruptedException is not preserved as a cause. However, TaskCancelledException may not have a constructor that accepts a cause parameter. Additionally, for InterruptedException, the exception itself typically doesn't contain critical debugging information - the interrupt is the signal, not an error condition. The improvement is minor.	Low

Suggestions up to commit 21dd714

Category	Suggestion	Impact
General	Cancel future on interrupt After re-interrupting the thread, the TaskCancelledException is thrown but the nextBatchFuture may still be running. Consider cancelling the future before throwing to prevent resource leaks and ensure the background task stops promptly. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java [122-125] } catch (InterruptedException e) { Thread.currentThread().interrupt(); + nextBatchFuture.cancel(true); throw new TaskCancelledException("The task is cancelled."); } catch (ExecutionException e) { Suggestion importance[1-10]: 7 __ Why: Valid suggestion to cancel the nextBatchFuture when interrupted to prevent resource leaks and ensure the background task stops promptly. This improves cleanup behavior when a query is cancelled, though the impact depends on whether the future's task respects cancellation.	Medium
	Stop cancellation poller after interruption The cancellation poller continues running even after interrupting the execution thread. Once the task is cancelled and the thread is interrupted, the poller should cancel itself to avoid unnecessary polling overhead. Add a check to stop the poller after interruption. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [143-157] private Cancellable scheduleCancellationPoller( @Nullable CancellableTask cancellableTask, Thread executionThread) { if (cancellableTask == null) { return NOOP_CANCELLABLE; } - return threadPool.scheduleWithFixedDelay( + final Cancellable[] self = new Cancellable[1]; + self[0] = threadPool.scheduleWithFixedDelay( () -> { if (cancellableTask.isCancelled()) { LOG.debug("Task cancelled, interrupting complex pool execution thread"); executionThread.interrupt(); + if (self[0] != null) { + self[0].cancel(); + } } }, CANCEL_POLL_INTERVAL, ThreadPool.Names.GENERIC); + return self[0]; } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that the cancellation poller continues running after interrupting the execution thread. However, the poller is already cancelled in the finally block (line 105), so this is a minor optimization to stop it earlier. The impact is limited to reducing unnecessary polling cycles between cancellation detection and task completion.	Low
	Check interrupt status before clearing Calling Thread.interrupted() clears the interrupt flag without checking if the thread was actually interrupted. This can mask cancellation signals. Check the interrupt status before clearing it, and if interrupted, ensure proper cleanup or re-throw an appropriate exception. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/ThreadPoolExecutionDispatcher.java [99-106] } catch (Exception e) { LOG.error("Exception during task execution on complex pool", e); if (failureListener != null) { failureListener.onFailure(e); } } finally { cancelPoller.cancel(); - Thread.interrupted(); + if (Thread.interrupted()) { + LOG.debug("Execution thread was interrupted, clearing interrupt flag"); + } Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid point about checking interrupt status before clearing it. However, the current code clears the interrupt flag in the finally block to ensure the thread is in a clean state after execution. Adding a check would provide better logging but doesn't fundamentally change the behavior, as the interrupt flag is intentionally cleared regardless of whether the thread was interrupted.	Low
	Record metric after optimization completes If optimization throws an exception, analyzeMetric is set before the exception propagates, potentially recording incorrect timing. Move the metric recording inside a try-finally block or after confirming optimization succeeded to ensure accurate profiling even on failure. core/src/main/java/org/opensearch/sql/executor/QueryService.java [242-250] RelNode optimizedPlan = CalciteToolsHelper.optimize(calcitePlan, context); -analyzeMetric.set(System.nanoTime() - analyzeStart); +long optimizeEnd = System.nanoTime(); +analyzeMetric.set(optimizeEnd - analyzeStart); // Wrap execution with EXECUTING stage tracking — dispatch via // ExecutionDispatcher which may route to a complex worker pool StageErrorHandler.executeStageVoid( QueryProcessingStage.EXECUTING, () -> executionDispatcher.dispatch(optimizedPlan, context, listener, executionEngine), "while running the query"); Suggestion importance[1-10]: 3 __ Why: The suggestion misunderstands the code flow. The analyzeMetric is set after optimize() completes successfully (line 243). If optimization throws an exception, the metric won't be set because execution won't reach that line. The suggested change adds an unnecessary intermediate variable without improving correctness or clarity.	Low

[github-actions]

Persistent review updated to latest commit 46c3cbe

[github-actions]

Persistent review updated to latest commit 6fdd82e

[github-actions]

Persistent review updated to latest commit 434021f

[github-actions]

Persistent review updated to latest commit 169375f

[github-actions]

Persistent review updated to latest commit 9c57cd7

[github-actions]

Persistent review updated to latest commit 559a2d0

[github-actions]

Persistent review updated to latest commit 559a2d0

[penghuo]

Optimize already been called in QueryService. Why call optimize again?

[Swiddis]

ah yeah, my bad -- I was moving optimize out of 'execute' since we need the optimized plan to make complex branch decisions, and did a ctrl+f to find old execute/run call sites and make sure they called optimize.

[penghuo]

I did not get u. We should run optimize 2 times?

[penghuo]

When query schedule in complex-worker thread, the complex query timeout control OpenSearchQueryManager does not take effect.

[Swiddis]

It's applying but slowly, I poked more and it's because script-based complex queries can spend very long delays in DSL (compared to e.g. joins where individual DSL pages are fast). I added some polling so the PPL thread interrupts faster in these cases. Notably we still leave zombie search tasks behind, but this is a preexisting issue and they're still usually much faster than the overall query (involving multiple pages -- in testing I got no sub-searches beyond 10 seconds or so).

I can follow up doing more aggressive cancelation that propagates to the search task if we need it but I believe the polling impl is enough for this PR.

[github-actions]

Persistent review updated to latest commit 73c68cf

[github-actions]

Persistent review updated to latest commit 21dd714

[github-actions]

Persistent review updated to latest commit 9ba8eef

[github-actions]

Persistent review updated to latest commit 009300a

[github-actions]

Persistent review updated to latest commit 7580ddb

[github-actions]

Persistent review updated to latest commit aaaa8e4

[github-actions]

Persistent review updated to latest commit 1053561