[Feature] Integrate SQL/PPL with query-insights plugin

[KishoreKicha14]

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)

Description

[Describe what this change achieves]

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

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 Code Analyzer ❗

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

Path	Line	Severity	Description
plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java	477	medium	getTaskHeaders() registers x-original-query for propagation to ALL subtasks across the cluster. This causes raw SQL/PPL query text (potentially containing embedded credentials, PII, or sensitive filter values) to be forwarded to every task spawned from the originating request, expanding the blast radius of inadvertent query-content exposure.
legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java	168	medium	The original SQL query text is stored verbatim (up to 4096 chars) in the x-original-query thread-context header. Queries can embed credentials or PII (e.g., WHERE password='secret'). Storing query text in a propagated header makes it readable by any downstream component that inspects thread context, and it will appear in diagnostic output collected by query-insights.

The table above displays the top 10 most important findings.

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

---

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]

PR Reviewer Guide 🔍

(Review updated until commit c3e375a)

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 The startOrRestore() method reads from Log4j ThreadContext and writes to the ThreadLocal CURRENT without synchronization. If multiple threads call this concurrently (e.g., during thread pool initialization or cross-thread handoff), one thread's tracker could be overwritten by another's before persist() is called, causing phase data loss or corruption. public static QueryPhaseTracker startOrRestore() { QueryPhaseTracker tracker = new QueryPhaseTracker(); String stored = org.apache.logging.log4j.ThreadContext.get(LOG4J_KEY); if (stored != null && !stored.isEmpty()) { try { for (String part : stored.split(",")) { // Format: "phase:wallNanos|cpu:cpuNanos|mem:memBytes" String[] segments = part.split("\\|"); String[] kv = segments[0].split(":", 2); if (kv.length == 2) { tracker.phases.put(kv[0], Long.parseLong(kv[1])); for (int i = 1; i < segments.length; i++) { String[] metric = segments[i].split(":", 2); if (metric.length == 2 && "cpu".equals(metric[0])) { tracker.cpuPhases.put(kv[0], Long.parseLong(metric[1])); } else if (metric.length == 2 && "mem".equals(metric[0])) { tracker.memPhases.put(kv[0], Long.parseLong(metric[1])); } } } } } catch (NumberFormatException e) { // Malformed data in ThreadContext — start fresh tracker.phases.clear(); tracker.cpuPhases.clear(); tracker.memPhases.clear(); } } CURRENT.set(tracker); return tracker; } Memory Leak If an exception occurs after start() or startOrRestore() but before clear() is called, the ThreadLocal CURRENT retains a reference to the tracker. In thread pool environments, this causes a memory leak as threads are reused without cleanup. The code does not guarantee clear() is called in a finally block or via try-with-resources. public static QueryPhaseTracker start() { QueryPhaseTracker tracker = new QueryPhaseTracker(); CURRENT.set(tracker); return tracker; } Swallowed Exception The catch block in writePhaseHeader() silently swallows all exceptions without logging. If header writing fails due to a bug (e.g., NPE, serialization error), there is no diagnostic trace, making production issues difficult to debug. At minimum, log the exception at WARN or DEBUG level. } catch (Exception e) { // Best-effort — don't fail the query if phase header can't be written } Duplicate Headers The code unconditionally calls putHeader() for QUERY_SOURCE_HEADER and QUERY_EXECUTION_ID_HEADER without checking if they already exist. If the same request is retried or processed multiple times (e.g., due to internal routing), this will throw an IllegalArgumentException because OpenSearch ThreadContext does not allow overwriting existing headers. client .threadPool() .getThreadContext() .putHeader(QuerySourceHeaders.QUERY_SOURCE_HEADER, "sql"); client .threadPool() .getThreadContext() .putHeader( QuerySourceHeaders.QUERY_EXECUTION_ID_HEADER, java.util.UUID.randomUUID().toString());

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to c3e375a

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Use Java 11-compatible thread ID method The Thread.currentThread().threadId() method was introduced in Java 19. Using it will cause compilation or runtime failures on Java 11/17 environments. Replace with Thread.currentThread().getId() which is available since Java 1.5 and returns the same thread identifier. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [132-135] public void beginPhase(String name) { endCurrentPhase(); activePhase = name; activeStart = System.nanoTime(); activeCpuStart = CPU_SUPPORTED ? THREAD_MX.getCurrentThreadCpuTime() : 0; activeMemStart = SUN_THREAD_MX != null - ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) + ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().getId()) : 0; } Suggestion importance[1-10]: 10 __ Why: Critical compatibility issue. Thread.currentThread().threadId() requires Java 19+, but OpenSearch supports Java 11/17. This will cause compilation or runtime failures. Must use getId() instead.	High
	Fix Java version compatibility issue The Thread.currentThread().threadId() method requires Java 19+. Replace with Thread.currentThread().getId() to maintain compatibility with Java 11/17 environments that OpenSearch supports. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [147-149] public void endCurrentPhase() { if (activePhase != null) { long elapsed = System.nanoTime() - activeStart; phases.merge(activePhase, elapsed, Long::sum); if (CPU_SUPPORTED) { long cpuElapsed = THREAD_MX.getCurrentThreadCpuTime() - activeCpuStart; cpuPhases.merge(activePhase, cpuElapsed, Long::sum); } if (SUN_THREAD_MX != null) { long memElapsed = - SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) + SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().getId()) - activeMemStart; memPhases.merge(activePhase, memElapsed, Long::sum); } activePhase = null; } } Suggestion importance[1-10]: 10 __ Why: Same critical compatibility issue as suggestion 1. threadId() requires Java 19+ but the project targets Java 11/17. This will break compilation or runtime execution.	High
General	Prevent incorrect total time calculation If overallStart is never initialized (e.g., when restoring from ThreadContext), overallStart will be 0, causing total to be an incorrect large value. Initialize overallStart in startOrRestore() to System.nanoTime() if no phases were restored, or skip adding total if overallStart is 0. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [156-160] public void endAll() { endCurrentPhase(); - long total = System.nanoTime() - overallStart; - phases.put("total", total); + if (overallStart > 0) { + long total = System.nanoTime() - overallStart; + phases.put("total", total); + } } Suggestion importance[1-10]: 8 __ Why: Valid concern. When startOrRestore() is called, overallStart remains 0 if phases are restored from ThreadContext, causing incorrect total calculation. The guard prevents this edge case.	Medium

---

Previous suggestions

Suggestions up to commit a38aeb9

Category	Suggestion	Impact
Possible issue	Handle nullable type wrappers correctly The method does not handle nullable type wrappers correctly. When comparing types, Calcite may wrap types in nullable variants, causing identical types with different nullability to be treated as mismatches. Consider using SqlTypeUtil.equalSansNullability or unwrapping nullable types before comparison to ensure consistent matching behavior. core/src/main/java/org/opensearch/sql/expression/function/PPLTypeChecker.java [561-571] private static boolean typesMatch(RelDataType expected, RelDataType actual) { - if (expected instanceof AbstractExprRelDataType<?> expUdt - && actual instanceof AbstractExprRelDataType<?> actUdt) { + RelDataType expBase = expected; + RelDataType actBase = actual; + if (expBase instanceof AbstractExprRelDataType<?> expUdt + && actBase instanceof AbstractExprRelDataType<?> actUdt) { return expUdt.getUdt() == actUdt.getUdt(); } - if (expected instanceof AbstractExprRelDataType<?> - || actual instanceof AbstractExprRelDataType<?>) { + if (expBase instanceof AbstractExprRelDataType<?> + || actBase instanceof AbstractExprRelDataType<?>) { return false; } - return expected.getSqlTypeName() == actual.getSqlTypeName(); + return SqlTypeUtil.equalSansNullability( + OpenSearchTypeFactory.TYPE_FACTORY, expBase, actBase); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that typesMatch may not handle nullable type wrappers consistently. Using SqlTypeUtil.equalSansNullability would provide more robust type comparison, though the current implementation may work for the specific use case. The impact is moderate as it could affect type matching in edge cases.	Medium
General	Validate column names before building projections The duplicate column name check occurs after all projections are built, which means the RelBuilder state is already modified. If a collision is detected, the builder is left in an inconsistent state. Move the validation before building projections to fail fast and avoid partial state corruption. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [4158-4168] Set<String> seenNames = new HashSet<>(); -for (String name : reorderNames) { +for (int i = 0; i < reorderNames.size(); i++) { + String name = reorderNames.get(i); if (!seenNames.add(name)) { throw new IllegalArgumentException( "xyseries produced duplicate output column name '" + name + "'. Use a format template containing both $AGG$ and $VAL$ so column names" + " are unique."); } } +List<RexNode> reorderProjections = new ArrayList<>(); +reorderProjections.add(b.field(xFieldName)); +for (String aggName : yDataFieldNames) { + for (String pivotVal : pivotValues) { + String pivotColName = pivotVal + "_" + aggName; + try { + reorderProjections.add(b.field(pivotColName)); + } catch (IllegalArgumentException e) { + throw new IllegalStateException( + "xyseries: expected pivot output column '" + pivotColName + "' not found", e); + } + } +} Suggestion importance[1-10]: 6 __ Why: The suggestion to validate column names before building projections is reasonable to avoid partial state corruption. However, the current implementation is acceptable since the exception is thrown before the final project call, and the impact is limited to error handling clarity rather than correctness.	Low
	Narrow exception handling scope The method silently catches all exceptions when checking for com.sun.management.ThreadMXBean availability, which can hide unexpected errors like ClassCastException or NoClassDefFoundError. Narrow the catch block to only handle expected exceptions (e.g., ClassNotFoundException) to avoid masking genuine issues during initialization. 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 (Exception e) { - // not available + } catch (ClassNotFoundException | NoClassDefFoundError e) { + // not available on this JVM } return null; } Suggestion importance[1-10]: 5 __ Why: The suggestion to narrow exception handling is a good practice to avoid masking unexpected errors. However, the current broad catch is intentional for best-effort initialization, and the impact is low since the method returns null on any failure, which is handled gracefully by callers.	Low

Suggestions up to commit 70cb9db

Category	Suggestion	Impact
Possible issue	Handle malformed serialized phase data The Long.parseLong() calls can throw NumberFormatException if the serialized data is corrupted or malformed. Wrap parsing logic in a try-catch block to prevent deserialization failures from crashing query execution. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [66-81] for (String part : stored.split(",")) { - // Format: "phase:wallNanos|cpu:cpuNanos|mem:memBytes" - String[] segments = part.split("\\|"); - String[] kv = segments[0].split(":", 2); - if (kv.length == 2) { - tracker.phases.put(kv[0], Long.parseLong(kv[1])); - for (int i = 1; i < segments.length; i++) { - String[] metric = segments[i].split(":", 2); - if (metric.length == 2 && "cpu".equals(metric[0])) { - tracker.cpuPhases.put(kv[0], Long.parseLong(metric[1])); - } else if (metric.length == 2 && "mem".equals(metric[0])) { - tracker.memPhases.put(kv[0], Long.parseLong(metric[1])); + try { + String[] segments = part.split("\\|"); + String[] kv = segments[0].split(":", 2); + if (kv.length == 2) { + tracker.phases.put(kv[0], Long.parseLong(kv[1])); + for (int i = 1; i < segments.length; i++) { + String[] metric = segments[i].split(":", 2); + if (metric.length == 2 && "cpu".equals(metric[0])) { + tracker.cpuPhases.put(kv[0], Long.parseLong(metric[1])); + } else if (metric.length == 2 && "mem".equals(metric[0])) { + tracker.memPhases.put(kv[0], Long.parseLong(metric[1])); + } } } + } catch (NumberFormatException e) { + // Skip malformed phase data } } Suggestion importance[1-10]: 8 __ Why: The suggestion addresses a real risk where corrupted or malformed serialized data could cause NumberFormatException, potentially crashing query execution. Adding exception handling ensures the system can gracefully skip invalid data and continue processing.	Medium
	Handle unsupported memory allocation measurement The getThreadAllocatedBytes() method can throw UnsupportedOperationException if thread memory allocation measurement is not supported. Wrap the call in a try-catch block to prevent runtime exceptions and gracefully handle unsupported platforms. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [111-120] public void beginPhase(String name) { endCurrentPhase(); activePhase = name; activeStart = System.nanoTime(); activeCpuStart = CPU_SUPPORTED ? THREAD_MX.getCurrentThreadCpuTime() : 0; - activeMemStart = - SUN_THREAD_MX != null - ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) - : 0; + try { + activeMemStart = + SUN_THREAD_MX != null + ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) + : 0; + } catch (UnsupportedOperationException e) { + activeMemStart = 0; + } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that getThreadAllocatedBytes() can throw UnsupportedOperationException. Adding exception handling prevents runtime failures on platforms where thread memory allocation measurement is not supported, improving robustness.	Medium
	Prevent exception in memory tracking The getThreadAllocatedBytes() call in endCurrentPhase() can throw UnsupportedOperationException on platforms where thread memory allocation measurement is unavailable. Add exception handling to prevent phase tracking failures. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [130-135] 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 allocation tracking not supported on this platform + } } Suggestion importance[1-10]: 7 __ Why: Similar to suggestion 1, this correctly identifies the same potential UnsupportedOperationException in endCurrentPhase(). The exception handling prevents phase tracking failures and ensures graceful degradation when memory tracking is unavailable.	Medium

Suggestions up to commit 1e86aa4

Category	Suggestion	Impact
Possible issue	Handle unsupported memory allocation measurement The getThreadAllocatedBytes() method can throw UnsupportedOperationException if thread memory allocation measurement is not enabled. Wrap the call in a try-catch block to handle this gracefully and prevent query execution failures. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [111-120] public void beginPhase(String name) { endCurrentPhase(); activePhase = name; activeStart = System.nanoTime(); activeCpuStart = CPU_SUPPORTED ? THREAD_MX.getCurrentThreadCpuTime() : 0; - activeMemStart = - SUN_THREAD_MX != null - ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) - : 0; + try { + activeMemStart = + SUN_THREAD_MX != null + ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) + : 0; + } catch (UnsupportedOperationException e) { + activeMemStart = 0; + } } Suggestion importance[1-10]: 7 __ Why: Valid suggestion addressing a potential runtime exception. The getThreadAllocatedBytes() method can throw UnsupportedOperationException if thread memory allocation measurement is not enabled, which could break query execution. Adding exception handling improves robustness.	Medium
	Handle unsupported memory tracking exception Similar to beginPhase(), the getThreadAllocatedBytes() call in endCurrentPhase() can throw UnsupportedOperationException. Wrap this call in a try-catch block to ensure phase tracking doesn't break query execution. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [130-135] 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 allocation tracking not supported + } } Suggestion importance[1-10]: 7 __ Why: Valid suggestion similar to suggestion 2. The getThreadAllocatedBytes() call in endCurrentPhase() can also throw UnsupportedOperationException, and wrapping it in a try-catch block prevents potential query execution failures while maintaining graceful degradation of the tracking feature.	Medium
	Add null check before instanceof The method catches all exceptions but only handles the case where the cast fails. If THREAD_MX is null, this will throw a NullPointerException before the instanceof check. Add a null check before the instanceof operation to prevent potential crashes. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [40-49] private static com.sun.management.ThreadMXBean getSunThreadMXBean() { try { - if (THREAD_MX instanceof com.sun.management.ThreadMXBean sun) { + if (THREAD_MX != null && THREAD_MX instanceof com.sun.management.ThreadMXBean sun) { return sun; } } catch (Exception e) { // not available } return null; } Suggestion importance[1-10]: 2 __ Why: The suggestion is technically incorrect. THREAD_MX is initialized as a static final field from ManagementFactory.getThreadMXBean(), which never returns null according to the JDK specification. The instanceof check already handles the case safely without needing an explicit null check.	Low

Suggestions up to commit 8c958a8

Category	Suggestion	Impact
Possible issue	Use Java 8 compatible thread ID The threadId() method was introduced in Java 19. For compatibility with earlier Java versions, use Thread.currentThread().getId() instead to avoid runtime errors on older JVMs. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [116-118] activeMemStart = SUN_THREAD_MX != null - ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) + ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().getId()) : 0; Suggestion importance[1-10]: 9 __ Why: The threadId() method is only available in Java 19+, while OpenSearch typically supports Java 8/11. Using getId() ensures compatibility with older JVMs and prevents runtime errors. This is a critical compatibility issue.	High
General	Validate non-negative memory allocation delta Memory allocation can decrease during execution, resulting in negative memElapsed values. Add validation to ensure only non-negative memory deltas are recorded to prevent misleading metrics. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [129-133] if (SUN_THREAD_MX != null) { long memElapsed = SUN_THREAD_MX.getThreadAllocatedBytes( - Thread.currentThread().threadId()) - activeMemStart; - memPhases.merge(activePhase, memElapsed, Long::sum); + Thread.currentThread().getId()) - activeMemStart; + if (memElapsed > 0) { + memPhases.merge(activePhase, memElapsed, Long::sum); + } } Suggestion importance[1-10]: 6 __ Why: While memory allocation can theoretically decrease (e.g., due to GC or measurement artifacts), adding validation prevents recording negative values that could skew metrics. The suggestion also correctly applies the getId() fix from suggestion 1.	Low

Suggestions up to commit 705b115

Category	Suggestion	Impact
General	Handle malformed phase data gracefully The Long.parseLong() calls can throw NumberFormatException if the stored data is corrupted or malformed. Wrap the parsing logic in a try-catch block to prevent crashes and ensure the tracker continues functioning even with invalid data. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [62-85] public static QueryPhaseTracker startOrRestore() { QueryPhaseTracker tracker = new QueryPhaseTracker(); String stored = org.apache.logging.log4j.ThreadContext.get(LOG4J_KEY); if (stored != null && !stored.isEmpty()) { - for (String part : stored.split(",")) { - // Format: "phase:wallNanos|cpu:cpuNanos|mem:memBytes" - String[] segments = part.split("\\|"); - String[] kv = segments[0].split(":", 2); - if (kv.length == 2) { - tracker.phases.put(kv[0], Long.parseLong(kv[1])); - for (int i = 1; i < segments.length; i++) { - String[] metric = segments[i].split(":", 2); - if (metric.length == 2 && "cpu".equals(metric[0])) { - tracker.cpuPhases.put(kv[0], Long.parseLong(metric[1])); - } else if (metric.length == 2 && "mem".equals(metric[0])) { - tracker.memPhases.put(kv[0], Long.parseLong(metric[1])); + try { + for (String part : stored.split(",")) { + // Format: "phase:wallNanos|cpu:cpuNanos|mem:memBytes" + String[] segments = part.split("\\|"); + String[] kv = segments[0].split(":", 2); + if (kv.length == 2) { + tracker.phases.put(kv[0], Long.parseLong(kv[1])); + for (int i = 1; i < segments.length; i++) { + String[] metric = segments[i].split(":", 2); + if (metric.length == 2 && "cpu".equals(metric[0])) { + tracker.cpuPhases.put(kv[0], Long.parseLong(metric[1])); + } else if (metric.length == 2 && "mem".equals(metric[0])) { + tracker.memPhases.put(kv[0], Long.parseLong(metric[1])); + } } } } + } catch (NumberFormatException e) { + // Corrupted data - continue with empty tracker } } CURRENT.set(tracker); return tracker; } Suggestion importance[1-10]: 7 __ Why: This is a valid suggestion addressing a potential NumberFormatException when parsing stored phase data. The try-catch block prevents crashes from corrupted data, which is important for robustness. However, it might be better to log the exception for debugging purposes rather than silently continuing.	Medium
	Protect against memory measurement failures The getThreadAllocatedBytes() method can throw UnsupportedOperationException if thread memory allocation measurement is not enabled. Wrap this call in a try-catch to prevent runtime exceptions and ensure tracking continues even if memory measurement fails. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [111-119] public void beginPhase(String name) { endCurrentPhase(); activePhase = name; activeStart = System.nanoTime(); activeCpuStart = CPU_SUPPORTED ? THREAD_MX.getCurrentThreadCpuTime() : 0; - activeMemStart = SUN_THREAD_MX != null - ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) - : 0; + try { + activeMemStart = SUN_THREAD_MX != null + ? SUN_THREAD_MX.getThreadAllocatedBytes(Thread.currentThread().threadId()) + : 0; + } catch (UnsupportedOperationException e) { + activeMemStart = 0; + } } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that getThreadAllocatedBytes() can throw UnsupportedOperationException. Adding a try-catch block improves robustness by preventing runtime exceptions. However, the same issue exists in endCurrentPhase() at lines 130-132, which is not addressed by this suggestion.	Low
	Add exception logging for debugging The getSunThreadMXBean() method catches all exceptions but doesn't log them, making debugging difficult if the cast fails unexpectedly. Consider logging the exception at debug level to aid troubleshooting while keeping the fallback behavior intact. common/src/main/java/org/opensearch/sql/common/utils/QueryPhaseTracker.java [40-49] private static com.sun.management.ThreadMXBean getSunThreadMXBean() { try { if (THREAD_MX instanceof com.sun.management.ThreadMXBean sun) { return sun; } } catch (Exception e) { - // not available + // Log at debug level for troubleshooting + org.apache.logging.log4j.LogManager.getLogger(QueryPhaseTracker.class) + .debug("Sun ThreadMXBean not available", e); } return null; } Suggestion importance[1-10]: 3 __ Why: While logging exceptions can aid debugging, this is a minor improvement. The method already has appropriate fallback behavior (returning null), and the exception is expected when com.sun.management.ThreadMXBean is unavailable. The suggestion is valid but offers limited impact.	Low

[github-actions]

Persistent review updated to latest commit 8c958a8

[github-actions]

Persistent review updated to latest commit 1e86aa4

[github-actions]

Persistent review updated to latest commit 70cb9db

[github-actions]

Persistent review updated to latest commit a38aeb9

[github-actions]

Persistent review updated to latest commit c3e375a