Updating analyze endpoint

[Krish-Gandhi]

Description

• Removing query operation tracking and operator_ tree from analyze
• Cleaned up analyze response
• Adding cache hit analysis, cache disabling, functionality
• 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,
  "possibleCacheHit": false
}

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 58bbf5e.

Path	Line	Severity	Description
core/src/main/java/org/opensearch/sql/executor/QueryService.java	1339	medium	The withCheckedArithmetic method and its helpers (isCheckableIntegerArithmetic, isCheckableLongType, findArithmeticOverflow) were deleted. This removes the protection that rewrote PLUS/MINUS/TIMES over BIGINT operands to overflow-checked variants (Math.addExact etc.), meaning integer/long arithmetic will now silently wrap on overflow instead of throwing ArithmeticException. This is a deliberate regression of an overflow guard that was previously applied to both coordinator-side and pushed-down (script) arithmetic.
core/src/main/java/org/opensearch/sql/executor/QueryService.java	284	low	boolean disableCache is hardcoded to true with the corresponding parameter commented out (// boolean disableCache,). This unconditionally bypasses the OpenSearch shard request cache for every analyzeWithCalcite call. While bypassing cache for diagnostic endpoints is plausible, the commented-out parameter suggests this was intentionally hidden from callers rather than being a deliberate API design choice, making the behavior non-obvious.
opensearch/src/main/java/org/opensearch/sql/opensearch/request/OpenSearchQueryRequest.java	227	low	A [CACHE_DEBUG] LOG.info statement logging disableRequestCache flag state and the resolved requestCache() value is commented out but left in production code. Debug log lines that expose internal request configuration details are a minor anomaly and should be removed before merging.

The table above displays the top 10 most important findings.

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

---

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 3954423)

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 Resource Leak The CalcitePlanContext.disableRequestCache ThreadLocal is set but not always removed in the analyzeWithCalcite method. If an exception occurs before the finally block at line 393, the ThreadLocal remains set, potentially affecting subsequent requests on the same thread. This can cause unintended cache-disabling behavior for other queries. CalcitePlanContext.disableRequestCache.set(true); } AtomicReference<ExecutionEngine.QueryResponse> queryResponseRef = new AtomicReference<>(); AtomicReference<QueryProfile> profileRef = new AtomicReference<>(); AtomicReference<Exception> errorRef = new AtomicReference<>(); CountDownLatch latch = new CountDownLatch(1); executeWithCalcite( plan, queryType, null, new ResponseListener<>() { @Override public void onResponse(ExecutionEngine.QueryResponse response) { ProfileMetric formatMetric = QueryProfiling.current().getOrCreateMetric(MetricName.FORMAT); long formatStart = System.nanoTime(); int resultSize = response.getResults().size(); for (var exprValue : response.getResults()) { exprValue.tupleValue().entrySet().stream() .map(e -> e.getValue().value()) .toArray(Object[]::new); } formatMetric.set(System.nanoTime() - formatStart); profileRef.set(QueryProfiling.current().finish()); queryResponseRef.set(response); latch.countDown(); } @Override public void onFailure(Exception e) { errorRef.set(e); latch.countDown(); } }); try { latch.await(); } 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(); } Possible Issue The extractIndexNames method at line 739 collects relation names by calling getTableQualifiedName().toString() on Relation nodes. If getTableQualifiedName() returns null, this will throw a NullPointerException. The code does not guard against this scenario. private static String[] extractIndexNames(UnresolvedPlan plan) { Set<String> names = new HashSet<>(); collectRelationNames(plan, names); return names.toArray(String[]::new); } private static void collectRelationNames(Node node, Set<String> names) { if (node instanceof Relation relation) { names.add(relation.getTableQualifiedName().toString()); } if (node.getChild() != null) { for (Node child : node.getChild()) { collectRelationNames(child, names); } } } Possible Issue The getRequestCacheHitCount method returns -1 when nodeClientOpt.isEmpty() is true (line 366), but the calling code in QueryService.analyzeWithCalcite (lines 346, 402) uses this -1 value in comparisons (cacheHitsBefore >= 0, cacheHitsAfter >= 0) without documenting that -1 means "not supported". If the REST client path is taken, cache hit detection silently fails. This inconsistency can mislead users about cache behavior. public long getRequestCacheHitCount(String... indexNames) { Optional<NodeClient> nodeClientOpt = client.getNodeClient(); if (nodeClientOpt.isEmpty()) { return -1; } try { return nodeClientOpt .get() .admin() .indices() .prepareStats(indexNames) .clear() .setRequestCache(true) .get() .getTotal() .getRequestCache() .getHitCount(); } catch (Exception e) { logger.warn("Failed to retrieve request cache stats", e); return -1; } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 3954423

Explore these optional code suggestions:

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

---

Previous suggestions

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

Suggestions up to commit ac2182c

Category	Suggestion	Impact
Possible issue	Ensure ThreadLocal cleanup on exceptions The disableRequestCache ThreadLocal is set but never cleaned up if an exception occurs before the finally block. Move the ThreadLocal setup into a try-finally block that wraps the entire cache-hit measurement and query execution to ensure cleanup happens even on early exceptions. core/src/main/java/org/opensearch/sql/executor/QueryService.java [337-386] -long cacheHitsBefore = disableCache ? -1 : executionEngine.getRequestCacheHitCount(indexNames); +long cacheHitsBefore = -1; +try { + if (disableCache) { + CalcitePlanContext.disableRequestCache.set(true); + } + cacheHitsBefore = disableCache ? -1 : executionEngine.getRequestCacheHitCount(indexNames); -if (disableCache) { - CalcitePlanContext.disableRequestCache.set(true); -} - -AtomicReference<ExecutionEngine.QueryResponse> queryResponseRef = new AtomicReference<>(); -... + AtomicReference<ExecutionEngine.QueryResponse> queryResponseRef = new AtomicReference<>(); + ... } finally { CalcitePlanContext.disableRequestCache.remove(); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that CalcitePlanContext.disableRequestCache.remove() should be called even if an exception occurs before the finally block at line 385. However, the existing code already has a finally block that removes the ThreadLocal, and there's also an earlier cleanup in the catch (InterruptedException e) block at line 381. The suggestion would improve robustness by ensuring the ThreadLocal is set within the try block, but the current code does handle cleanup in the main error paths.	Medium
General	Remove or enable cache detection The disableCache variable is hardcoded to true, making the cache hit detection code unreachable. Either remove the dead code for cache hit detection or expose disableCache as a parameter so the feature can be enabled when needed. core/src/main/java/org/opensearch/sql/executor/QueryService.java [330-337] -boolean disableCache = true; -// Phase 1: Execute via the exact same path as executeWithCalcite + executionEngine.execute -// to get identical profile timings. Use a latch to synchronize the async callback. -// Force profiling on so executeWithCalcite activates QueryProfiling. +boolean disableCache = true; // TODO: make configurable via request parameter QueryContext.setProfile(true); String[] indexNames = extractIndexNames(plan); long cacheHitsBefore = disableCache ? -1 : executionEngine.getRequestCacheHitCount(indexNames); Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that disableCache is hardcoded to true at line 330, making the cache hit detection code (lines 337, 393-398) effectively dead code. The TODO comment suggestion is reasonable, but the PR may intentionally disable this feature for now. The score is moderate because while this is a valid observation, it may be intentional technical debt that the team is aware of.	Low
	Document cache hit detection limitations The cache hit detection logic has a race condition: other concurrent queries on the same indices can increment the cache hit count between measurements, causing false positives. Consider adding a warning in the response or documentation that this detection is best-effort and may report false positives in multi-tenant environments. core/src/main/java/org/opensearch/sql/executor/QueryService.java [393-398] long cacheHitsAfter = disableCache ? -1 : executionEngine.getRequestCacheHitCount(indexNames); +// Note: possibleCacheHit may have false positives due to concurrent queries boolean possibleCacheHit = !disableCache && cacheHitsBefore >= 0 && cacheHitsAfter >= 0 && cacheHitsAfter > cacheHitsBefore; Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a potential race condition in cache hit detection where concurrent queries could cause false positives. Adding a comment would improve code clarity. However, this is a minor documentation improvement rather than a functional issue, and the feature is already experimental (as indicated by the analyze endpoint being experimental).	Low

Suggestions up to commit 58bbf5e

Category	Suggestion	Impact
General	Fix typo in field name The field name serverity is misspelled and should be severity. This typo will propagate through the API response and could cause confusion or integration issues for API consumers. core/src/main/java/org/opensearch/sql/executor/AnalyzeResponse.java [65-73] @Data @Builder public static class Recommendation { - private final RecommendationSeverityLevel serverity; + private final RecommendationSeverityLevel severity; private final String rule; private final String message; private final String affected_node; private final String suggestion; } Suggestion importance[1-10]: 9 __ Why: The suggestion identifies a critical typo in the field name serverity which should be severity. This is a public API field that will be exposed in responses, making this a high-impact bug that could cause integration issues.	High
	Remove redundant ThreadLocal cleanup The ThreadLocal cleanup in the catch block is redundant because the finally block already ensures cleanup. Remove the duplicate remove() call from the catch block to avoid confusion and maintain cleaner exception handling. core/src/main/java/org/opensearch/sql/executor/QueryService.java [336-343] } 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 ThreadLocal cleanup in the catch block when the finally block already handles it. This is a valid code quality improvement that eliminates unnecessary duplication.	Medium
	Remove unreachable cache tracking code The disableCache variable is hardcoded to true, making the conditional logic for cache hit tracking unreachable. Either remove the dead code branches or make disableCache configurable via a parameter to enable cache hit detection when needed. core/src/main/java/org/opensearch/sql/executor/QueryService.java [287-298] -boolean disableCache = true; // Phase 1: Execute via the exact same path as executeWithCalcite + executionEngine.execute // to get identical profile timings. Use a latch to synchronize the async callback. // Force profiling on so executeWithCalcite activates QueryProfiling. QueryContext.setProfile(true); String[] indexNames = extractIndexNames(plan); -long cacheHitsBefore = disableCache ? -1 : executionEngine.getRequestCacheHitCount(indexNames); +boolean disableCache = true; +long cacheHitsBefore = -1; -if (disableCache) { - CalcitePlanContext.disableRequestCache.set(true); -} +CalcitePlanContext.disableRequestCache.set(true); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that disableCache is hardcoded to true, making the conditional logic for cacheHitsBefore unreachable. However, the commented parameter // boolean disableCache at line 278 suggests this may be intentionally disabled for now with plans to make it configurable later.	Medium
	Simplify disabled cache hit logic Since disableCache is hardcoded to true, cacheHitsAfter will always be -1 and possibleCacheHit will always be false. This creates misleading logic that suggests cache hit detection is functional when it's actually disabled. Simplify by directly setting possibleCacheHit = false. core/src/main/java/org/opensearch/sql/executor/QueryService.java [350-355] -long cacheHitsAfter = disableCache ? -1 : executionEngine.getRequestCacheHitCount(indexNames); -boolean possibleCacheHit = - !disableCache - && cacheHitsBefore >= 0 - && cacheHitsAfter >= 0 - && cacheHitsAfter > cacheHitsBefore; +long cacheHitsAfter = -1; +boolean possibleCacheHit = false; Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that with disableCache hardcoded to true, the complex conditional logic for possibleCacheHit is unnecessary. However, similar to suggestion 1, this may be intentionally structured for future configurability.	Medium

[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