Updating analyze endpoint

[Krish-Gandhi]

Description

• Removing query operation tracking and operator_ tree from analyze
• Cleaned up analyze response
• Adding rule-based recommendations to analyze

This PR improves correctness of the analyze endpoint, addressing the operator_tree correctness issue mentioned in #5568. Additionally, this PR enhances analyze by providing rule-based query optimization recommendations in the response.

Example Query and Response

curl -X POST "localhost:9200/_plugins/_ppl" \ 
  -H "Content-Type: application/json" \
  -d '{"query": "source=`test_data` | head 1000 | JOIN left=l right=r on l.api_version = r.api_version [source = `test_data` | head 1000] | eval trip = client_city + r.client_city | sort trip | fields trip | head 2", "analyze": true}'

The response of this will be as follows (logical and physical plans are trimmed for brevity):

{
  "logicalPlan": [...],
  "physicalPlan": [...],
  "profile": {
    "summary": {
      "total_time_ms": 1219.86
    },
    "phases": {
      "analyze": {
        "time_ms": 2.56
      },
      "optimize": {
        "time_ms": 7.05
      },
      "execute": {
        "time_ms": 1210.1
      },
      "format": {
        "time_ms": 0.01
      }
    },
    "plan": {
      "node": "EnumerableLimit",
      "time_ms": 1208.79,
      "rows": 2,
      "children": [
        {
          "node": "CalciteEnumerableTopK",
          "time_ms": 1208.78,
          "rows": 2,
          "children": [
            {
              "node": "EnumerableCalc",
              "time_ms": 1173.21,
              "rows": 1000000,
              "children": [
                {
                  "node": "EnumerableMergeJoin",
                  "time_ms": 1142.42,
                  "rows": 1000000,
                  "children": [
                    {
                      "node": "CalciteEnumerableIndexScan",
                      "time_ms": 1100.8,
                      "rows": 1000
                    },
                    {
                      "node": "CalciteEnumerableIndexScan",
                      "time_ms": 17.64,
                      "rows": 1000
                    }
                  ]
                }
              ]
            }
          ]
        }
      ]
    },
    "thread_pool": "sql-complex-worker"
  },
  "recommendations": [
    {
      "severity": "CRITICAL",
      "rule": "Join Row Explosion",
      "message": "Join expanded 2000 rows into 1000000 rows (500.0×)",
      "affected_node": "EnumerableMergeJoin",
      "suggestion": "Add filters to the subqueries before the join to reduce rows."
    },
    {
      "severity": "INFO",
      "rule": "Bottleneck Stage",
      "message": "CalciteEnumerableIndexScan took 1100.8 ms (91% of execution)",
      "affected_node": "CalciteEnumerableIndexScan"
    }
  ],
  "schema": [
    {
      "name": "trip",
      "type": "STRING"
    }
  ],
  "datarows": [
    [
      "AarontonAaronton"
    ],
    [
      "AarontonAdamsborough"
    ]
  ],
  "total": 2,
  "size": 2
}

Recommendations Implemented

Rule	Severity	Trigger	Configurable Thresholds	Recommendation	Message
Ineffective Filter	WARNING	node.node contains "filter" or "project"; rows_out / rows_in > x	x = 0.95 (INEFFECTIVE_FILTER_MAX_PASS_RATIO)	Consider removing the filter or making it more selective.	Filter only dropped <pct>% of rows
Join Row Explosion	WARNING (ratio > x), CRITICAL (ratio >= z)	node.node contains "join"; rows_out / rows_in > x	x = 5.0 (JOIN_EXPLOSION_RATIO), z = 20.0 (JOIN_EXPLOSION_CRITICAL_RATIO)	Add filters to the subqueries before the join to reduce rows.	Join expanded <rows_in> rows into <rows_out> rows (<ratio>×)
Expensive Sort	WARNING	node.node contains "sort"; duration(node) / profile.phases.execute.time_ms > x and rows_in > y	x = 0.20 (EXPENSIVE_SORT_TIME_FRACTION), y = 50,000 (EXPENSIVE_SORT_MIN_ROWS)	Filter or limit rows before sorting (e.g. add head or a where).	Sorting <rows_in> rows took <duration> ms (<pct>% of execution)
Bottleneck Stage	INFO	argmax(duration(node)) / profile.phases.execute.time_ms > x	x = 0.75 (BOTTLENECK_TIME_FRACTION)	—	<node> took <duration> ms (<pct>% of execution)
Optimize Phase Dominates	INFO	profile.phases.execute.time_ms < profile.phases.optimize.time_ms and profile.phases.optimize.time_ms > x	x = 75 ms (OPTIMIZE_DOMINATES_MIN_MS)	—	Query planning took <optimize> ms vs <execute> ms executing

Related Issues

#5568
#5500
#4343
#5044
#5688

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 7b87b68.

Path	Line	Severity	Description
core/src/main/java/org/opensearch/sql/executor/QueryService.java	339	medium	boolean disableCache is hardcoded to true with the caller-configurable parameter commented out. This permanently suppresses cache hit detection (possibleCacheHit always false) and forces request cache disabled on every analyze call — the feature appears intentionally hobbled pending a decision, but the hardcoded override is anomalous and should be reviewed.
opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java	229	low	A debug log statement `LOG.info([CACHE_DEBUG] ...)` was left commented in production code; not malicious but indicates debugging instrumentation that was not cleaned up before merge.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 1 | 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]

PR Reviewer Guide 🔍

(Review updated until commit d58f214)

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 withCheckedArithmetic call wraps convertToCalcitePlan but the result is assigned to calcitePlan which is never used. The physical plan generation via Hook.PLAN_BEFORE_IMPLEMENTATION still operates on the unwrapped relNode. If withCheckedArithmetic is meant to enforce overflow checks during physical planning (as suggested by the new test analyzeLongArithmeticOverflowReturnsError), this assignment does nothing because the hook captures the plan from a separate execution path that does not see calcitePlan. RelNode calcitePlan = withCheckedArithmetic(convertToCalcitePlan(relNode, context), context);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to d58f214

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Apply checked arithmetic before plan conversion The withCheckedArithmetic call wraps the physical plan conversion but is applied to the logical plan. This may not enable checked arithmetic during physical plan generation (which happens inside convertToCalcitePlan). Consider applying checked arithmetic before or during the conversion to ensure overflow checks are active throughout the planning process. core/src/main/java/org/opensearch/sql/executor/QueryService.java [399-400] -RelNode calcitePlan = - withCheckedArithmetic(convertToCalcitePlan(relNode, context), context); +RelNode logicalWithChecked = withCheckedArithmetic(relNode, context); +RelNode calcitePlan = convertToCalcitePlan(logicalWithChecked, context); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that withCheckedArithmetic is applied after convertToCalcitePlan, which may not enable overflow checks during physical plan generation. However, the test analyzeLongArithmeticOverflowReturnsError passes, indicating the current approach works. The suggestion is valid but may not be critical.	Medium
General	Guard against row count overflow Summing child rows without overflow protection can silently wrap on very large datasets, producing incorrect negative or small values that break ratio-based rules. Use Math.addExact or check for overflow to prevent silent arithmetic errors in row count calculations. core/src/main/java/org/opensearch/sql/executor/analyze/AnalyzeRecommendationBuilder.java [268-277] private static long rowsIn(PlanNode node) { if (node.getChildren() == null || node.getChildren().isEmpty()) { return 0; } long sum = 0; for (PlanNode child : node.getChildren()) { - sum += child.getRows(); + try { + sum = Math.addExact(sum, child.getRows()); + } catch (ArithmeticException e) { + return Long.MAX_VALUE; // clamp to max on overflow + } } return sum; } Suggestion importance[1-10]: 5 __ Why: Adding overflow protection for row count summation is a reasonable defensive measure, though the likelihood of summing child row counts to exceed Long.MAX_VALUE in practice is low. The suggestion improves robustness but addresses an edge case rather than a likely issue.	Low
	Validate timing semantics before subtraction The duration calculation assumes cumulative timing semantics (parent includes children), but if time_ms is already exclusive (self-time only), subtracting the max child would produce incorrect negative or zero values. Verify that PlanNode.getTimeMillis() returns cumulative wall-time as documented, or adjust the logic if the timing is already exclusive. core/src/main/java/org/opensearch/sql/executor/analyze/AnalyzeRecommendationBuilder.java [257-265] private static double duration(PlanNode node) { + // Verify timing semantics: if time_ms is already exclusive, return it directly + if (node.getChildren() == null || node.getChildren().isEmpty()) { + return node.getTimeMillis(); + } double maxChild = 0; - if (node.getChildren() != null) { - for (PlanNode child : node.getChildren()) { - maxChild = Math.max(maxChild, child.getTimeMillis()); - } + for (PlanNode child : node.getChildren()) { + maxChild = Math.max(maxChild, child.getTimeMillis()); } return Math.max(0, node.getTimeMillis() - maxChild); } Suggestion importance[1-10]: 3 __ Why: The suggestion questions whether time_ms is cumulative or exclusive, but the class javadoc explicitly states "a node's time_ms is cumulative wall-time". The existing logic is correct per the documented semantics. The suggestion adds unnecessary complexity without evidence of a bug.	Low

---

Previous suggestions

Suggestions up to commit 7b87b68

Category	Suggestion	Impact
Possible issue	ThreadLocal cleanup missing on early exception The disableRequestCache ThreadLocal is set but never cleaned up if the execution path throws an exception before reaching the finally block. This can leak the ThreadLocal value to subsequent requests on the same thread, causing unintended cache behavior. core/src/main/java/org/opensearch/sql/executor/QueryService.java [345-351] long cacheHitsBefore = disableCache ? -1 : executionEngine.getRequestCacheHitCount(indexNames); -if (disableCache) { - CalcitePlanContext.disableRequestCache.set(true); -} +try { + if (disableCache) { + CalcitePlanContext.disableRequestCache.set(true); + } -AtomicReference<ExecutionEngine.QueryResponse> queryResponseRef = new AtomicReference<>(); + AtomicReference<ExecutionEngine.QueryResponse> queryResponseRef = new AtomicReference<>(); Suggestion importance[1-10]: 9 __ Why: Critical bug: if an exception occurs between setting the ThreadLocal and the try-catch block, the ThreadLocal will leak to subsequent requests on the same thread, causing unintended cache behavior. This is a serious correctness issue that can affect query execution.	High
General	Remove redundant ThreadLocal cleanup The disableRequestCache ThreadLocal is removed twice: once in the catch block and again in the finally block. The explicit removal in the catch block is redundant since the finally block always executes. Remove the duplicate cleanup to simplify the code. core/src/main/java/org/opensearch/sql/executor/QueryService.java [389-395] } catch (InterruptedException e) { Thread.currentThread().interrupt(); - CalcitePlanContext.disableRequestCache.remove(); listener.onFailure(new RuntimeException("Interrupted while waiting for query execution", e)); return; } finally { CalcitePlanContext.disableRequestCache.remove(); } Suggestion importance[1-10]: 5 __ Why: Valid improvement: the disableRequestCache.remove() call in the catch block is redundant since the finally block always executes. Removing it simplifies the code and improves maintainability.	Low
	Remove commented debug log statement The commented-out debug log statement should be removed before merging to production. Debug logs left in the code can clutter the codebase and may accidentally be uncommented later, causing performance issues or exposing sensitive information. opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java [224-231] SearchRequest searchRequest = new SearchRequest().indices(indexName.getIndexNames()).source(this.sourceBuilder); if (disableRequestCache) { searchRequest.requestCache(false); } -// LOG.info("[CACHE_DEBUG] disableRequestCache={}, searchRequest.requestCache()={}", -// disableRequestCache, searchRequest.requestCache()); Suggestion importance[1-10]: 4 __ Why: Minor code quality issue: the commented-out debug log should be removed to keep the codebase clean. While correct, this is a low-impact suggestion that only affects code cleanliness.	Low

Suggestions up to commit 833e6f8

Category	Suggestion	Impact
General	Remove duplicate ThreadLocal cleanup The disableRequestCache ThreadLocal is removed twice: once in the catch block and again in the finally block. This is redundant and could mask issues if the finally block fails. Remove the duplicate cleanup from the catch block since the finally block guarantees cleanup. core/src/main/java/org/opensearch/sql/executor/QueryService.java [389-395] } catch (InterruptedException e) { Thread.currentThread().interrupt(); - CalcitePlanContext.disableRequestCache.remove(); listener.onFailure(new RuntimeException("Interrupted while waiting for query execution", e)); return; } finally { CalcitePlanContext.disableRequestCache.remove(); } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies redundant cleanup of the disableRequestCache ThreadLocal in both the catch block (line 390) and the finally block (line 394). Removing the duplicate from the catch block is the right approach since the finally block guarantees cleanup regardless of how the try block exits. This improves code clarity and prevents potential issues if the finally block fails.	Medium
	Validate index names before cache check If extractIndexNames returns an empty array, getRequestCacheHitCount is called with no indices, which may return aggregate stats across all indices instead of failing safely. Validate that indexNames is non-empty before retrieving cache stats to avoid incorrect cache hit detection. core/src/main/java/org/opensearch/sql/executor/QueryService.java [345-346] String[] indexNames = extractIndexNames(plan); -long cacheHitsBefore = disableCache ? -1 : executionEngine.getRequestCacheHitCount(indexNames); +long cacheHitsBefore = + disableCache || indexNames.length == 0 + ? -1 + : executionEngine.getRequestCacheHitCount(indexNames); Suggestion importance[1-10]: 6 __ Why: The suggestion identifies a potential issue where an empty indexNames array could lead to incorrect cache hit detection. Adding a check for indexNames.length == 0 is a reasonable defensive programming practice. However, the impact is moderate since the getRequestCacheHitCount implementation may already handle empty arrays gracefully, and the cache hit detection is already guarded by the disableCache flag.	Low
Possible issue	Fix cache flag timing issue The cache hit count is retrieved before disabling the cache, but the disableRequestCache flag is set afterward. This creates a race condition where the first query execution might use the cache before it's disabled, making the cache hit detection unreliable. Set the flag before retrieving the initial cache hit count. core/src/main/java/org/opensearch/sql/executor/QueryService.java [346-351] -long cacheHitsBefore = disableCache ? -1 : executionEngine.getRequestCacheHitCount(indexNames); - if (disableCache) { CalcitePlanContext.disableRequestCache.set(true); } +long cacheHitsBefore = disableCache ? -1 : executionEngine.getRequestCacheHitCount(indexNames); + AtomicReference<ExecutionEngine.QueryResponse> queryResponseRef = new AtomicReference<>(); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential race condition where the cache hit count is retrieved before the disableRequestCache flag is set. Setting the flag first ensures the cache is disabled before the initial count is retrieved, making the cache hit detection more reliable. However, since disableCache is hardcoded to true in the current code (line 339), this race condition doesn't currently manifest in practice.	Medium

Suggestions up to commit 9fcb5e7

Category	Suggestion	Impact
Possible issue	ThreadLocal cleanup missing on early exception The disableRequestCache ThreadLocal is set but never cleaned up if the execution path throws an exception before reaching the finally block. This can leak the disabled state to subsequent requests on the same thread, causing unintended cache behavior. core/src/main/java/org/opensearch/sql/executor/QueryService.java [345-352] long cacheHitsBefore = disableCache ? -1 : executionEngine.getRequestCacheHitCount(indexNames); if (disableCache) { CalcitePlanContext.disableRequestCache.set(true); } -AtomicReference<ExecutionEngine.QueryResponse> queryResponseRef = new AtomicReference<>(); +try { + AtomicReference<ExecutionEngine.QueryResponse> queryResponseRef = new AtomicReference<>(); Suggestion importance[1-10]: 8 __ Why: If an exception occurs between setting disableRequestCache and the try block starting at line 356, the ThreadLocal will leak to subsequent requests on the same thread. This is a real correctness issue that could cause cache behavior bugs.	Medium
General	Redundant ThreadLocal cleanup in exception handler The disableRequestCache.remove() is called twice when an InterruptedException occurs: once in the catch block and again in the finally block. This is redundant and could mask issues if the finally block is expected to be the sole cleanup point. core/src/main/java/org/opensearch/sql/executor/QueryService.java [389-395] } catch (InterruptedException e) { Thread.currentThread().interrupt(); - CalcitePlanContext.disableRequestCache.remove(); listener.onFailure(new RuntimeException("Interrupted while waiting for query execution", e)); return; } finally { CalcitePlanContext.disableRequestCache.remove(); } Suggestion importance[1-10]: 5 __ Why: The double cleanup is redundant but not harmful since remove() is idempotent. However, removing the redundant call improves code clarity and ensures the finally block is the single cleanup point.	Low
	Missing validation for empty index names If extractIndexNames returns an empty array, getRequestCacheHitCount may behave unexpectedly or return incorrect metrics. Validate that indexNames is non-empty before calling cache-related methods to ensure accurate cache hit tracking. core/src/main/java/org/opensearch/sql/executor/QueryService.java [345-346] String[] indexNames = extractIndexNames(plan); -long cacheHitsBefore = disableCache ? -1 : executionEngine.getRequestCacheHitCount(indexNames); +long cacheHitsBefore = (disableCache || indexNames.length == 0) ? -1 : executionEngine.getRequestCacheHitCount(indexNames); Suggestion importance[1-10]: 4 __ Why: While empty index names could cause unexpected behavior in getRequestCacheHitCount, the method likely handles this gracefully by returning -1. The suggestion adds defensive programming but addresses a minor edge case.	Low
	Overly broad exception handling masks failures Catching all exceptions and silently returning -1 can hide critical failures (e.g., authentication errors, cluster unavailability). Consider logging at error level or re-throwing exceptions that indicate serious issues rather than treating all failures as benign. opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java [380-383] } catch (Exception e) { - logger.warn("Failed to retrieve request cache stats", e); + logger.error("Failed to retrieve request cache stats for indices: " + String.join(",", indexNames), e); return -1; } Suggestion importance[1-10]: 3 __ Why: Changing log level from warn to error and adding index names improves diagnostics slightly, but the method's contract is to return -1 on any failure, so the broad catch is intentional. The improvement is marginal.	Low

Suggestions up to commit 3954423

Category	Suggestion	Impact
Possible issue	Fix phase lookup type mismatch The method assumes profile.getPhases() returns a Map<String, Phase>, but the actual type is Map<MetricName, Double> (see test setup). This mismatch will cause a ClassCastException at runtime when accessing phase timings. Correct the lookup to use MetricName enum keys instead of string keys. core/src/main/java/org/opensearch/sql/executor/analyze/AnalyzeRecommendationBuilder.java [280-284] -private double phaseTime(String phaseName) { - QueryProfile.Phase phase = - profile.getPhases() == null ? null : profile.getPhases().get(phaseName); - return phase == null ? 0 : phase.getTimeMillis(); +private double phaseTime(MetricName phaseName) { + return profile.getPhases() == null ? 0 : profile.getPhases().getOrDefault(phaseName, 0.0); } Suggestion importance[1-10]: 10 __ Why: Critical type error: profile.getPhases() returns Map<MetricName, Double> but the code treats it as Map<String, Phase>, causing a ClassCastException at runtime. The suggested fix correctly uses MetricName enum keys.	High
	Prevent ThreadLocal leak on early exception The disableRequestCache ThreadLocal is set but never removed if the code path before latch.await() throws an exception. This creates a ThreadLocal leak where subsequent requests on the same thread inherit the stale value. Wrap the entire block in try-finally to guarantee cleanup. core/src/main/java/org/opensearch/sql/executor/QueryService.java [345-395] long cacheHitsBefore = disableCache ? -1 : executionEngine.getRequestCacheHitCount(indexNames); if (disableCache) { CalcitePlanContext.disableRequestCache.set(true); } -AtomicReference<ExecutionEngine.QueryResponse> queryResponseRef = new AtomicReference<>(); -... +try { + AtomicReference<ExecutionEngine.QueryResponse> queryResponseRef = new AtomicReference<>(); + ... } finally { CalcitePlanContext.disableRequestCache.remove(); } Suggestion importance[1-10]: 9 __ Why: Critical bug: if an exception occurs before latch.await(), the ThreadLocal is never cleaned up, causing state pollution across requests on the same thread. The fix correctly wraps the entire block in try-finally.	High
General	Guard against empty index array If extractIndexNames returns an empty array, getRequestCacheHitCount may behave unexpectedly or return -1 even when cache stats are available. Validate that indexNames is non-empty before calling getRequestCacheHitCount to avoid incorrect cache-hit detection. core/src/main/java/org/opensearch/sql/executor/QueryService.java [345-346] String[] indexNames = extractIndexNames(plan); -long cacheHitsBefore = disableCache ? -1 : executionEngine.getRequestCacheHitCount(indexNames); +long cacheHitsBefore = disableCache || indexNames.length == 0 ? -1 : executionEngine.getRequestCacheHitCount(indexNames); Suggestion importance[1-10]: 6 __ Why: Valid defensive check: an empty indexNames array could cause getRequestCacheHitCount to return -1 even when the method is supported, leading to incorrect possibleCacheHit detection. The fix prevents this edge case.	Low
	Avoid ThreadLocal race in async execution The disableRequestCache field is read from the ThreadLocal at construction time but the ThreadLocal may be cleared before the search executes (e.g., in async callbacks). Store the boolean value at construction rather than relying on ThreadLocal state during execution to prevent race conditions. opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java [224-231] SearchRequest searchRequest = new SearchRequest().indices(indexName.getIndexNames()).source(this.sourceBuilder); -if (disableRequestCache) { +if (this.disableRequestCache) { searchRequest.requestCache(false); } Suggestion importance[1-10]: 3 __ Why: The concern is valid but the code already stores disableRequestCache as a final instance field at construction (line 146), so the ThreadLocal is only read once. The suggestion's premise is incorrect; no race exists here.	Low

Suggestions up to commit 6c8e52b

Category	Suggestion	Impact
General	Remove unreachable cache tracking code The disableCache variable is hardcoded to true, making the conditional logic and cache hit tracking unreachable. The commented parameter boolean disableCache in the method signature suggests this should be configurable. Either remove the dead code or expose disableCache as a method parameter. core/src/main/java/org/opensearch/sql/executor/QueryService.java [330-341] -boolean disableCache = true; // Force profiling on so executeWithCalcite activates QueryProfiling. QueryContext.setProfile(true); -String[] indexNames = extractIndexNames(plan); -long cacheHitsBefore = disableCache ? -1 : executionEngine.getRequestCacheHitCount(indexNames); +CalcitePlanContext.disableRequestCache.set(true); -if (disableCache) { - CalcitePlanContext.disableRequestCache.set(true); -} - Suggestion importance[1-10]: 7 __ Why: The disableCache variable is hardcoded to true, making the conditional checks for cacheHitsBefore and related logic unreachable. This creates dead code that should be removed or the variable should be made configurable as the commented parameter suggests.	Medium
	Remove redundant ThreadLocal cleanup The CalcitePlanContext.disableRequestCache.remove() call in the catch block is redundant because the finally block always executes afterward. Remove the duplicate cleanup from the catch block to avoid confusion and maintain cleaner exception handling. core/src/main/java/org/opensearch/sql/executor/QueryService.java [379-386] } catch (InterruptedException e) { Thread.currentThread().interrupt(); - CalcitePlanContext.disableRequestCache.remove(); listener.onFailure(new RuntimeException("Interrupted while waiting for query execution", e)); return; } finally { CalcitePlanContext.disableRequestCache.remove(); } Suggestion importance[1-10]: 6 __ Why: The CalcitePlanContext.disableRequestCache.remove() call in the catch block at line 381 is redundant since the finally block at line 385 always executes and performs the same cleanup. Removing the duplicate improves code clarity.	Low

[github-actions]

Persistent review updated to latest commit ac2182c

[github-actions]

Persistent review updated to latest commit 6c8e52b

[github-actions]

Persistent review updated to latest commit 3954423

[github-actions]

Persistent review updated to latest commit 9fcb5e7

[github-actions]

Persistent review updated to latest commit 833e6f8

[dai-chen]

Can we push request cache related changes later until we feel it's required? Currently I'm not sure how useful it is and passing it around seems tricky.

[Krish-Gandhi]

I think it is useful because a user doesn’t benefit from analysis of a cached query but I don’t mind removing it. What are your thoughts @ahkcs ?

[ahkcs]

It's a useful feature since cold timings matter, but I think we can follow @dai-chen's suggestion here to temporarily defer request cache related changes as they may be risky.

Two issues identified regarding request cache changes:

possibleCacheHit is always false today. disableCache is hardcoded true, so possibleCacheHit = !disableCache && … is a constant false, and getRequestCacheHitCount is never called (the disableCache ? -1 : … branch always wins).

The disable itself no-ops for script-bearing queries. disableRequestCache is set on the coordinating thread, but it's not in snapshotThreadLocals()/restoreThreadLocals(). The complex-pool dispatch relies on that snapshot to cross threads, and the request reads disableRequestCache.get() lazily in CalciteEnumerableIndexScan.enumerator() — on the sql-complex-worker thread, where it's the default false. So any hasScripts plan silently keeps the cache on.

[Krish-Gandhi]

Sounds good. Removing possibleCacheHit and cache disabling functionality on analyze path.

[dai-chen]

Split the internal logic into recommendation rule?

[Krish-Gandhi]

Can you elaborate on this

[dai-chen]

Sure, just thinking split the logic into some small inner class/lambda. Ref: FilterQueryBuilder

[dai-chen]

could you remind me currently how we capture the operator-level metrics?

[Krish-Gandhi]

They are captured using logic from existing profile API by reusing same execution path

[dai-chen]

Could you double confirm because I don't remember profile API has operator-level metrics. Is this from actual test output?

[penghuo]

How per-operator metrics collected? e.g. EnumerableCalc? I think profile API only collect phase (execution) metrics.
How does rows collected? why 0?

[Krish-Gandhi]

Here is output from existing profile API (i.e. not my changes):

curl -X POST "localhost:9200/_plugins/_ppl" \
  -H "Content-Type: application/json" \
  -d '{"query": "SOURCE = `test_data` | join left=l right=r on l.client_city=r.client_city `test_data` | head 1 | fields client_city", "profile": true}'
{
  "profile": {
    "summary": {
      "total_time_ms": 2203.61
    },
    "phases": {
      "analyze": {
        "time_ms": 7.76
      },
      "optimize": {
        "time_ms": 21.64
      },
      "execute": {
        "time_ms": 2174.02
      },
      "format": {
        "time_ms": 0.02
      }
    },
    "plan": {
      "node": "EnumerableCalc",
      "time_ms": 2171.57,
      "rows": 1,
      "children": [
        {
          "node": "EnumerableLimit",
          "time_ms": 2171.43,
          "rows": 1,
          "children": [
            {
              "node": "EnumerableLimit",
              "time_ms": 2171.42,
              "rows": 1,
              "children": [
                {
                  "node": "EnumerableMergeJoin",
                  "time_ms": 2171.42,
                  "rows": 2,
                  "children": [
                    {
                      "node": "CalciteEnumerableIndexScan",
                      "time_ms": 1842.1,
                      "rows": 367
                    },
                    {
                      "node": "CalciteEnumerableIndexScan",
                      "time_ms": 328.95,
                      "rows": 367
                    }
                  ]
                }
              ]
            }
          ]
        }
      ]
    },
    "thread_pool": "sql-complex-worker"
  },
  "schema": [
    {
      "name": "client_city",
      "type": "string"
    }
  ],
  "datarows": [
    [
      "Aaronborough"
    ]
  ],
  "total": 1,
  "size": 1
}

This is called from the analyze path and included in the analyze response.

How does rows collected? why 0?

This is because of the query includes where bytes_sent < 30 and there aren't any rows that fit that condition. This was done intentionally to show the general structure of the response with multiple operators, rather than using | head 1 where a pushdown can be confusing to reader. I can update endpoints.md to show a different query if needed.

[github-actions]

Persistent review updated to latest commit 7b87b68

[github-actions]

Persistent review updated to latest commit d58f214