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

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 When latch.await() is interrupted, the thread's interrupt flag is restored but the method continues to check errorRef.get() and queryResponseRef.get(). If the latch was interrupted before the callback set these references, both will be null, causing a NullPointerException when accessing queryResponse.getSchema() at line 338. latch.await(); } catch (InterruptedException e) { Thread.currentThread().interrupt(); listener.onFailure(new RuntimeException("Interrupted while waiting for query execution", e)); return; } if (errorRef.get() != null) { listener.onFailure(errorRef.get()); return; } Possible NPE queryResponseRef.get() can be null if the callback hasn't executed yet when latch.await() returns (e.g., due to interruption or spurious wakeup). Accessing queryResponse.getSchema() at line 338 without a null check will throw NullPointerException. // If the profile plan tree has branching (any node with >1 child), our linear // operator tree logic won't work. Return a response that 'fallsback' on `profile` // by only including fields mirroring the `profile` endpoint. if (profile != null && profile.getPlan() != null && !isLinearPlanTree(profile)) { List<AnalyzeResponse.SchemaColumn> schema = new ArrayList<>(); if (queryResponse.getSchema() != null) { Incorrect ID Tracking When tracking is enabled, idBefore is set to context.relBuilder.peek().getId() only if the builder is non-empty. If empty, idBefore = -1. After child.accept(), idAfter = context.relBuilder.peek().getId() assumes the builder is non-empty. If the child produces no RelNodes, peek() will throw NoSuchElementException. 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. Incorrect Exclusive Time Exclusive time is computed as inclusive - childInclusive where childInclusive is the time of the node at index p-1. For the bottom node (p=0), childInclusive=0 is correct. For other nodes, this assumes a linear chain where each node has exactly one child. If the profile tree has multiple children or a different structure, this calculation produces incorrect exclusive times. double inclusive = planNodes.get(p).getTimeMillis(); double childInclusive = (p > 0) ? planNodes.get(p - 1).getTimeMillis() : 0; double exclusive = Math.max(0, inclusive - childInclusive); long rows = planNodes.get(p).getRows();

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to e12840a

Explore these optional code suggestions:

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

---

Previous suggestions

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

Suggestions up to commit b4ff0a9

Category	Suggestion	Impact
Possible issue	Add timeout to prevent indefinite blocking The latch.await() call blocks indefinitely if the query never completes. Add a timeout to prevent the thread from hanging forever, and handle the timeout case by returning an appropriate error to the client. core/src/main/java/org/opensearch/sql/executor/QueryService.java [291-292] -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: The suggestion addresses a critical issue where latch.await() blocks indefinitely if the query never completes. Adding a timeout with proper error handling prevents thread hangs and improves system resilience, which is important for production stability.	Medium
	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 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 [223-230] 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]: 7 __ Why: The suggestion correctly identifies a potential NoSuchElementException when context.relBuilder.peek() is called on an empty stack after accept(). Adding a size check before peek() prevents crashes, though the likelihood depends on whether accept() can leave the builder empty in practice.	Medium
General	Include query field in fallback response The fallback response for non-linear plan trees omits the query field (commented out), causing inconsistent API responses. Either include the query field or document why it's intentionally excluded to maintain consistent response structure. core/src/main/java/org/opensearch/sql/executor/QueryService.java [310-338] if (profile != null && profile.getPlan() != null && !isLinearPlanTree(profile)) { ... listener.onResponse( AnalyzeResponse.builder() - // .query(query) + .query(query) .profile(profile) ... .build()); return; } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies an inconsistency where the query field is commented out in the fallback response but included in the main response. Including it ensures consistent API responses, though this is a minor issue that doesn't affect functionality.	Low
	Validate timing ordering before subtraction The exclusive time calculation assumes a linear parent-child relationship, but if planNodes ordering doesn't match the actual tree structure, the subtraction produces incorrect exclusive times. Verify that planNodes is correctly ordered before computing exclusive times. core/src/main/java/org/opensearch/sql/executor/QueryService.java [537-543] for (int p = 0; p < planNodes.size(); p++) { double inclusive = planNodes.get(p).getTimeMillis(); - double childInclusive = (p > 0) ? planNodes.get(p - 1).getTimeMillis() : 0; + double childInclusive = 0; + if (p > 0 && planNodes.get(p - 1).getTimeMillis() <= inclusive) { + childInclusive = planNodes.get(p - 1).getTimeMillis(); + } double exclusive = Math.max(0, inclusive - childInclusive); long rows = planNodes.get(p).getRows(); physicalTimings.add(new double[] {exclusive, rows}); } Suggestion importance[1-10]: 5 __ Why: The suggestion adds a validation check before computing exclusive times, but the improved_code only checks if childInclusive <= inclusive, which doesn't fully validate the ordering assumption. The concern is valid but the fix is incomplete and may not prevent incorrect calculations in all cases.	Low

Suggestions up to commit ff87829

Category	Suggestion	Impact
Possible issue	Guard against empty builder stack Check if context.relBuilder is empty before calling peek() after accept() returns. If the builder is empty, peek() will throw NoSuchElementException, causing the analyze operation to fail unexpectedly. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [222-232] if (context.isTrackingEnabled()) { 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(); java.util.List<Integer> producedIds = new java.util.ArrayList<>(); for (int id = idBefore + 1; id <= idAfter; id++) { producedIds.add(id); } context.recordMapping(unresolved.getClass().getSimpleName(), producedIds); return result; } Suggestion importance[1-10]: 8 __ Why: This addresses a potential NoSuchElementException when calling peek() on an empty relBuilder stack after accept() returns. The check is necessary to prevent runtime failures during tracking.	Medium
	Add timeout to latch await Add a timeout to latch.await() to prevent indefinite blocking if the async callback never completes. Without a timeout, the thread could hang forever if executeWithCalcite fails to invoke either callback, leading to resource exhaustion. core/src/main/java/org/opensearch/sql/executor/QueryService.java [292-297] -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("Timeout waiting for query execution")); + 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: Adding a timeout prevents indefinite blocking if the async callback fails to complete, which is a valid concern for production systems. However, the 60-second timeout value is arbitrary and may need tuning based on actual query execution times.	Medium
General	Prevent negative pushed node count Validate that pushedNodeCount is non-negative before using it as a loop boundary. If physicalDepth exceeds logicalDepth due to unexpected plan structure, the negative value could cause incorrect operator tree grouping or infinite loops. core/src/main/java/org/opensearch/sql/executor/QueryService.java [486-508] -int pushedNodeCount = logicalDepth - physicalDepth; +int pushedNodeCount = Math.max(0, logicalDepth - physicalDepth); ... long pushedLogicalNodes = 0; int pushedSegments = 0; for (int idx = 0; idx < querySegments.size() && pushedLogicalNodes < pushedNodeCount; idx++) { Set<Integer> ids = idx < exclusiveIds.size() ? exclusiveIds.get(idx) : Set.of(); long planNodeCount = ids.stream().filter(idToDescription::containsKey).count(); pushedLogicalNodes += planNodeCount; pushedSegments++; } Suggestion importance[1-10]: 6 __ Why: Using Math.max(0, ...) prevents negative values that could cause incorrect operator tree grouping. While this is a defensive programming practice, the scenario where physicalDepth exceeds logicalDepth may indicate a deeper issue that should be investigated rather than silently handled.	Low
	Include empty operator tree field Set operator_tree to an empty list instead of omitting it when falling back to profile-only mode. Consumers expecting the field to always be present may encounter null pointer exceptions or deserialization errors if it's missing. core/src/main/java/org/opensearch/sql/executor/QueryService.java [329-338] -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()); - return; -} +listener.onResponse( + AnalyzeResponse.builder() + .profile(profile) + .operator_tree(List.of()) + .schema(schema) + .datarows(datarows) + .total(datarows.length) + .size(datarows.length) + .build()); Suggestion importance[1-10]: 5 __ Why: Setting operator_tree to an empty list instead of omitting it improves API consistency and prevents potential null pointer exceptions in consumers. However, the AnalyzeResponse.builder() may already handle null fields appropriately, and the documentation should clarify the field's optionality.	Low

Suggestions up to commit 7f164a0

Category	Suggestion	Impact
Possible issue	Guard against empty builder stack Accessing context.relBuilder.peek() after unresolved.accept() can throw NoSuchElementException if the builder stack is empty. Verify the builder has elements before calling peek() to prevent runtime crashes during tracking. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [222-232] if (context.isTrackingEnabled()) { 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(); java.util.List<Integer> producedIds = new java.util.ArrayList<>(); for (int id = idBefore + 1; id <= idAfter; id++) { producedIds.add(id); } context.recordMapping(unresolved.getClass().getSimpleName(), producedIds); return result; } Suggestion importance[1-10]: 8 __ Why: This is a critical safety check. After unresolved.accept() at line 224, the builder stack could be empty, and calling peek() at line 225 would throw NoSuchElementException. The guard prevents a potential runtime crash during tracking.	Medium
	Add timeout to latch await Add a timeout to latch.await() to prevent indefinite blocking if the async callback never completes. Without a timeout, the thread could hang forever if executeWithCalcite fails to invoke either callback, leading to resource exhaustion. core/src/main/java/org/opensearch/sql/executor/QueryService.java [291-297] -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; + } } catch (InterruptedException e) { Thread.currentThread().interrupt(); listener.onFailure(new RuntimeException("Interrupted while waiting for query execution", e)); return; } Suggestion importance[1-10]: 7 __ Why: Adding a timeout prevents indefinite blocking if the async callback fails to complete, which is a valid concern for production code. However, the 60-second timeout value is arbitrary and may need tuning based on actual query execution patterns.	Medium
	Prevent negative pushed node count When pushedNodeCount is negative (if physicalDepth > logicalDepth), the loop condition pushedLogicalNodes < pushedNodeCount will never be satisfied, causing all segments to be marked as pushed down incorrectly. Add validation to ensure pushedNodeCount is non-negative. core/src/main/java/org/opensearch/sql/executor/QueryService.java [484-508] -int pushedNodeCount = logicalDepth - physicalDepth; +int pushedNodeCount = Math.max(0, logicalDepth - physicalDepth); ... long pushedLogicalNodes = 0; int pushedSegments = 0; for (int idx = 0; idx < querySegments.size() && pushedLogicalNodes < pushedNodeCount; idx++) { Set<Integer> ids = idx < exclusiveIds.size() ? exclusiveIds.get(idx) : Set.of(); long planNodeCount = ids.stream().filter(idToDescription::containsKey).count(); pushedLogicalNodes += planNodeCount; pushedSegments++; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that pushedNodeCount could be negative if physicalDepth > logicalDepth, which would cause incorrect pushdown marking. Using Math.max(0, ...) at line 486 ensures the value is non-negative and prevents logic errors in the subsequent loop.	Medium
General	Remove or uncomment query field The commented-out .query(query) field should either be included or removed entirely. Leaving commented code in production suggests incomplete implementation and may confuse future maintainers about whether this field should be present. core/src/main/java/org/opensearch/sql/executor/QueryService.java [329-338] if (profile != null && profile.getPlan() != null && !isLinearPlanTree(profile)) { ... listener.onResponse( AnalyzeResponse.builder() - // .query(query) + .query(query) .profile(profile) .schema(schema) .datarows(datarows) .total(datarows.length) .size(datarows.length) .build()); return; } Suggestion importance[1-10]: 3 __ Why: While removing commented code improves maintainability, the commented field at line 332 appears intentional (possibly for debugging). The suggestion correctly identifies that the field should be included in the fallback response, as it is included in the main response at line 419.	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

[ahkcs]

Let's update the endpoint.md(https://github.com/opensearch-project/sql/blob/main/docs/user/ppl/interfaces/endpoint.md) and integ-test/src/yamlRestTest/resources/rest-api-spec/test/api/ppl.profile.yml file for our profile API change

[ahkcs]

Let's also document the current limitation for join, also we can provide fallback option to original profile API when we meet limitations

[github-actions]

Persistent review updated to latest commit 7f164a0

[github-actions]

Persistent review updated to latest commit ff87829

[Krish-Gandhi]

Updated docs/user/ppl/interfaces/endpoint.md to add documentation for current state of analyze and update note for profile endpoint, as well as a note about when the logic of analyze falls apart and fallback to profile. Also updated integ-test/src/yamlRestTest/resources/rest-api-spec/test/api/ppl.profile.yml and added integ-test/src/yamlRestTest/resources/rest-api-spec/test/api/ppl.analyze.yml.

[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!