Overriding profile endpoint with analyze endpoint with operator tree and profiling

[Krish-Gandhi]

Description

• Introduces the analyze endpoint for PPL queries, which can be activated by passing "analyze": true as a request body parameter.
• Overrides the existing profile endpoint to run analyze functionality.
• Propagate PPLanalyze flag through transport and request parsing.
• Combines logical plan and physical plan nodes by walking each tree and grouping nodes into an operator_tree.
• Maps PPL query segments and estimated/actual rows to operator_tree nodes.
• Combined current profile plan response with operator_tree nodes to calculate time taken by each node.

Note: This PR provides a simple implementation of creating the operator_tree, which works for simpler queries. The logic doesn't hold for queries that produce non-linear physical plan trees (for example, JOINs). This only affects the operator_tree, meaning the subset of the response that corresponds to profile is 100% functional. Therefore, in this scenario, the analyze endpoint will "fallback" to the profile output by returning a response mirroring the profile endpoint (by only including the profile, schema, datarows, total, and size fields in the response). This operator_tree issue will be addressed in a later PR.

---

Important

This PR overrides the existing profile endpoint by routing all requests with either "analyze": true or "profile": true (or both) to pplService.analyze() in TransportPPLQueryAction.java. For current end-users of the profile endpoint, this will make little difference, as the response from profile is a subset of the response from analyze. In simpler terms, current end-users can make no changes and have similar results.

All of the existing code for profile still exists and is in place. This makes it extremely simple to separate analyze and profile again. This can be done by removing the || transformedRequest.profile() part of the boolean expression in line 200 of TransportPPLQueryAction.java.

plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java:193-209:

    if (transformedRequest.isExplainRequest()) {
      pplService.explain(
          transformedRequest, createExplainResponseListener(transformedRequest, clearingListener));
    /**
     * Removing  `|| transformedRequest.profile()` from line 200 will
     * separate the `profile` and `analyze` endpoints. See PR #5568.
     */
    } else if (transformedRequest.analyze() || transformedRequest.profile()) {
      pplService.analyze(
        transformedRequest, createAnalyzeResponseListener(transformedRequest, clearingListener));
    } else {
      pplService.execute(
          transformedRequest,
          createListener(transformedRequest, clearingListener),
          createExplainResponseListener(transformedRequest, clearingListener));
    }
  }

---

Example Query and Response

The following curl command will run the query source=accounts | where age < 30 | eval full_name = firstname + \" \" + lastname | fields full_name, email, age on the analyze endpoint:

curl -X POST "localhost:9200/_plugins/_ppl" \
  -H "Content-Type: application/json" \
  -d '{"query": "source=accounts | where age < 30 | eval full_name = firstname + \" \" + lastname | fields full_name, email, age", "analyze": true}'

The response of this will be as follows. (NOTE: The "logicalPlan" and "physicalPlan" fields are included for debugging purposes and should not be included in the final version of this endpoint.)

{
  "query": "source=accounts | where age < 30 | eval full_name = firstname + \" \" + lastname | fields full_name, email, age",
  "querySegments": [
    {
      "nodeType": "SearchFrom",
      "source": "source=accounts"
    },
    {
      "nodeType": "WhereCommand",
      "source": "where age < 30"
    },
    {
      "nodeType": "EvalCommand",
      "source": "eval full_name = firstname + \" \" + lastname"
    },
    {
      "nodeType": "FieldsCommand",
      "source": "fields full_name, email, age"
    }
  ],
  "logicalPlan": [
    "LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT]): rowcount = 5000.0, cumulative cost = {114000.0 rows, 145000.0 cpu, 0.0 io}, id = 103",
    "LogicalProject(full_name=[||(||($0, ' '), $4)], email=[$3], age=[$2]): rowcount = 5000.0, cumulative cost = {109000.0 rows, 25000.0 cpu, 0.0 io}, id = 102",
    "LogicalFilter(condition=[<($2, 30)]): rowcount = 5000.0, cumulative cost = {104000.0 rows, 10000.0 cpu, 0.0 io}, id = 100",
    "CalciteLogicalIndexScan(table=[[OpenSearch, accounts]]): rowcount = 10000.0, cumulative cost = {99000.0 rows, 0.0 cpu, 0.0 io}, id = 99"
  ],
  "physicalPlan": [
    "EnumerableCalc(expr#0..3=[{inputs}], expr#4=[' '], expr#5=[||($t0, $t4)], expr#6=[||($t5, $t3)], full_name=[$t6], email=[$t2], age=[$t1]): rowcount = 5000.0, cumulative cost = {22996.4 rows, 50000.0 cpu, 0.0 io}, id = 193",
    "CalciteEnumerableIndexScan(table=[[OpenSearch, accounts]], PushDownContext=[[PROJECT->[firstname, age, email, lastname], FILTER-><($1, 30), LIMIT->10000], OpenSearchRequestBuilder(sourceBuilder={\"from\":0,\"size\":10000,\"timeout\":\"1m\",\"query\":{
  ],
  "profile": {
    "summary": {
      "total_time_ms": 777.18
    },
    "phases": {
      "analyze": {
        "time_ms": 477.57
      },
      "optimize": {
        "time_ms": 225.46
      },
      "execute": {
        "time_ms": 73.41
      },
      "format": {
        "time_ms": 0.11
      }
    },
    "plan": {
      "node": "EnumerableCalc",
      "time_ms": 64.19,
      "rows": 3,
      "children": [
        {
          "node": "CalciteEnumerableIndexScan",
          "time_ms": 64.0,
          "rows": 3
        }
      ]
    }
  },
  "operator_tree": [
    {
      "source": "source=accounts | where age < 30",
      "node_type": [
        "SearchFrom",
        "WhereCommand"
      ],
      "description": [
        "CalciteLogicalIndexScan(table=[[OpenSearch, accounts]]): rowcount = 10000.0, cumulative cost = {99000.0 rows, 0.0 cpu, 0.0 io}, id = 99",
        "LogicalFilter(condition=[<($2, 30)]): rowcount = 5000.0, cumulative cost = {104000.0 rows, 10000.0 cpu, 0.0 io}, id = 100"
      ],
      "estimated_rows": 5000,
      "actual_time_ms": "64.00 ms",
      "actual_rows": 3,
      "is_pushed_down": true
    },
    {
      "source": "eval full_name = firstname + \" \" + lastname | fields full_name, email, age",
      "node_type": [
        "EvalCommand",
        "FieldsCommand"
      ],
      "description": [
        "LogicalProject(full_name=[||(||($0, ' '), $4)], email=[$3], age=[$2]): rowcount = 5000.0, cumulative cost = {109000.0 rows, 25000.0 cpu, 0.0 io}, id = 102"
      ],
      "estimated_rows": 5000,
      "actual_time_ms": "0.19 ms",
      "actual_rows": 3
    }
  ],
  "recommendations": [
    {
      "serverity": "INFO",
      "rule": "Pushdown visibility",
      "message": "1 of 2 stages pushed down; 1 ran in-memory"
    },
    {
      "serverity": "INFO",
      "rule": "Bottleneck stage",
      "message": "87% of time is in the *SearchFrom, WhereCommand* stage",
      "affected_node": "source=accounts | where age < 30",
      "suggestion": "Consider optimizing the SearchFrom, WhereCommand operation"
    }
  ],
  "schema": [
    {
      "name": "full_name",
      "type": "STRING"
    },
    {
      "name": "email",
      "type": "STRING"
    },
    {
      "name": "age",
      "type": "LONG"
    }
  ],
  "datarows": [
    [
      "Jane Smith",
      "jane@example.com",
      28
    ],
    [
      "Kyle Miller",
      "goat@example.com",
      22
    ],
    [
      "Joette Kap",
      "coast2coast@example.com",
      22
    ]
  ],
  "total": 3,
  "size": 3
}

Performance of analyze

After writing a benchmarking script to run 20 queries on a sample 10 GB dataset, the results were as follows. This shows that analyze has a small enough overhead to justify running it by default on the Discover tab on the OpenSearch Dashboard.

=== PPL Query Benchmark ===

--- Results ---

normal      avg=4489.8ms  p50=206.9ms  p95=1083.1ms  min=28.5ms  max=84080.9ms
profile     avg=4407.9ms  p50=197.9ms  p95=749.1ms  min=19.1ms  max=83598.1ms
analyze     avg=4410.2ms  p50=213.3ms  p95=801.3ms  min=38.3ms  max=83146.3ms

Related Issues

#5500
Resolves #4343

Check List

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

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

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit ca820c6)

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 Interruption If the latch.await() is interrupted, the thread's interrupt flag is restored but the listener.onFailure is called with a new RuntimeException wrapping the InterruptedException. If the caller expects to handle interruptions differently (e.g., by propagating the interrupt without wrapping), this approach may not be appropriate. In a production scenario where thread interruption is used for cancellation, wrapping the exception could obscure the original intent. latch.await(); } catch (InterruptedException e) { Thread.currentThread().interrupt(); listener.onFailure(new RuntimeException("Interrupted while waiting for query execution", e)); Possible Issue In visitChildren, when tracking is enabled, the code assumes context.relBuilder.size() > 0 to get idBefore. If the builder is empty at the start, idBefore is set to -1. Then producedIds are computed as [idBefore+1 .. idAfter]. If the builder remains empty after child.accept (no RelNodes produced), idAfter could be less than idBefore+1, leading to an empty or incorrect range. This could happen if a child command produces no RelNodes, causing the loop to generate no ids or negative ids, which may break downstream logic expecting valid id ranges. if (context.isTrackingEnabled() && node instanceof UnresolvedPlan) { // Track each child's total contribution (the subtree it produces) RelNode result = null; for (Node child : node.getChild()) { int idBefore = context.relBuilder.size() > 0 ? context.relBuilder.peek().getId() : -1; RelNode childResult = child.accept(this, context); result = childResult; // After child.accept returns, the child's visit* method has fully completed, // so all RelNodes produced by that child (including ITS children) are on the stack. int idAfter = context.relBuilder.peek().getId(); if (child instanceof UnresolvedPlan) { List<Integer> producedIds = new ArrayList<>(); for (int id = idBefore + 1; id <= idAfter; id++) { producedIds.add(id); } context.recordMapping(child.getClass().getSimpleName(), producedIds); } } Possible Issue The code calls QueryProfiling.noop() in phase 2 to avoid profiling overhead, but then attempts to use profile data from phase 1. If phase 1's profile is null or incomplete (e.g., if profiling was not enabled or failed), the code proceeds to build the operator tree with a null profile. While buildOperatorTree handles null profile, the fallback logic in lines 360-389 checks profile != null && profile.getPlan() != null. If profile is null, the fallback is skipped and the code continues to phase 2, which may produce an operator tree with missing timing data. This could result in a response with null or zero timings, which may confuse consumers expecting valid timing data when analyze is requested. QueryProfiling.noop(); CalciteClassLoaderHelper.withCalciteClassLoader( () -> { CalcitePlanContext context = CalcitePlanContext.create( buildFrameworkConfig(), SysLimit.fromSettings(settings), queryType); context.setTrackingEnabled(true); RelNode relNode = analyze(plan, context); RelNode calcitePlan = convertToCalcitePlan(relNode, context); AtomicReference<String> physicalPlanRef = new AtomicReference<>(); AtomicReference<RelNode> physicalRelRef = new AtomicReference<>(); try (Hook.Closeable closeable = Hook.PLAN_BEFORE_IMPLEMENTATION.addThread( obj -> { RelRoot relRoot = (RelRoot) obj; physicalRelRef.set(relRoot.rel); physicalPlanRef.set( RelOptUtil.toString(relRoot.rel, SqlExplainLevel.ALL_ATTRIBUTES)); })) { try (java.sql.PreparedStatement ignored = OpenSearchRelRunners.run(context, calcitePlan)) { } catch (java.sql.SQLException e) { throw new RuntimeException(e); } } String logicalPlanStr = RelOptUtil.toString(calcitePlan, SqlExplainLevel.ALL_ATTRIBUTES); List<String> logicalPlanNodes = java.util.Arrays.stream(logicalPlanStr.split("\n")) .map(String::trim) .filter(s -> !s.isEmpty()) .toList(); List<String> physicalPlanNodes = java.util.Arrays.stream(physicalPlanRef.get().split("\n")) .map(String::trim) .filter(s -> !s.isEmpty()) .toList(); // Build operator tree using phase 2's tracking data + phase 1's profile. List<AnalyzeResponse.OperatorNode> operatorTree = buildOperatorTree( querySegments, logicalPlanNodes, context.getNodeIdMappings(), calcitePlan, physicalRelRef.get(), profile);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to ca820c6

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add timeout to latch await The latch.await() call blocks indefinitely if the query never completes. Add a timeout to prevent the analyze endpoint from hanging forever when queries fail silently or take too long, ensuring the service remains responsive. core/src/main/java/org/opensearch/sql/executor/QueryService.java [342] -latch.await(); +if (!latch.await(60, TimeUnit.SECONDS)) { + listener.onFailure(new RuntimeException("Query execution timed out after 60 seconds")); + return; +} Suggestion importance[1-10]: 9 __ Why: The latch.await() call blocks indefinitely if the query never completes, which can cause the analyze endpoint to hang. Adding a timeout is a critical improvement to prevent resource exhaustion and ensure the service remains responsive under failure conditions.	High
	Guard against empty builder stack The code assumes context.relBuilder.peek() is always non-null after accept(), but if the builder is empty, this will throw a NoSuchElementException. Add a null/empty check before calling peek() to prevent runtime failures when the builder stack is unexpectedly empty. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [236-243] int idBefore = context.relBuilder.size() > 0 ? context.relBuilder.peek().getId() : -1; RelNode result = unresolved.accept(this, context); +if (context.relBuilder.size() == 0) { + return result; +} int idAfter = context.relBuilder.peek().getId(); List<Integer> producedIds = new ArrayList<>(); for (int id = idBefore + 1; id <= idAfter; id++) { producedIds.add(id); } Suggestion importance[1-10]: 8 __ Why: The code calls context.relBuilder.peek().getId() after accept() without verifying the builder is non-empty. If the builder stack is unexpectedly empty, this will throw a NoSuchElementException, causing a runtime failure. The suggestion correctly identifies this potential issue and provides a reasonable guard.	Medium
General	Prevent negative pushed node count If physicalDepth > logicalDepth due to unexpected plan structure, pushedNodeCount becomes negative, causing incorrect segment grouping. Add a check to ensure pushedNodeCount is non-negative and handle the edge case gracefully to prevent logic errors in operator tree construction. core/src/main/java/org/opensearch/sql/executor/QueryService.java [536] -int pushedNodeCount = logicalDepth - physicalDepth; +int pushedNodeCount = Math.max(0, logicalDepth - physicalDepth); Suggestion importance[1-10]: 8 __ Why: If physicalDepth > logicalDepth due to unexpected plan structure, pushedNodeCount becomes negative, which would cause incorrect behavior in subsequent segment grouping logic. Using Math.max(0, ...) is a simple and effective safeguard against this edge case.	Medium
	Validate ID ordering before loop When idBefore equals idAfter, the loop produces an empty list, but when idBefore > idAfter (which can occur if RelNode IDs are not strictly increasing), the loop never executes and silently produces incorrect results. Add validation to ensure idAfter >= idBefore before constructing the range. core/src/main/java/org/opensearch/sql/executor/QueryService.java [240-242] -for (int id = idBefore + 1; id <= idAfter; id++) { - producedIds.add(id); +if (idAfter >= idBefore) { + for (int id = idBefore + 1; id <= idAfter; id++) { + producedIds.add(id); + } } Suggestion importance[1-10]: 7 __ Why: The suggestion addresses a potential edge case where idAfter < idBefore could lead to an empty list being produced silently. While the current logic assumes IDs are strictly increasing, adding this validation makes the code more robust against unexpected RelNode ID behavior.	Medium

---

Previous suggestions

Suggestions up to commit ca820c6

Category	Suggestion	Impact
Possible issue	Prevent NoSuchElementException on empty builder The code assumes context.relBuilder.peek() is always valid after accept(), but if the builder is empty, this will throw a NoSuchElementException. Add a check to ensure the builder is not empty before calling peek(). core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [236-243] int idBefore = context.relBuilder.size() > 0 ? context.relBuilder.peek().getId() : -1; RelNode result = unresolved.accept(this, context); +if (context.relBuilder.size() == 0) { + return result; +} int idAfter = context.relBuilder.peek().getId(); List<Integer> producedIds = new ArrayList<>(); for (int id = idBefore + 1; id <= idAfter; id++) { producedIds.add(id); } Suggestion importance[1-10]: 8 __ Why: The code calls context.relBuilder.peek() after accept() without verifying the builder is non-empty, which can throw NoSuchElementException. This is a critical bug that could crash the tracking logic.	Medium
	Guard peek() with size check Similar to the analyze method, peek() is called without verifying the builder is non-empty. If child.accept() empties the builder, peek() will throw NoSuchElementException. Add a size check before accessing peek(). core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [255-260] int idBefore = context.relBuilder.size() > 0 ? context.relBuilder.peek().getId() : -1; RelNode childResult = child.accept(this, context); result = childResult; +if (context.relBuilder.size() == 0) { + continue; +} int idAfter = context.relBuilder.peek().getId(); Suggestion importance[1-10]: 8 __ Why: Similar to suggestion 1, peek() is called without verifying the builder is non-empty after child.accept(). This can cause NoSuchElementException and is a critical bug in the tracking logic.	Medium
	Add type check before casting plan The cast (QueryProfile.PlanNode) profile.getPlan() assumes the plan is always of this type. If getPlan() returns a different type or null, this will cause a ClassCastException. Add a type check before casting. core/src/main/java/org/opensearch/sql/executor/QueryService.java [574-584] List<double[]> physicalTimings = new ArrayList<>(); -if (profile != null && profile.getPlan() != null) { +if (profile != null && profile.getPlan() instanceof QueryProfile.PlanNode) { List<QueryProfile.PlanNode> planNodes = new ArrayList<>(); QueryProfile.PlanNode current = (QueryProfile.PlanNode) profile.getPlan(); while (current != null) { planNodes.add(current); current = (current.getChildren() != null && !current.getChildren().isEmpty()) ? current.getChildren().get(0) : null; } Suggestion importance[1-10]: 7 __ Why: The cast (QueryProfile.PlanNode) profile.getPlan() assumes a specific type without verification. Adding an instanceof check prevents potential ClassCastException and makes the code more robust.	Medium
General	Clean up profiling context on interruption After catching InterruptedException and re-interrupting the thread, the method returns but doesn't clean up the profiling context. Ensure QueryProfiling.noop() or similar cleanup is called to prevent resource leaks or stale state. core/src/main/java/org/opensearch/sql/executor/QueryService.java [341-347] try { latch.await(); } catch (InterruptedException e) { Thread.currentThread().interrupt(); + QueryProfiling.noop(); listener.onFailure(new RuntimeException("Interrupted while waiting for query execution", e)); return; } Suggestion importance[1-10]: 6 __ Why: The suggestion to call QueryProfiling.noop() on interruption is reasonable to prevent stale profiling state, though the impact is moderate since the method already re-interrupts the thread and returns early.	Low

Suggestions up to commit 10853cf

Category	Suggestion	Impact
Possible issue	Add timeout to latch await Using latch.await() without a timeout can cause the thread to block indefinitely if the async callback never completes. Add a timeout to await() and handle the timeout case to prevent potential deadlocks. core/src/main/java/org/opensearch/sql/executor/QueryService.java [311-348] CountDownLatch latch = new CountDownLatch(1); executeWithCalcite( plan, queryType, null, new ResponseListener<>() { @Override public void onResponse(ExecutionEngine.QueryResponse response) { ... latch.countDown(); } @Override public void onFailure(Exception e) { errorRef.set(e); latch.countDown(); } }); try { - latch.await(); + if (!latch.await(60, TimeUnit.SECONDS)) { + listener.onFailure(new RuntimeException("Query execution timed out after 60 seconds")); + return; + } Suggestion importance[1-10]: 8 __ Why: Using latch.await() without a timeout can cause indefinite blocking if the async callback fails to complete. Adding a timeout prevents potential deadlocks and improves system resilience, which is critical for production systems.	Medium
	Add null safety check for builder The code assumes context.relBuilder.peek() is always non-null after accept(), which may not hold if the builder is empty. Add a null check or verify the builder has elements before calling peek() to prevent potential NullPointerException. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [236-243] int idBefore = context.relBuilder.size() > 0 ? context.relBuilder.peek().getId() : -1; RelNode result = unresolved.accept(this, context); +if (context.relBuilder.size() == 0) { + context.recordMapping(unresolved.getClass().getSimpleName(), List.of()); + return result; +} int idAfter = context.relBuilder.peek().getId(); List<Integer> producedIds = new ArrayList<>(); for (int id = idBefore + 1; id <= idAfter; id++) { producedIds.add(id); } Suggestion importance[1-10]: 7 __ Why: The code assumes context.relBuilder.peek() is non-null after accept(), which could cause a NullPointerException if the builder is empty. Adding a null check improves robustness, though the likelihood of this occurring in practice may be low.	Medium
General	Clean up profiling context properly The early return for non-linear plan trees doesn't clean up the QueryProfiling context that was activated at the start of analyzeWithCalcite. This could lead to profiling state leaking into subsequent queries. Ensure profiling is properly reset before returning. core/src/main/java/org/opensearch/sql/executor/QueryService.java [361-390] if (profile != null && profile.getPlan() != null && !isLinearPlanTree(profile)) { ... - listener.onResponse( - AnalyzeResponse.builder() - .profile(profile) - .schema(schema) - .datarows(datarows) - .total(datarows.length) - .size(datarows.length) - .build()); + try { + listener.onResponse( + AnalyzeResponse.builder() + .profile(profile) + .schema(schema) + .datarows(datarows) + .total(datarows.length) + .size(datarows.length) + .build()); + } finally { + QueryProfiling.clear(); + } return; } Suggestion importance[1-10]: 6 __ Why: The early return for non-linear plan trees doesn't explicitly clean up the QueryProfiling context. While the context may be cleaned up elsewhere, explicitly resetting it here improves code clarity and prevents potential state leakage.	Low
	Verify multi-child result handling The loop overwrites result with each child's result, meaning only the last child's RelNode is returned. For nodes with multiple children (e.g., joins), this loses information about earlier children. Verify this behavior is intentional or accumulate results appropriately. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [251-257] if (context.isTrackingEnabled() && node instanceof UnresolvedPlan) { RelNode result = null; for (Node child : node.getChild()) { int idBefore = context.relBuilder.size() > 0 ? context.relBuilder.peek().getId() : -1; RelNode childResult = child.accept(this, context); - result = childResult; + if (result == null) { + result = childResult; + } + if (context.relBuilder.size() == 0) continue; int idAfter = context.relBuilder.peek().getId(); Suggestion importance[1-10]: 5 __ Why: The loop overwrites result with each child's result, keeping only the last one. For nodes with multiple children (e.g., joins), this may lose information. However, the PR description indicates this is for linear plan trees, so this behavior may be intentional. Verification is recommended.	Low

Suggestions up to commit e12840a

Category	Suggestion	Impact
Possible issue	Prevent NoSuchElementException on empty builder The code assumes context.relBuilder.peek() is non-null after accept() returns, but if the builder is empty, this will throw a NoSuchElementException. Add a null/empty check before calling peek() to prevent runtime exceptions when the builder stack is unexpectedly empty. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [226-234] int idBefore = context.relBuilder.size() > 0 ? context.relBuilder.peek().getId() : -1; RelNode result = unresolved.accept(this, context); +if (context.relBuilder.size() == 0) { + return result; +} int idAfter = context.relBuilder.peek().getId(); List<Integer> producedIds = new ArrayList<>(); for (int id = idBefore + 1; id <= idAfter; id++) { producedIds.add(id); } Suggestion importance[1-10]: 8 __ Why: The code assumes context.relBuilder.peek() is non-null after accept() returns. If the builder is empty, this will throw a NoSuchElementException. The suggestion correctly identifies this potential runtime exception and provides a reasonable fix by checking if the builder is empty before calling peek().	Medium
	Prevent NoSuchElementException in visitChildren tracking Similar to the analyze method, visitChildren calls context.relBuilder.peek() without checking if the builder is empty after child.accept() returns. If a child produces no RelNodes, this will throw NoSuchElementException. Add a size check before accessing peek(). core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [242-251] if (context.isTrackingEnabled() && node instanceof UnresolvedPlan) { - // Track each child's total contribution (the subtree it produces) RelNode result = null; for (Node child : node.getChild()) { int idBefore = context.relBuilder.size() > 0 ? context.relBuilder.peek().getId() : -1; RelNode childResult = child.accept(this, context); result = childResult; - // After child.accept returns, the child's visit* method has fully completed, - // so all RelNodes produced by that child (including ITS children) are on the stack. + if (context.relBuilder.size() == 0) { + continue; + } int idAfter = context.relBuilder.peek().getId(); Suggestion importance[1-10]: 8 __ Why: Similar to suggestion 1, this correctly identifies that context.relBuilder.peek() could throw NoSuchElementException if the builder is empty after child.accept() returns. The suggestion provides a reasonable fix by checking the builder size before calling peek().	Medium
General	Clean up profiling state on interruption After catching InterruptedException and re-interrupting the thread, the method returns without cleaning up the QueryProfiling context or resetting QueryContext.setProfile(false). This could leave profiling enabled for subsequent requests. Ensure proper cleanup in the exception path. core/src/main/java/org/opensearch/sql/executor/QueryService.java [317-323] try { latch.await(); } catch (InterruptedException e) { Thread.currentThread().interrupt(); + QueryContext.setProfile(false); listener.onFailure(new RuntimeException("Interrupted while waiting for query execution", e)); return; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that QueryContext.setProfile(false) should be called in the exception path to prevent profiling from remaining enabled for subsequent requests. This is a valid resource cleanup issue that could affect subsequent operations.	Medium
	Extract duplicate schema/datarows extraction logic The fallback path for non-linear plan trees duplicates schema and datarows extraction logic that appears again later in the method. Extract this logic into a helper method to avoid code duplication and reduce maintenance burden. core/src/main/java/org/opensearch/sql/executor/QueryService.java [336-365] if (profile != null && profile.getPlan() != null && !isLinearPlanTree(profile)) { - List<AnalyzeResponse.SchemaColumn> schema = new ArrayList<>(); - if (queryResponse.getSchema() != null) { - for (ExecutionEngine.Schema.Column col : queryResponse.getSchema().getColumns()) { - schema.add( - AnalyzeResponse.SchemaColumn.builder() - .name(col.getName()) - .type(col.getExprType().typeName()) - .build()); - } - } - Object[][] datarows = new Object[queryResponse.getResults().size()][]; - int rowIdx = 0; - for (var exprValue : queryResponse.getResults()) { - datarows[rowIdx++] = - exprValue.tupleValue().entrySet().stream() - .map(e -> e.getValue().value()) - .toArray(Object[]::new); - } + List<AnalyzeResponse.SchemaColumn> schema = extractSchema(queryResponse); + Object[][] datarows = extractDatarows(queryResponse); listener.onResponse( AnalyzeResponse.builder() - // .query(query) .profile(profile) .schema(schema) .datarows(datarows) .total(datarows.length) .size(datarows.length) .build()); return; } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies code duplication between the fallback path and the main path. Extracting this logic into helper methods would improve maintainability. However, the impact is moderate as it's primarily a code quality improvement rather than a functional issue.	Low

Suggestions up to commit af78a5e

Category	Suggestion	Impact
Possible issue	Prevent NoSuchElementException on empty builder The code assumes context.relBuilder is non-empty after accept() returns, but if the builder is empty, peek() will throw NoSuchElementException. Add a check to verify the builder has elements before calling peek() to prevent runtime crashes. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [227-234] int idBefore = context.relBuilder.size() > 0 ? context.relBuilder.peek().getId() : -1; RelNode result = unresolved.accept(this, context); +if (context.relBuilder.size() == 0) { + return result; +} int idAfter = context.relBuilder.peek().getId(); List<Integer> producedIds = new ArrayList<>(); for (int id = idBefore + 1; id <= idAfter; id++) { producedIds.add(id); } Suggestion importance[1-10]: 8 __ Why: The code calls context.relBuilder.peek() after unresolved.accept() without verifying the builder is non-empty. If accept() leaves the builder empty, peek() will throw NoSuchElementException, causing a runtime crash. This is a critical defensive check.	Medium
	Guard peek call after child accept Similar to the previous issue, peek() is called without verifying the builder is non-empty after child.accept(). If the child produces no RelNodes, this will throw NoSuchElementException. Guard the peek() call with a size check. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [246-251] int idBefore = context.relBuilder.size() > 0 ? context.relBuilder.peek().getId() : -1; RelNode childResult = child.accept(this, context); result = childResult; +if (context.relBuilder.size() == 0) { + continue; +} int idAfter = context.relBuilder.peek().getId(); Suggestion importance[1-10]: 8 __ Why: Similar to the first issue, peek() is called after child.accept() without checking if the builder is non-empty. This can throw NoSuchElementException if the child produces no RelNodes, which is a potential runtime crash.	Medium
General	Validate hook object type before casting The hook callback casts obj to RelRoot without validation. If Calcite passes an unexpected object type, this will throw ClassCastException. Add type checking before casting to handle unexpected hook invocations gracefully. core/src/main/java/org/opensearch/sql/executor/QueryService.java [383-397] try (Hook.Closeable closeable = Hook.PLAN_BEFORE_IMPLEMENTATION.addThread( obj -> { - RelRoot relRoot = (RelRoot) obj; - physicalRelRef.set(relRoot.rel); - physicalPlanRef.set( - RelOptUtil.toString(relRoot.rel, SqlExplainLevel.ALL_ATTRIBUTES)); + if (obj instanceof RelRoot relRoot) { + physicalRelRef.set(relRoot.rel); + physicalPlanRef.set( + RelOptUtil.toString(relRoot.rel, SqlExplainLevel.ALL_ATTRIBUTES)); + } })) { try (java.sql.PreparedStatement ignored = OpenSearchRelRunners.run(context, calcitePlan)) { } catch (java.sql.SQLException e) { throw new RuntimeException(e); } } Suggestion importance[1-10]: 6 __ Why: The hook callback casts obj to RelRoot without validation. While Calcite's PLAN_BEFORE_IMPLEMENTATION hook is expected to pass RelRoot, adding type checking makes the code more defensive against unexpected behavior or future Calcite changes.	Low

Suggestions up to commit 9fa9096

Category	Suggestion	Impact
Possible issue	Prevent NoSuchElementException on empty builder The tracking logic assumes relBuilder.peek() is always valid after accept(), but if the builder is empty, peek() will throw NoSuchElementException. Add a null/empty check before calling peek() to prevent crashes when the builder stack is unexpectedly empty. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [226-235] int idBefore = context.relBuilder.size() > 0 ? context.relBuilder.peek().getId() : -1; RelNode result = unresolved.accept(this, context); +if (context.relBuilder.size() == 0) { + context.recordMapping(unresolved.getClass().getSimpleName(), List.of()); + return result; +} int idAfter = context.relBuilder.peek().getId(); List<Integer> producedIds = new ArrayList<>(); for (int id = idBefore + 1; id <= idAfter; id++) { producedIds.add(id); } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential NoSuchElementException when context.relBuilder.peek() is called on an empty builder. The check context.relBuilder.size() > 0 is only performed before accept(), but not after, which could lead to a crash if the builder becomes empty during processing.	Medium
	Add timeout to prevent indefinite blocking The latch.await() blocks indefinitely if the async callback never completes (e.g., due to an internal error that bypasses both onResponse and onFailure). Add a timeout to await() to prevent the thread from hanging forever and allow graceful error handling. core/src/main/java/org/opensearch/sql/executor/QueryService.java [317-323] -CountDownLatch latch = new CountDownLatch(1); -... try { - latch.await(); + if (!latch.await(60, java.util.concurrent.TimeUnit.SECONDS)) { + listener.onFailure(new RuntimeException("Query execution timed out after 60 seconds")); + return; + } } catch (InterruptedException e) { Thread.currentThread().interrupt(); listener.onFailure(new RuntimeException("Interrupted while waiting for query execution", e)); return; } Suggestion importance[1-10]: 7 __ Why: Valid concern about indefinite blocking if the async callback fails to complete. Adding a timeout to latch.await() is a reasonable safeguard against thread hangs, though the 60-second timeout value is somewhat arbitrary and could be configurable.	Medium
General	Populate missing fields in fallback response When falling back due to non-linear plan trees, the response omits critical fields like query, querySegments, logicalPlan, and physicalPlan that clients may expect. This inconsistency could break consumers. Either populate these fields or document the fallback behavior clearly to avoid unexpected null values. core/src/main/java/org/opensearch/sql/executor/QueryService.java [336-365] if (profile != null && profile.getPlan() != null && !isLinearPlanTree(profile)) { ... listener.onResponse( AnalyzeResponse.builder() + .query(query) + .querySegments(querySegments) + .logicalPlan(List.of()) + .physicalPlan(List.of()) + .operator_tree(List.of()) + .recommendations(List.of()) .profile(profile) .schema(schema) .datarows(datarows) .total(datarows.length) .size(datarows.length) .build()); return; } Suggestion importance[1-10]: 6 __ Why: The suggestion identifies an inconsistency where the fallback response omits fields like query, querySegments, logicalPlan, and physicalPlan. While this could cause issues for clients expecting these fields, the PR documentation mentions this is intentional fallback behavior for non-linear plans. Populating empty lists is a reasonable improvement for consistency.	Low
	Avoid overwriting result in multi-child loop The loop unconditionally overwrites result with each child's result, meaning only the last child's RelNode is returned. For nodes with multiple children (e.g., joins), this discards earlier children's contributions. Verify this is intentional or accumulate results properly. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [245-251] for (Node child : node.getChild()) { int idBefore = context.relBuilder.size() > 0 ? context.relBuilder.peek().getId() : -1; RelNode childResult = child.accept(this, context); - result = childResult; + if (result == null) { + result = childResult; + } + if (context.relBuilder.size() == 0) continue; int idAfter = context.relBuilder.peek().getId(); Suggestion importance[1-10]: 3 __ Why: The suggestion raises a concern about overwriting result in the loop, but the improved code doesn't fully address the issue. The logic appears intentional for linear plan processing (each child builds on the previous), and the suggestion's fix of only setting result once may break the intended behavior. The concern is valid but the solution needs more thought.	Low

[github-actions]

PR Code Analyzer ❗

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

Path	Line	Severity	Description
core/src/main/java/org/opensearch/sql/executor/QueryService.java	280	medium	The analyze endpoint silently routes existing `profile: true` requests to the new analyze handler (`transformedRequest.analyze() || transformedRequest.profile()`), changing the response format for all current profile consumers. While the profile sub-object is preserved, additional fields (logicalPlan, physicalPlan, operator_tree, datarows, schema, etc.) are now returned unconditionally. Consumers expecting the old profile-only response shape may inadvertently expose or log internal query plan details (table structures, filter predicates, cost estimates, row counts) that were not previously surfaced.
core/src/main/java/org/opensearch/sql/executor/QueryService.java	302	low	CountDownLatch.await() is called with no timeout. If the inner executeWithCalcite callback never fires (e.g., thread pool exhaustion or a hung query), the calling thread blocks indefinitely, potentially exhausting the thread pool and causing a denial-of-service condition.
core/src/main/java/org/opensearch/sql/executor/QueryService.java	358	low	The analyze path executes the user query twice — once via executeWithCalcite for profiling and a second time via OpenSearchRelRunners.run() for plan capture — doubling datastore load per analyze request. On expensive queries this doubles resource consumption, which could be exploited to amplify resource usage compared to normal query execution.

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]

Persistent review updated to latest commit 50ddb92

[github-actions]

Persistent review updated to latest commit 127dfe7

[github-actions]

Persistent review updated to latest commit 727e3da

[dai-chen]

High level questions:

1. Is it possible to avoid adding more top-level fields like querySegments, logicalPlan, physicalPlan, operator_tree and recommendations. Any idea how to group them better? Like the first 3 more like explain output and the last 2 belong to profile?
2. How to extend this to SQL language and Analytics engine backend? From what I see, querySegments may not work for SQL and operator_tree is mostly for OpenSearch execution?

[Krish-Gandhi]

High level questions:

1. Is it possible to avoid adding more top-level fields like querySegments, logicalPlan, physicalPlan, operator_tree and recommendations. Any idea how to group them better? Like the first 3 more like explain output and the last 2 belong to profile?

Agreed, querySegments, logicalPlan and physicalPlan will be removed in a future PR. They are redundant, but left in for now because they are useful for debugging.

2. How to extend this to SQL language and Analytics engine backend? From what I see, querySegments may not work for SQL and operator_tree is mostly for OpenSearch execution?

As far as I can tell, extending to SQL will be interesting for a few reasons, however it (should be) much easier. The main complexity in my implementation for PPL ANALYZE is building the operator_tree, i.e. mapping query operations to their physical execution node. Theoretically, this should be much simpler in SQL, because the push down logic is simpler to track and doesn't use Calcite for regular OpenSearch indices. After the operator_tree is built, the same recommendations/rules can be applied. This changes if Calcite is integrated more with SQL in the future, but, in that scenario, the current PPL ANALYZE solution is easier to utilize with SQL. SQL on analytics-engine indices already uses Calcite today, so the current implementation should work with minimal changes. SQL segments would be clause-based rather than pipe-based.

For the Analytics engine, we need a profile-to-operator-tree adapter, but the data is already there and it provides more insights than Calcite. Again, after the operator_tree is built, we can use the same rules/recommendations.

[github-actions]

Persistent review updated to latest commit b4ff0a9

[github-actions]

Persistent review updated to latest commit 9fa9096

[github-actions]

Persistent review updated to latest commit af78a5e

[github-actions]

Persistent review updated to latest commit e12840a

[dai-chen]

High level questions:

1. Is it possible to avoid adding more top-level fields like querySegments, logicalPlan, physicalPlan, operator_tree and recommendations. Any idea how to group them better? Like the first 3 more like explain output and the last 2 belong to profile?

Agreed, querySegments, logicalPlan and physicalPlan will be removed in a future PR. They are redundant, but left in for now because they are useful for debugging.

2. How to extend this to SQL language and Analytics engine backend? From what I see, querySegments may not work for SQL and operator_tree is mostly for OpenSearch execution?

As far as I can tell, extending to SQL will be interesting for a few reasons, however it (should be) much easier. The main complexity in my implementation for PPL ANALYZE is building the operator_tree, i.e. mapping query operations to their physical execution node. Theoretically, this should be much simpler in SQL, because the push down logic is simpler to track and doesn't use Calcite for regular OpenSearch indices. After the operator_tree is built, the same recommendations/rules can be applied. This changes if Calcite is integrated more with SQL in the future, but, in that scenario, the current PPL ANALYZE solution is easier to utilize with SQL. SQL on analytics-engine indices already uses Calcite today, so the current implementation should work with minimal changes. SQL segments would be clause-based rather than pipe-based.

For the Analytics engine, we need a profile-to-operator-tree adapter, but the data is already there and it provides more insights than Calcite. Again, after the operator_tree is built, we can use the same rules/recommendations.

Sure, IMO, the response format should be clear and extensible for different language (SQL/PPL) + backend (OS/AE) without breaking changes. It will be very helpful if we can take all these into account in the proposal.

Also could you confirm if we need feature branch if there is a series of PR for this work? Currently we cut release branch from main branch directly. Just want to make sure there is no half-done or impact on existing functionalities. Thanks!

[github-actions]

Persistent review updated to latest commit 10853cf

[github-actions]

Persistent review updated to latest commit ca820c6

[github-actions]

Persistent review updated to latest commit ca820c6