Integrate SQL REST endpoint with analytics engine path

[ahkcs]

Summary

• Add SQL query routing through the analytics engine for Parquet-backed indices
• SQL queries targeting parquet_* indices are routed to RestUnifiedQueryAction via the unified query pipeline (Calcite SQL parser → UnifiedQueryPlanner → AnalyticsExecutionEngine)
• Non-analytics SQL queries fall through to the existing V2 engine unchanged

Changes

• Add SqlNode table extraction in RestUnifiedQueryAction.extractIndexName() for SQL routing
• Add executeSql() and explainSql() methods in RestUnifiedQueryAction
• Add analytics routing in RestSqlAction via optional BiFunction router
• Wire the router in SQLPlugin.createSqlAnalyticsRouter()
• Add AnalyticsSQLIT integration tests

Resolves step 3: #5248

[github-actions]

PR Code Analyzer ❗

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

Path	Line	Severity	Description
plugin/build.gradle	351	high	A prebuilt binary plugin zip file ('analytics-engine-3.6.0-SNAPSHOT.zip') is loaded from a local 'libs/' directory that is not managed by a standard package registry and has no visible source code in this diff. Per mandatory supply chain rules, all build plugin additions must be flagged — the provenance and contents of this binary artifact cannot be verified from the diff alone.
plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java	200	medium	The new SQL analytics router intercepts all queries targeting 'parquet_*' indices and routes them through a separate execution path (RestUnifiedQueryAction + StubQueryPlanExecutor) before standard authorization and request handling. If the analytics execution path has weaker access controls than the normal SQL engine, this router could act as an authorization bypass for any index whose name matches the 'parquet_' prefix. The stub executor returning hardcoded rows regardless of the plan also warrants review.

The table above displays the top 10 most important findings.

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

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit a91d349)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Add analytics SQL integration tests Relevant files: integ-test/src/test/java/org/opensearch/sql/sql/AnalyticsSQLIT.java integ-test/src/test/java/org/opensearch/sql/sql/AnalyticsSQLExplainIT.java Sub-PR theme: Add SQL routing and executeSql/explainSql methods to RestUnifiedQueryAction Relevant files: plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java Sub-PR theme: Wire analytics router into RestSqlAction via SQLPlugin Relevant files: legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java

⚡ Recommended focus areas for review Stub Mismatch The test explicitly documents that data values appear mismatched due to the stub executor returning rows in a fixed order that doesn't match the schema order. Tests with known-incorrect assertions should not be merged as-is; they validate the wrong behavior and will need to be updated when the real engine is wired in, creating technical debt. // Stub returns [ts, status, message, ip_addr] per row, mapped by position to // [ip_addr, message, status, ts] schema. Values appear mismatched — expected with stub. verifyDataRows( result, rows("2024-01-15 10:30:00", 200, "Request completed", "192.168.1.1"), rows("2024-01-15 10:31:00", 200, "Health check OK", "192.168.1.2"), rows("2024-01-15 10:32:00", 500, "Internal server error", "192.168.1.3")); Error Swallowed In createSqlAnalyticsRouter(), both onFailure handlers send only e.getMessage() as the response body with no JSON structure. This may expose raw exception messages to clients and doesn't conform to the standard OpenSearch error response format. Additionally, exceptions thrown by channel.sendResponse() itself are not caught. public void onFailure(Exception e) { channel.sendResponse( new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, e.getMessage())); } Incomplete SQL Routing extractTableNameFromSqlNode only handles SqlSelect with a direct SqlIdentifier or SqlJoin from clause. Subqueries, CTEs, aliased tables (SqlBasicCall with AS), and other SQL constructs will return null, causing analytics queries with these patterns to silently fall through to the legacy engine rather than failing with a clear error. private static String extractTableNameFromSqlNode(SqlNode sqlNode) { if (sqlNode instanceof SqlSelect select) { SqlNode from = select.getFrom(); if (from instanceof SqlIdentifier id) { return id.toString(); } if (from instanceof SqlJoin join) { // For joins, extract from the left table if (join.getLeft() instanceof SqlIdentifier leftId) { return leftId.toString(); } } } return null; } Fallback Logic In the analytics router fallback path (when analyticsRouter.apply() returns false), the code calls explainRequest and executeSqlRequest from the legacy engine rather than delegating cleanly to newSqlQueryHandler. This means non-analytics queries go through the legacy engine even when newSqlQueryHandler would have handled them, potentially bypassing the V2 engine for supported queries. try { newSqlQueryHandler .prepareRequest( finalRequest, (ch, ex) -> { try { Format fmt = SqlRequestParam.getFormat(request.params()); QueryAction qa = explainRequest(client, sqlRequest, fmt); executeSqlRequest(request, qa, client, ch); } catch (Exception e) { handleException(ch, e); } }, StubQueryPlanExecutor StubQueryPlanExecutor is wired into production code via SQLPlugin.createSqlAnalyticsRouter(). Stub/test implementations should not be used in production plugin initialization. This should be replaced with the real executor or guarded by a feature flag before merging. /** * Execute a SQL query through the unified query pipeline. Uses {@link

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to a91d349

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Guard against null exception messages in error responses When e.getMessage() returns null (e.g., for a NullPointerException), BytesRestResponse will receive a null message, potentially causing a secondary failure. Use Objects.toString(e.getMessage(), e.getClass().getName()) or similar to ensure a non-null error message is always sent. plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java [224-246] @Override public void onFailure(Exception e) { + String msg = e.getMessage() != null ? e.getMessage() : e.getClass().getName(); channel.sendResponse( - new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, e.getMessage())); + new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, msg)); } }); } else { unifiedQueryHandler.executeSql( sqlRequest.getQuery(), QueryType.SQL, new ActionListener<>() { @Override public void onResponse(TransportPPLQueryResponse response) { channel.sendResponse( new BytesRestResponse( RestStatus.OK, "application/json; charset=UTF-8", response.getResult())); } @Override public void onFailure(Exception e) { + String msg = e.getMessage() != null ? e.getMessage() : e.getClass().getName(); channel.sendResponse( - new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, e.getMessage())); + new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, msg)); } }); Suggestion importance[1-10]: 5 __ Why: e.getMessage() can return null for some exceptions like NullPointerException, which could cause issues when constructing BytesRestResponse. The fix is straightforward and prevents a potential secondary failure, though the improved_code only fixes one of the two onFailure methods while the suggestion mentions both.	Low
	Safely serialize query parameter in JSON body The SQL query is directly interpolated into a JSON string without escaping, which will break if the query contains double quotes or backslashes. Use a JSONObject to safely serialize the query value instead of manual string concatenation. integ-test/src/test/java/org/opensearch/sql/sql/AnalyticsSQLIT.java [60-66] private JSONObject executeSqlQuery(String sql) throws IOException { Request request = new Request("POST", "/_plugins/_sql"); - request.setJsonEntity("{\"query\": \"" + sql + "\"}"); + JSONObject body = new JSONObject(); + body.put("query", sql); + request.setJsonEntity(body.toString()); request.setOptions(RequestOptions.DEFAULT); Response response = client().performRequest(request); return new JSONObject(getResponseBody(response)); } Suggestion importance[1-10]: 4 __ Why: The suggestion is valid — direct string interpolation into JSON is fragile and could break with special characters. However, in integration tests the queries are hardcoded simple strings, so the practical risk is low. The improvement is real but minor in this context.	Low
General	Handle nested joins in SQL table name extraction The method returns null for subqueries, aliases, and nested joins, which will cause isAnalyticsIndex to receive null and potentially throw a NullPointerException or silently misroute queries. At minimum, the callers of extractIndexName should handle a null return safely, and this method should be documented to clarify its limitations. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [246-259] private static String extractTableNameFromSqlNode(SqlNode sqlNode) { if (sqlNode instanceof SqlSelect select) { SqlNode from = select.getFrom(); if (from instanceof SqlIdentifier id) { return id.toString(); } if (from instanceof SqlJoin join) { // For joins, extract from the left table - if (join.getLeft() instanceof SqlIdentifier leftId) { + SqlNode left = join.getLeft(); + if (left instanceof SqlIdentifier leftId) { return leftId.toString(); } + // Recurse for nested joins + return extractTableNameFromSqlNode(left); } } return null; } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that nested joins (e.g., A JOIN B JOIN C) would return null from the current implementation, and adds recursion to handle them. This is a genuine edge case improvement, though the current PR is focused on basic analytics routing and nested joins may not be a priority yet.	Low

---

Previous suggestions

Suggestions up to commit 7b63b49

Category	Suggestion	Impact
Possible issue	Fix incorrect fallback routing for non-analytics queries The fallback path inside the analytics router lambda calls explainRequest unconditionally for all non-analytics queries, even for regular (non-explain) SQL queries. This means every non-analytics query will be treated as an explain request, which is incorrect. The fallback should mirror the original logic that only calls explainRequest when the request is an explain request. legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java [165-187] return channel -> { if (!analyticsRouter.apply(finalRequest, channel)) { // Not an analytics query — delegate to normal SQL engine try { newSqlQueryHandler .prepareRequest( finalRequest, (ch, ex) -> { try { Format fmt = SqlRequestParam.getFormat(request.params()); - QueryAction qa = explainRequest(client, sqlRequest, fmt); - executeSqlRequest(request, qa, client, ch); + if (finalRequest.isExplainRequest()) { + QueryAction qa = explainRequest(client, sqlRequest, fmt); + executeSqlRequest(request, qa, client, ch); + } else { + handleException(ch, ex); + } } catch (Exception e) { handleException(ch, e); } }, this::handleException) .accept(channel); } catch (Exception e) { handleException(channel, e); } } }; Suggestion importance[1-10]: 7 __ Why: The fallback lambda calls explainRequest unconditionally for all non-analytics queries, which would incorrectly treat regular SQL queries as explain requests. This is a legitimate logic bug in the fallback path, though the improved_code also has issues since it calls handleException(ch, ex) for non-explain queries instead of properly executing them.	Medium
	Prevent JSON injection in query serialization The SQL query is directly interpolated into a JSON string without escaping, which will break if the query contains special characters like quotes or backslashes. Use a proper JSON serialization method (e.g., JSONObject) to build the request body safely. integ-test/src/test/java/org/opensearch/sql/sql/AnalyticsSQLIT.java [59-61] private JSONObject executeSqlQuery(String sql) throws IOException { Request request = new Request("POST", "/_plugins/_sql"); - request.setJsonEntity("{\"query\": \"" + sql + "\"}"); + JSONObject body = new JSONObject(); + body.put("query", sql); + request.setJsonEntity(body.toString()); Suggestion importance[1-10]: 5 __ Why: Direct string interpolation into JSON is fragile and could break with special characters in test queries. Using JSONObject for serialization is safer, though in integration tests the queries are controlled strings, making this a low-risk issue.	Low
General	Handle nested joins in table name extraction For JOIN queries, the left side may itself be a nested join or subquery rather than a direct SqlIdentifier, causing null to be returned and the analytics routing to silently fall through. The method should recursively handle nested joins or subqueries to correctly extract the table name. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [252-257] if (from instanceof SqlJoin join) { - // For joins, extract from the left table - if (join.getLeft() instanceof SqlIdentifier leftId) { - return leftId.toString(); - } + // For joins, recursively extract from the left side + return extractTableNameFromSqlNode(join.getLeft()); } Suggestion importance[1-10]: 5 __ Why: The current implementation only handles SqlIdentifier on the left side of a join, but nested joins or subqueries would return null, causing silent fallthrough. Recursive extraction is more robust, though this is an edge case for the current stub implementation.	Low
	Guard against null exception messages in error responses Sending e.getMessage() directly in the response may expose internal implementation details or sensitive information. Additionally, e.getMessage() can be null for some exceptions, causing a NullPointerException. Use a safe fallback message and consider logging the full exception. plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java [224-227] @Override public void onFailure(Exception e) { + String message = e.getMessage() != null ? e.getMessage() : "Internal server error"; + LOG.error("Analytics query execution failed", e); channel.sendResponse( - new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, e.getMessage())); + new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, message)); } Suggestion importance[1-10]: 4 __ Why: e.getMessage() can return null for some exceptions, which could cause issues in the response. However, BytesRestResponse may handle null gracefully, and this is a minor defensive improvement rather than a critical fix.	Low

Suggestions up to commit 38710ef

Category	Suggestion	Impact
Possible issue	Guard against null exception messages e.getMessage() can return null for some exception types (e.g., NullPointerException), which would cause a NullPointerException when constructing the BytesRestResponse. Use a null-safe fallback such as e.toString() or Objects.toString(e.getMessage(), e.getClass().getName()). plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java [227-231] @Override public void onFailure(Exception e) { + String msg = e.getMessage() != null ? e.getMessage() : e.getClass().getName(); channel.sendResponse( new org.opensearch.rest.BytesRestResponse( - org.opensearch.core.rest.RestStatus.INTERNAL_SERVER_ERROR, e.getMessage())); + org.opensearch.core.rest.RestStatus.INTERNAL_SERVER_ERROR, msg)); } Suggestion importance[1-10]: 6 __ Why: e.getMessage() can return null for certain exception types like NullPointerException, which would cause a secondary NullPointerException when constructing BytesRestResponse. This pattern appears twice in the file (for both explain and execute failure handlers), making it a real risk.	Low
	Avoid unsafe JSON string concatenation Building JSON by string concatenation is unsafe — if sql contains double quotes or backslashes, it will produce malformed JSON and cause hard-to-diagnose test failures. Use a JSONObject to construct the request body safely. integ-test/src/test/java/org/opensearch/sql/sql/AnalyticsSQLIT.java [59-61] private JSONObject executeSqlQuery(String sql) throws IOException { Request request = new Request("POST", "/_plugins/_sql"); - request.setJsonEntity("{\"query\": \"" + sql + "\"}"); + JSONObject body = new JSONObject(); + body.put("query", sql); + request.setJsonEntity(body.toString()); Suggestion importance[1-10]: 5 __ Why: String concatenation for JSON construction is fragile and can produce malformed JSON if sql contains special characters. Using JSONObject is safer and more robust, though in test code the risk is lower since queries are hardcoded strings.	Low
General	Handle nested joins in table name extraction For a join query, the left side may itself be another SqlJoin (e.g., three-way join) or a subquery, not just a SqlIdentifier. In those cases the method silently returns null, which will cause the analytics routing check to fail and fall through to the normal engine unexpectedly. Consider recursing into the left node or handling nested joins explicitly. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [252-257] if (from instanceof SqlJoin join) { - // For joins, extract from the left table - if (join.getLeft() instanceof SqlIdentifier leftId) { - return leftId.toString(); - } + // For joins, recursively extract from the left-most table + return extractTableNameFromSqlNode(join.getLeft()); } Suggestion importance[1-10]: 6 __ Why: The current implementation only handles the case where the left side of a join is a SqlIdentifier, silently returning null for nested joins. Recursing into join.getLeft() correctly handles multi-table joins and prevents unexpected fallthrough to the normal SQL engine.	Low
	Remove duplicate stale Javadoc comment There are two consecutive Javadoc comment blocks on the same method, which is invalid — only the last one will be associated with the method by Javadoc tools, and the first is dead/misleading documentation. Remove the first (stale) comment block. plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java [182-190] - /** - * Create a routing function for SQL queries targeting analytics engine indices. Returns a {@link - * org.opensearch.rest.BaseRestHandler.RestChannelConsumer} if the query targets an analytics - * index, or {@code null} to fall through to the normal SQL engine. - */ /** * Creates a routing function for SQL queries targeting analytics engine indices. Returns {@code * true} if the query was handled (analytics index), {@code false} to fall through to normal SQL. */ Suggestion importance[1-10]: 5 __ Why: Two consecutive Javadoc blocks exist on the same method — the first one is stale and contradicts the second. Only the last Javadoc block is associated with the method, making the first misleading dead documentation that should be removed.	Low

[github-actions]

Persistent review updated to latest commit 7b63b49

[github-actions]

Persistent review updated to latest commit a91d349

[dai-chen]

Why we need separate execute and explain logic for SQL path?

[ahkcs]

Good point, removed the duplicate methods. SQL and PPL now share the same execute() and explain(). For SQL we pass profiling=false and ExplainMode.STANDARD since SQLQueryRequest doesn't have these fields yet — should we add them to SQLQueryRequest or is this fine for now?

[dai-chen]

Could you also check EXPLAIN statement? It's supported by Calcite SQL, e.g., EXPLAIN PLAN FOR ... Ref: https://calcite.apache.org/docs/reference.html

[ahkcs]

Investigated — EXPLAIN PLAN FOR is parsed by Calcite as a SqlExplain node, but SqlToRelConverter.convertQueryRecursive() doesn't handle it (throws AssertionError("not a query: ...") in the default case). Calcite treats EXPLAIN as a meta-statement handled at the JDBC layer (CalcitePrepareImpl), not at the SqlToRelConverter level.

To support it, we'd need to detect SqlExplain in CalciteNativeStrategy.plan(), unwrap the inner query, plan it, and return the plan as a formatted result. However, the resulting plan would be identical to what
/_plugins/_sql/_explain already returns for the same query — the only difference is the response format (EXPLAIN PLAN FOR returns the plan as a query result row, /_explain returns it as a JSON explain response)

Do we want to support EXPLAIN statement?

[ahkcs]

Error log:

curl -s -X POST localhost:9200/_plugins/_sql -H 'Content-Type: application/json' -d '{"query": "EXPLAIN PLAN FOR SELECT * FROM parquet_logs"}'
{
  "error": {
    "reason": "Invalid SQL query",
    "details": "Query must start with SELECT, DELETE, SHOW or DESCRIBE: EXPLAIN PLAN FOR SELECT * FROM parquet_logs",
    "type": "SQLFeatureNotSupportedException"
  },
  "status": 400
}%                      

[dai-chen]

just wonder analyticsRouter can be null? Is this implementation different from PPL path?

[ahkcs]

Good point — analyticsRouter is always provided. Removed the no-arg constructor and null check

The implementation differs from PPL because RestSqlAction is in the legacy module which can't depend on plugin module, so we can't inject RestUnifiedQueryAction directly like PPL does in TransportPPLQueryAction. The BiFunction bridges the module boundary — it's created in SQLPlugin (plugin module) where RestUnifiedQueryAction is visible, then passed to RestSqlAction (legacy module).

[dai-chen]

Got it. I think we may want to enable profiling, at least for unified SQL path later. @penghuo