[Feature] Integrate SQL/PPL with query-insights plugin by KishoreKicha14 · Pull Request #5636 · opensearch-project/sql

KishoreKicha14 · 2026-07-19T09:01:52Z

Description

Propagate SQL/PPL query metadata to OpenSearch's query-insights plugin so that DSL queries generated by the SQL engine are identifiable and traceable back to their originating SQL/PPL statement.

Changes:

Add thread context headers (x-query-source, x-original-query, x-query-execution-id, x-query-phases) set before DSL execution
Register headers via getTaskHeaders() so TaskManager copies them into SearchTask for query-insights to read
Add QueryPhaseTracker to instrument parse/analyze/plan phases with wall-clock time, CPU time, and memory allocation per phase
Execution ID links multiple DSL queries from a single SQL/PPL execution (e.g., JOINs, pagination)

Related Issues

Resolves #5677

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 · 2026-07-19T09:02:47Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit bd9913a.

Path	Line	Severity	Description
plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportSqlQueryAction.java	49	low	The CancellableTask is set on the executing thread's ThreadLocal via OpenSearchQueryManager.setCancellableTask() but is never cleared in a finally block within doExecute. In a thread-pool context this means transport worker threads may retain a stale task reference after the action completes, potentially causing subsequent unrelated requests on the same thread to inherit the old coordinator task as their parent. This is inconsistent with the save/restore pattern used in BackgroundSearchScanner.searchWithTask and looks like an oversight rather than malicious intent.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 0 | Low: 1

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-07-19T09:03:39Z

PR Reviewer Guide 🔍

(Review updated until commit `d12593c`)

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 Thread Safety Issue The `searchWithTask` method restores the previous task in the finally block, but if `previous` is null, it calls `clearCancellableTask()`. This can cause a race condition when multiple background threads share the same pool: if thread A sets a task, thread B (with previous=null) completes and clears the ThreadLocal, thread A's task is lost even though A is still executing. This occurs when concurrent queries use the same background executor pool. private OpenSearchResponse searchWithTask( OpenSearchRequest request, @Nullable CancellableTask task) { if (task == null) { return client.search(request); } CancellableTask previous = OpenSearchQueryManager.getCancellableTask(); OpenSearchQueryManager.setCancellableTask(task); try { return client.search(request); } finally { if (previous != null) { OpenSearchQueryManager.setCancellableTask(previous); } else { OpenSearchQueryManager.clearCancellableTask(); } } } Incomplete Cleanup In `doExecute`, if the work throws an exception after the listener is already completed (via sendResponse), the exception is silently swallowed. While the listener won't be double-completed, the exception itself is lost, making debugging difficult. The catch block at line 56-60 only surfaces exceptions when `completed` is false, but does not log or handle exceptions that occur after completion. try { request.getWork().accept(channel); } catch (Exception e) { // Only surface here if the channel hasn't already reported completion (success or error). if (completed.compareAndSet(false, true)) { listener.onFailure(e); }

github-actions · 2026-07-19T09:04:09Z

PR Code Suggestions ✨

Latest suggestions up to d12593c

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Log swallowed exceptions after response If `work.accept(channel)` throws an exception after the channel has already sent a response, the exception is silently swallowed. This could hide critical errors in the execution path. Consider logging the exception when it cannot be reported to the listener to aid debugging and monitoring. plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportSqlQueryAction.java [54-60] try { request.getWork().accept(channel); } catch (Exception e) { // Only surface here if the channel hasn't already reported completion (success or error). if (completed.compareAndSet(false, true)) { listener.onFailure(e); + } else { + // Log the exception since it occurred after response was already sent + logger.error("Exception occurred after SQL query response was sent", e); } } Suggestion importance[1-10]: 7 __ Why: Valid suggestion to log exceptions that occur after a response has been sent. This improves observability and debugging capabilities without changing behavior, though the impact is moderate since such exceptions should be rare.	Medium
	Indicate truncated query descriptions Truncating the query description without any indication may confuse users reviewing task details. When the query exceeds `MAX_DESCRIPTION_LENGTH`, append an ellipsis or truncation marker to clearly indicate the description is incomplete. plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportSqlQueryRequest.java [65-71] @Override public String getDescription() { if (query.length() > MAX_DESCRIPTION_LENGTH) { - return query.substring(0, MAX_DESCRIPTION_LENGTH); + return query.substring(0, MAX_DESCRIPTION_LENGTH) + "..."; } return query; } Suggestion importance[1-10]: 5 __ Why: Adding an ellipsis to truncated descriptions improves user experience by making truncation explicit. However, this is a minor enhancement with limited impact on functionality.	Low
	Add explicit exception handling The `searchWithTask` method does not handle exceptions thrown by `client.search(request)`. If an exception occurs, the finally block will still execute, but the exception will propagate without proper cleanup context. Consider wrapping the search call in a try-catch to ensure proper error handling while maintaining task cleanup. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java [127-143] private OpenSearchResponse searchWithTask( OpenSearchRequest request, @Nullable CancellableTask task) { if (task == null) { return client.search(request); } CancellableTask previous = OpenSearchQueryManager.getCancellableTask(); OpenSearchQueryManager.setCancellableTask(task); try { return client.search(request); + } catch (Exception e) { + // Ensure task cleanup happens before re-throwing + throw e; } finally { if (previous != null) { OpenSearchQueryManager.setCancellableTask(previous); } else { OpenSearchQueryManager.clearCancellableTask(); } } } Suggestion importance[1-10]: 2 __ Why: The suggestion adds a redundant catch block that immediately re-throws the exception. The `finally` block already ensures task cleanup happens regardless of exceptions, making this change unnecessary and adding no value.	Low

Previous suggestions

Suggestions up to commit 06af171

Category	Suggestion	Impact
Possible issue	Clear ThreadLocal after task execution The `CancellableTask` is set in the ThreadLocal but never cleared after execution completes. This can cause task references to leak across different requests that reuse the same thread, potentially linking unrelated queries. Add cleanup in a finally block to ensure the ThreadLocal is cleared. plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportSqlQueryAction.java [43-61] @Override protected void doExecute( Task task, TransportSqlQueryRequest request, ActionListener<TransportSqlQueryResponse> listener) { if (task instanceof CancellableTask cancellableTask) { OpenSearchQueryManager.setCancellableTask(cancellableTask); } AtomicBoolean completed = new AtomicBoolean(false); RestChannel channel = new CompletionSignalingChannel(request.getChannel(), listener, completed); try { request.getWork().accept(channel); } catch (Exception e) { if (completed.compareAndSet(false, true)) { listener.onFailure(e); } + } finally { + OpenSearchQueryManager.clearCancellableTask(); } } Suggestion importance[1-10]: 8 __ Why: This identifies a potential ThreadLocal leak where `CancellableTask` is set but never cleared after execution. Without cleanup, threads from the pool could carry stale task references to subsequent requests, causing incorrect parent-task linkage. Adding a finally block to clear the ThreadLocal is critical for correctness.	Medium
General	Set task before try block The task restoration logic in the finally block should always restore the previous state, regardless of whether it was null. The current implementation only clears when previous is null, but should use a single restoration path to ensure consistency and prevent potential ThreadLocal leaks in edge cases. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/BackgroundSearchScanner.java [127-143] private OpenSearchResponse searchWithTask( OpenSearchRequest request, @Nullable CancellableTask task) { if (task == null) { return client.search(request); } CancellableTask previous = OpenSearchQueryManager.getCancellableTask(); - OpenSearchQueryManager.setCancellableTask(task); try { + OpenSearchQueryManager.setCancellableTask(task); return client.search(request); } finally { if (previous != null) { OpenSearchQueryManager.setCancellableTask(previous); } else { OpenSearchQueryManager.clearCancellableTask(); } } } Suggestion importance[1-10]: 2 __ Why: The suggestion proposes moving `setCancellableTask` inside the try block, but the 'improved_code' is functionally identical to the 'existing_code' - both set the task before the try block. The current implementation correctly handles task restoration in the finally block, so this suggestion offers no actual improvement.	Low

Suggestions up to commit bd9913a

Category	Suggestion	Impact
Possible issue	Clear ThreadLocal after task execution The `CancellableTask` set in `OpenSearchQueryManager` is never cleared after execution completes. This could cause the ThreadLocal to retain stale task references across subsequent requests on the same thread, leading to incorrect parent task linkage. Add cleanup in a `finally` block to ensure the ThreadLocal is cleared. plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportSqlQueryAction.java [51-60] AtomicBoolean completed = new AtomicBoolean(false); RestChannel channel = new CompletionSignalingChannel(request.getChannel(), listener, completed); try { request.getWork().accept(channel); } catch (Exception e) { if (completed.compareAndSet(false, true)) { listener.onFailure(e); } +} finally { + OpenSearchQueryManager.clearCancellableTask(); } Suggestion importance[1-10]: 9 __ Why: Critical bug fix. The `CancellableTask` set in `OpenSearchQueryManager.setCancellableTask()` at line 48 is never cleared, causing ThreadLocal pollution that could lead to incorrect parent task linkage in subsequent requests on pooled threads.	High
General	Indicate query truncation in description Truncating the query at exactly `MAX_DESCRIPTION_LENGTH` characters could split multi-byte UTF-8 characters, potentially causing encoding issues or exceptions. Use a safer substring method that respects character boundaries, or add an ellipsis indicator to show truncation occurred. plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportSqlQueryRequest.java [65-71] @Override public String getDescription() { if (query.length() > MAX_DESCRIPTION_LENGTH) { - return query.substring(0, MAX_DESCRIPTION_LENGTH); + return query.substring(0, MAX_DESCRIPTION_LENGTH) + "..."; } return query; } Suggestion importance[1-10]: 4 __ Why: Minor improvement. Adding an ellipsis indicator makes it clearer that the query was truncated. However, the concern about UTF-8 character splitting is less critical since Java's `substring()` operates on UTF-16 code units and won't corrupt the string.	Low

Suggestions up to commit c50c128

Category	Suggestion	Impact
Possible issue	Prevent duplicate header exception Guard each `putHeader` call with a null-check on `getHeader` to prevent `IllegalArgumentException` if the header already exists. OpenSearch's `ThreadContext.putHeader` throws when a key is already present, which can occur in retry or plugin-chaining scenarios. legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java [158-166] -client - .threadPool() - .getThreadContext() - .putHeader(QuerySourceHeaders.QUERY_SOURCE_HEADER, "sql"); -client - .threadPool() - .getThreadContext() - .putHeader( - QuerySourceHeaders.QUERY_EXECUTION_ID_HEADER, java.util.UUID.randomUUID().toString()); +if (client.threadPool().getThreadContext().getHeader(QuerySourceHeaders.QUERY_SOURCE_HEADER) == null) { + client.threadPool().getThreadContext().putHeader(QuerySourceHeaders.QUERY_SOURCE_HEADER, "sql"); +} +if (client.threadPool().getThreadContext().getHeader(QuerySourceHeaders.QUERY_EXECUTION_ID_HEADER) == null) { + client.threadPool().getThreadContext().putHeader( + QuerySourceHeaders.QUERY_EXECUTION_ID_HEADER, java.util.UUID.randomUUID().toString()); +} Suggestion importance[1-10]: 8 __ Why: This addresses a real issue where `putHeader` throws `IllegalArgumentException` if the header already exists. The PPL path (lines 188-202 in `TransportPPLQueryAction.java`) correctly guards with null-checks, but the SQL path does not. This inconsistency could cause failures in retry or plugin-chaining scenarios, making it a significant correctness issue.	Medium
General	Replace deprecated Thread.getId() call Replace deprecated `Thread.currentThread().getId()` with `Thread.currentThread().threadId()`. The `getId()` method has been deprecated since Java 19, and the project targets Java 21. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [132-135] activeMemStart = SUN_THREAD_MX != null - ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().getId()) + ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) : 0; Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies the use of deprecated `Thread.currentThread().getId()` (deprecated since Java 19) and proposes replacing it with `threadId()`. Since the project targets Java 21, this is a valid modernization that removes deprecation warnings. However, it's not a critical bug—just a code quality improvement.	Medium
General	Prevent ThreadLocal leak on exceptions Add a `finally` block that calls `QueryPhaseTracker.clear()` to prevent ThreadLocal and Log4j key leaks when exceptions occur before `endAll()` or `writePhaseHeader()` are reached. This ensures cleanup on all exit paths. core/src/main/java/org/opensearch/sql/executor/QueryService.java [204-243] try { QueryPhaseTracker tracker = QueryPhaseTracker.startOrRestore(); tracker.beginPhase("analyze"); ProfileContext profileContext = QueryProfiling.activate(QueryContext.isProfileEnabled()); ... } catch (Throwable t) { + ... +} finally { + QueryPhaseTracker.clear(); +} Suggestion importance[1-10]: 7 __ Why: The suggestion identifies a resource leak where `QueryPhaseTracker` remains in ThreadLocal if an exception occurs before `endAll()` or `writePhaseHeader()`. Adding a `finally` block with `clear()` would ensure cleanup on all exit paths. However, the placement shown may conflict with existing exception handling logic, and the actual implementation would need careful integration with the existing `catch` block at line 243.	Medium

Suggestions up to commit dcd396f

Category	Suggestion	Impact
Possible issue	Handle potential UnsupportedOperationException The `getThreadAllocatedBytes()` method can throw `UnsupportedOperationException` if thread memory allocation measurement is not enabled or supported. This uncaught exception could crash the query execution. Wrap the call in a try-catch block to handle this gracefully and default to 0 on failure. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [132-135] -activeMemStart = - SUN_THREAD_MX != null - ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) - : 0; +activeMemStart = 0; +if (SUN_THREAD_MX != null) { + try { + activeMemStart = SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()); + } catch (UnsupportedOperationException e) { + // Memory allocation tracking not supported + } +} Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that `getThreadAllocatedBytes()` can throw `UnsupportedOperationException` at runtime. Adding error handling here would prevent query execution failures and aligns with the best-effort nature of memory tracking mentioned in the class documentation.	Medium
Possible issue	Protect against memory tracking failure The `getThreadAllocatedBytes()` call in `endCurrentPhase()` can throw `UnsupportedOperationException` if memory tracking becomes unavailable during execution. This would terminate the query unexpectedly. Wrap this call in a try-catch block to ensure phase tracking continues even if memory measurement fails. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [146-151] if (SUN_THREAD_MX != null) { - long memElapsed = - SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) - - activeMemStart; - memPhases.merge(activePhase, memElapsed, Long::sum); + try { + long memElapsed = + SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) + - activeMemStart; + memPhases.merge(activePhase, memElapsed, Long::sum); + } catch (UnsupportedOperationException e) { + // Memory tracking became unavailable + } } Suggestion importance[1-10]: 7 __ Why: Similar to suggestion 2, this correctly identifies that `getThreadAllocatedBytes()` can throw `UnsupportedOperationException` during `endCurrentPhase()`. Adding error handling ensures phase tracking continues even if memory measurement fails, which is consistent with the best-effort approach.	Medium
General	Avoid catching Error types The `getSunThreadMXBean()` method catches `NoClassDefFoundError` which is an `Error`, not an `Exception`. Catching `Error` types can mask serious JVM issues and is generally discouraged. Consider removing `NoClassDefFoundError` from the catch clause and only catch `UnsupportedOperationException`, or handle the class availability check differently using reflection or class loading checks. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [44-53] private static com.sun.management.ThreadMXBean getSunThreadMXBean() { try { if (THREAD_MX instanceof com.sun.management.ThreadMXBean sun) { return sun; } - } catch (NoClassDefFoundError \| UnsupportedOperationException e) { + } catch (UnsupportedOperationException e) { // com.sun.management not available on this JVM + } catch (Exception e) { + // Unexpected exception during cast check } return null; } Suggestion importance[1-10]: 4 __ Why: While catching `NoClassDefFoundError` is generally discouraged, in this specific case it's used for graceful degradation when `com.sun.management` classes are unavailable on non-HotSpot JVMs. The suggestion to catch `Exception` instead is incorrect since `NoClassDefFoundError` is not an `Exception`. However, the concern about catching `Error` types is valid for general best practices.	Low
General	Add bounds check before substring The `substring()` operation can throw `StringIndexOutOfBoundsException` if `MAX_ORIGINAL_QUERY_LENGTH` is negative or exceeds the string length. Although unlikely with the current constant value, add a bounds check to ensure `MAX_ORIGINAL_QUERY_LENGTH` is positive and does not exceed `queryText.length()` before truncating. legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java [167-174] String queryText = sqlRequest.getSql(); if (queryText != null && threadContext.getHeader(QuerySourceHeaders.ORIGINAL_QUERY_HEADER) == null) { - if (queryText.length() > QuerySourceHeaders.MAX_ORIGINAL_QUERY_LENGTH) { + if (queryText.length() > QuerySourceHeaders.MAX_ORIGINAL_QUERY_LENGTH + && QuerySourceHeaders.MAX_ORIGINAL_QUERY_LENGTH > 0) { queryText = queryText.substring(0, QuerySourceHeaders.MAX_ORIGINAL_QUERY_LENGTH); } threadContext.putHeader(QuerySourceHeaders.ORIGINAL_QUERY_HEADER, queryText); } Suggestion importance[1-10]: 3 __ Why: The suggestion adds a redundant check since the condition `queryText.length() > QuerySourceHeaders.MAX_ORIGINAL_QUERY_LENGTH` already ensures the substring operation is safe. The additional check for `MAX_ORIGINAL_QUERY_LENGTH > 0` provides minimal value given the constant is defined as 4096.	Low

Suggestions up to commit c50c128

Category	Suggestion	Impact
Possible issue	Guard putHeader to prevent exceptions Guard each `putHeader` call with a null-check on `getHeader` to prevent `IllegalArgumentException` if the header already exists. OpenSearch's `ThreadContext.putHeader` throws when the key is present, which can occur in retry or plugin-chaining scenarios. legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java [158-166] -client - .threadPool() - .getThreadContext() - .putHeader(QuerySourceHeaders.QUERY_SOURCE_HEADER, "sql"); -client - .threadPool() - .getThreadContext() - .putHeader( - QuerySourceHeaders.QUERY_EXECUTION_ID_HEADER, java.util.UUID.randomUUID().toString()); +if (client.threadPool().getThreadContext().getHeader(QuerySourceHeaders.QUERY_SOURCE_HEADER) == null) { + client.threadPool().getThreadContext().putHeader(QuerySourceHeaders.QUERY_SOURCE_HEADER, "sql"); +} +if (client.threadPool().getThreadContext().getHeader(QuerySourceHeaders.QUERY_EXECUTION_ID_HEADER) == null) { + client.threadPool().getThreadContext().putHeader( + QuerySourceHeaders.QUERY_EXECUTION_ID_HEADER, java.util.UUID.randomUUID().toString()); +} Suggestion importance[1-10]: 8 __ Why: Valid concern about `putHeader` throwing `IllegalArgumentException` if the header already exists. The PPL path correctly guards with null-checks (lines 188-194), while the SQL path does not. This inconsistency could cause failures in retry or plugin-chaining scenarios.	Medium
General	Replace deprecated Thread.getId() call Replace deprecated `Thread.currentThread().getId()` with `Thread.currentThread().threadId()`. The `getId()` method is deprecated since Java 19, and the project targets Java 21. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [132-135] activeMemStart = SUN_THREAD_MX != null - ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().getId()) + ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) : 0; Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that `Thread.currentThread().getId()` is deprecated since Java 19 and should be replaced with `threadId()` for Java 21 compatibility. However, this is a deprecation warning rather than a critical bug, so the impact is moderate.	Medium
General	Prevent ThreadLocal leak on exceptions Add a `finally` block that calls `QueryPhaseTracker.clear()` to prevent ThreadLocal and Log4j key leaks when exceptions occur before `endAll()` or `writePhaseHeader()` is reached. This ensures cleanup on all exit paths. core/src/main/java/org/opensearch/sql/executor/QueryService.java [204-243] try { QueryPhaseTracker tracker = QueryPhaseTracker.startOrRestore(); tracker.beginPhase("analyze"); ... } catch (Throwable t) { + ... +} finally { + QueryPhaseTracker.clear(); +} Suggestion importance[1-10]: 7 __ Why: The suggestion identifies a potential resource leak where `QueryPhaseTracker` may not be cleaned up if exceptions occur before `endAll()` is called. However, the `improved_code` shows a `finally` block that would unconditionally clear the tracker, which might interfere with the intended cross-thread propagation via `persist()`. A more nuanced approach is needed.	Medium

github-actions · 2026-07-19T09:08:11Z

Persistent review updated to latest commit 8c958a8

github-actions · 2026-07-19T09:32:37Z

Persistent review updated to latest commit 1e86aa4

github-actions · 2026-07-19T17:47:39Z

Persistent review updated to latest commit 70cb9db

github-actions · 2026-07-23T07:52:55Z

Persistent review updated to latest commit a38aeb9

github-actions · 2026-07-23T08:27:46Z

Persistent review updated to latest commit c3e375a

github-actions · 2026-07-27T17:51:31Z

Persistent review updated to latest commit 134b552

ansjcy

I need to take a closer look into the core logistics but please make sure you format this PR before submitting.

ansjcy · 2026-08-05T00:13:22Z

      Optional<Throwable> calciteFailure) {
    try {
-      executePlan(analyze(plan, queryType), PlanContext.emptyPlanContext(), listener);
+      org.opensearch.sql.common.utils.QueryPhaseTracker tracker =


please do proper import, don't use org.opensearch.sql.common.utils.QueryPhaseTracker..

ansjcy · 2026-08-05T00:14:06Z

-        // This happens if Calcite fell back to V2 due to some issue, and then V2 also failed.
-        // Prefer the Calcite error.
-        // https://github.com/opensearch-project/sql/issues/5060
+        // Calcite fell back to V2 which also failed — prefer Calcite error (#5060)


nit: why is this needed?

github-actions · 2026-08-05T00:27:15Z

Persistent review updated to latest commit b100945

github-actions · 2026-08-05T00:32:20Z

Persistent review updated to latest commit 4985f3d

…racking headers (x-query-source, x-original-query, x-query-execution-id, x-query-phases) to SQL and PPL execution paths so query-insights can identify and track SQL/PPL queries separately from DSL queries. - Add QueryPhaseTracker for tracking parse/analyze/plan phases - Tag thread context with SQL/PPL source headers in transport actions - Add writePhaseHeader() to Calcite execution path for PPL queries - Register all tracking headers as task headers for propagation Signed-off-by: Kishore Kumaar Natarajan <kkumaarn@amazon.com>

github-actions · 2026-08-05T00:59:31Z

Persistent review updated to latest commit c50c128

github-actions · 2026-08-05T02:25:22Z

Persistent review updated to latest commit dcd396f

github-actions · 2026-08-05T02:31:09Z

Persistent review updated to latest commit c50c128

dzane17 · 2026-08-11T23:12:29Z

Instead of injecting four new request headers, can you add the parent id to the DSL search tasks created by sql/ppl? Then every normal DSL search task will have a reference back to its originating sql/ppl query. Query Insights can simply look up whatever info it needs, like the original request body.

The gap right now is that not all DSL search tasks carry the parent PPL/SQL id. PPL v2 was instrumented recently in #5628 (applyParentTask in OpenSearchNodeClient). If the same were done for the v3 PPL and SQL paths (SQL also needs a coordinator task created first), it would avoid the new headers, getTaskHeaders() wiring, thread-context propagation, and raw query text on the wire that the header approach requires.

Swiddis

preliminary review, haven't traced through the low-level details of thread marking and such / why it matters whether or not we're on a sun jvm

Swiddis · 2026-08-11T17:21:26Z

        () -> {
          try {
+            QueryPhaseTracker tracker = QueryPhaseTracker.startOrRestore();
+            tracker.beginPhase("analyze");


Is it better to add these phase markers here instead of part of executeStage?

Can possibly rename StageErrorHandler if it's not broad enough to cover all stage-covering monitoring. Same thing with possibly merging the whole queryphasetracker into what we're already using to define these phases.

Swiddis · 2026-08-11T23:19:41Z

did you mean to put this ai review folder in the project root

Swiddis · 2026-08-11T23:20:33Z

    PPLQueryRequest transformedRequest = transportRequest.toPPLQueryRequest();
+
+    // Tag the thread context so query-insights can identify this as a PPL-derived query.
+    org.opensearch.common.util.concurrent.ThreadContext threadContext =


are these headers correctly getting propagated after a complex thread pool handoff?

Swiddis · 2026-08-11T23:37:39Z

Instead of injecting four new request headers, can you add the parent id to the DSL search tasks created by sql/ppl? Then every normal DSL search task will have a reference back to its originating sql/ppl query. Query Insights can simply look up whatever info it needs, like the original request body.

The gap right now is that not all DSL search tasks carry the parent PPL/SQL id. PPL v2 was instrumented recently in #5628 (applyParentTask in OpenSearchNodeClient). If the same were done for the v3 PPL and SQL paths (SQL also needs a coordinator task created first), it would avoid the new headers, getTaskHeaders() wiring, thread-context propagation, and raw query text on the wire that the header approach requires.

That pr is mostly focused on v3, not sure where v2 is from. But other than that, I like this proposal, we should be roughly aligning to the existing stage tracking and thread propagation we already have as part of complex/background thread handoff. Particularly because historically thread propagation is a pain to modify and guard on edge cases, I'd be very in favor of a single component that owns "create an instrumented thread context that runs $LAMBDA and emits the right metrics to the right places."

dzane17 · 2026-08-11T23:52:59Z

My bad, then the parent task approach is even simpler. @penghuo @dai-chen do you have any ideas or concerns?

…rent task Replace the query-source request headers with parent-task linkage so every DSL search task the SQL/PPL engines spawn references its originating query. query-insights can then look up the coordinator task (and its original request) instead of relying on x-query-* headers, avoiding raw query text on the wire, getTaskHeaders() wiring, and thread-context propagation. - SQL: dispatch execution through a local SqlQueryAction/TransportSqlQueryAction that registers a SqlQueryTask coordinator task (via NodeClient.executeLocally), binds it as the cancellable task, and completes on the REST channel response. Wired into RestSqlAction through a SqlCoordinatorTaskDispatcher seam (legacy module cannot depend on the plugin transport action directly). - PPL v3: propagate the coordinator task to the background prefetch threads in BackgroundSearchScanner so applyParentTask stamps the parent on prefetched DSL searches (previously lost on the sql_background_io pool). - Remove QuerySourceHeaders, QueryPhaseTracker, getTaskHeaders(), and all x-query-* header writes from the SQL/PPL execution paths. Note: SQL coordinator-task lifecycle (task-manager registration visibility, no-leak cleanup, and fallback/cursor/explain behavior) still needs live-cluster integration testing. Unit tests pass and all modules compile. Signed-off-by: Kishore Natarajan <kkumaarn@amazon.com>

github-actions · 2026-08-18T08:09:11Z

Persistent review updated to latest commit bd9913a

- SqlQueryTaskTest: child-cancellation flag and cancellation behavior - TransportSqlQueryRequestTest: createTask type, description text + truncation + null handling, work/channel carriage, validate - TransportSqlQueryActionTest: listener completion on response, cancellable task binding during execution, failure before response, no double completion when work throws after response, single completion on repeated sendResponse - SqlCoordinatorTaskDispatcherTest: PASSTHROUGH runs work with the channel Also apply spotless formatting to the coordinator-task sources. Signed-off-by: Kishore Natarajan <kkumaarn@amazon.com>

github-actions · 2026-08-18T09:46:33Z

Persistent review updated to latest commit 06af171

Restore the initiating thread's prior cancellable task (or clear it) in a finally block in TransportSqlQueryAction.doExecute so pooled transport workers don't retain a stale task reference. Safe because async workers capture the task synchronously during work.accept before it returns. Expand TransportSqlQueryActionTest to cover the thread-local clear (on both success and failure) and all CompletionSignalingChannel delegation methods. Signed-off-by: Kishore Natarajan <kkumaarn@amazon.com>

github-actions · 2026-08-18T11:00:38Z

Persistent review updated to latest commit d12593c

KishoreKicha14 requested review from LantaoJin, RyanL1997, Swiddis, acarbonetto, ahkcs, anirudha, dai-chen, joshuali925, mengweieric, noCharger, penghuo, ps48, qianheng-aws, songkant-aws, vamsimanohar, ykmr1224 and yuancu as code owners July 19, 2026 09:01

KishoreKicha14 marked this pull request as draft July 19, 2026 09:02

KishoreKicha14 force-pushed the feat/sql-query-insights-integration branch from 705b115 to 8c958a8 Compare July 19, 2026 09:06

KishoreKicha14 force-pushed the feat/sql-query-insights-integration branch from 8c958a8 to 1e86aa4 Compare July 19, 2026 09:31

KishoreKicha14 force-pushed the feat/sql-query-insights-integration branch from 1e86aa4 to 70cb9db Compare July 19, 2026 17:45

KishoreKicha14 force-pushed the feat/sql-query-insights-integration branch 2 times, most recently from 42927c2 to 78e60a8 Compare July 23, 2026 08:25

KishoreKicha14 force-pushed the feat/sql-query-insights-integration branch 2 times, most recently from a38aeb9 to 42927c2 Compare July 23, 2026 08:46

ansjcy reviewed Aug 5, 2026

View reviewed changes

KishoreKicha14 force-pushed the feat/sql-query-insights-integration branch from 134b552 to b100945 Compare August 5, 2026 00:25

KishoreKicha14 force-pushed the feat/sql-query-insights-integration branch from b100945 to 4985f3d Compare August 5, 2026 00:30

KishoreKicha14 mentioned this pull request Aug 5, 2026

[FEATURE] Integrate SQL/PPL with query-insights plugin for query source tracking #5677

Open

KishoreKicha14 force-pushed the feat/sql-query-insights-integration branch from 4985f3d to c50c128 Compare August 5, 2026 00:57

KishoreKicha14 force-pushed the feat/sql-query-insights-integration branch from c50c128 to dcd396f Compare August 5, 2026 02:23

KishoreKicha14 force-pushed the feat/sql-query-insights-integration branch from dcd396f to c50c128 Compare August 5, 2026 02:29

KishoreKicha14 marked this pull request as ready for review August 11, 2026 16:20

Swiddis reviewed Aug 11, 2026

View reviewed changes

Swiddis added enhancement New feature or request PPL Piped processing language labels Aug 11, 2026

Uh oh!

Conversation

KishoreKicha14 commented Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

Uh oh!

github-actions Bot commented Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Analyzer ❗

Uh oh!

github-actions Bot commented Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit d12593c)

Uh oh!

github-actions Bot commented Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions Bot commented Jul 19, 2026

Uh oh!

github-actions Bot commented Jul 19, 2026

Uh oh!

github-actions Bot commented Jul 19, 2026

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

github-actions Bot commented Jul 27, 2026

Uh oh!

ansjcy left a comment

Choose a reason for hiding this comment

Uh oh!

ansjcy Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

ansjcy Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 5, 2026

Uh oh!

github-actions Bot commented Aug 5, 2026

Uh oh!

github-actions Bot commented Aug 5, 2026

Uh oh!

github-actions Bot commented Aug 5, 2026

Uh oh!

github-actions Bot commented Aug 5, 2026

Uh oh!

dzane17 commented Aug 11, 2026

Uh oh!

Swiddis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Swiddis Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Swiddis Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

Swiddis Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

Swiddis commented Aug 11, 2026

Uh oh!

dzane17 commented Aug 11, 2026

Uh oh!

github-actions Bot commented Aug 18, 2026

Uh oh!

github-actions Bot commented Aug 18, 2026

Uh oh!

github-actions Bot commented Aug 18, 2026

Uh oh!

Reviewers

KishoreKicha14 commented Jul 19, 2026 •

edited

Loading

github-actions Bot commented Jul 19, 2026 •

edited

Loading

github-actions Bot commented Jul 19, 2026 •

edited

Loading

(Review updated until commit `d12593c`)

github-actions Bot commented Jul 19, 2026 •

edited

Loading

Swiddis left a comment •

edited

Loading

Swiddis Aug 11, 2026 •

edited

Loading