Fix SQL query routing to analytics engine

[finnegancarroll]

Description

SQL plugin appears to be falling back to the V2 engine instead of using the analytics engine due to the recent move to V2. It seems like when we are making our routing decision to see if we can use analytics engine we fetch a parser from the context such that we can parse the raw query and inspect the index (to determine if it's analytics plugin compatible).

However, this no longer returns a SqlNode as we are now using the SqlV2QueryParser. This causes a casting exception to be thrown and caught upstream, where we fallback to the regular SQL V2 engine.

Added a test which fails on main but passes with this PR.

Related Issues

N/A

Check List

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

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

[github-actions]

PR Code Analyzer ❗

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

Path	Line	Severity	Description
plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java	224	medium	SQL path switches from context.getParser().parse(query) to SqlParser.create(query).parseQuery() with default Calcite settings. The context parser is presumably configured with project-specific security constraints or dialect restrictions; bypassing it for index name extraction creates a parser discrepancy: the routing decision is made using a permissive default parser while execution uses the hardened context parser. An attacker could craft a query that routes to the analytics engine via the default parser but executes differently in the secure context. Warrants review to confirm no routing-relevant security controls were in the context parser.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | 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 e5c29fd)

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 Exception Handling The method signature now declares 'throws Exception', but the caller at line 178 does not handle this exception. If SqlParser.create(query).parseQuery() throws an exception (e.g., due to malformed SQL), it will propagate uncaught and likely cause the request to fail with an unhandled error instead of gracefully falling back to the V2 engine as intended. String query, QueryType queryType, UnifiedQueryContext context) throws Exception { Null Return Value The method extractTableNameFromSqlNode can return null (as seen in line 227), but there is no null check before calling toLowerCase(Locale.ROOT) on tableName. If extractTableNameFromSqlNode returns null, this will throw a NullPointerException when tableName.toLowerCase is invoked. String tableName = extractTableNameFromSqlNode(sqlNode); return Optional.ofNullable(tableName != null ? tableName.toLowerCase(Locale.ROOT) : null);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to e5c29fd
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle SQL parsing exceptions The SqlParser.create(query).parseQuery() call may throw unchecked exceptions during parsing. Wrap this in a try-catch block to handle parsing failures gracefully and prevent unexpected crashes when processing malformed SQL queries. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [225-227] -SqlNode sqlNode = SqlParser.create(query).parseQuery(); -String tableName = extractTableNameFromSqlNode(sqlNode); -return Optional.ofNullable(tableName != null ? tableName.toLowerCase(Locale.ROOT) : null); +try { + SqlNode sqlNode = SqlParser.create(query).parseQuery(); + String tableName = extractTableNameFromSqlNode(sqlNode); + return Optional.ofNullable(tableName != null ? tableName.toLowerCase(Locale.ROOT) : null); +} catch (Exception e) { + LOG.error("Failed to parse SQL query: {}", query, e); + return Optional.empty(); +} Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that SqlParser.create(query).parseQuery() can throw exceptions and proposes wrapping it in a try-catch block. However, since the method signature already declares throws Exception, this is more about defensive programming than fixing a bug. The score reflects its value as an error handling improvement.	Medium
General	Simplify optional null handling The null check is redundant since Optional.ofNullable() already handles null values. Simplify by applying toLowerCase() within Optional.map() to make the code more concise and idiomatic. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [226-227] String tableName = extractTableNameFromSqlNode(sqlNode); -return Optional.ofNullable(tableName != null ? tableName.toLowerCase(Locale.ROOT) : null); +return Optional.ofNullable(tableName).map(name -> name.toLowerCase(Locale.ROOT)); Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies redundant null checking and proposes a more idiomatic approach using Optional.map(). While valid, this is a minor code style improvement that enhances readability without fixing any functional issues.	Low

---

Previous suggestions

Suggestions up to commit b70eeb0

Category	Suggestion	Impact
General	Simplify redundant null check The null check and ternary operation are redundant since Optional.ofNullable() already handles null values. Simplify by applying toLowerCase() only when the table name is non-null using Optional.map(). plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [225-227] SqlNode sqlNode = SqlParser.create(query).parseQuery(); String tableName = extractTableNameFromSqlNode(sqlNode); -return Optional.ofNullable(tableName != null ? tableName.toLowerCase(Locale.ROOT) : null); +return Optional.ofNullable(tableName).map(name -> name.toLowerCase(Locale.ROOT)); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies redundant null handling. Using Optional.map() is cleaner and more idiomatic than the ternary operator with Optional.ofNullable(). This improves code readability and maintainability without changing functionality.	Medium

[github-actions]

Persistent review updated to latest commit e5c29fd

[finnegancarroll]

Fix in: #5456