Fix SQL query routing to analytics engine after V2 parser change

[dai-chen]

Description

The switch from Calcite parser to SQL V2 parser in PR #5438 changed the SQL parse result type from SqlNode to UnresolvedPlan. This broke extractIndexName() in RestUnifiedQueryAction and caused all SQL queries to fall through to the V2 engine instead of routing to the analytics engine.

TODO: Need to add sanity IT with analytics engine once stable.

Related Issues

Part of #5248

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

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 Possible Issue The extractIndexName() method now unconditionally casts the parse result to UnresolvedPlan for both PPL and SQL queries. If the V2 parser returns a different type for SQL queries (e.g., for SHOW/DESCRIBE statements or malformed queries), this will throw a ClassCastException at runtime. The removed code had separate handling for SQL vs PPL query types, suggesting they may produce different parse result types. UnresolvedPlan unresolvedPlan = (UnresolvedPlan) context.getParser().parse(query);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to a58f5ae

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle potential ClassCastException safely The unchecked cast to UnresolvedPlan can throw ClassCastException if the parser returns a different type for SQL queries. Wrap the cast in a try-catch block or verify the type before casting to prevent runtime exceptions. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [210-211] -UnresolvedPlan unresolvedPlan = (UnresolvedPlan) context.getParser().parse(query); -return Optional.ofNullable(unresolvedPlan.accept(new IndexNameExtractor(), null)); +try { + UnresolvedPlan unresolvedPlan = (UnresolvedPlan) context.getParser().parse(query); + return Optional.ofNullable(unresolvedPlan.accept(new IndexNameExtractor(), null)); +} catch (ClassCastException e) { + return Optional.empty(); +} Suggestion importance[1-10]: 8 __ Why: The unchecked cast to UnresolvedPlan is risky, especially since the PR removes SQL-specific handling. The test cases show SQL queries are now processed through this path, which could cause ClassCastException if the parser returns a different type for SQL queries.	Medium
General	Remove unused queryType parameter The method now ignores the queryType parameter but still accepts it. This creates confusion and potential bugs if callers expect different behavior based on query type. Either remove the unused parameter or add logic to handle SQL queries differently if needed. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [208-212] private static Optional<String> extractIndexName( - String query, QueryType queryType, UnifiedQueryContext context) { + String query, UnifiedQueryContext context) { UnresolvedPlan unresolvedPlan = (UnresolvedPlan) context.getParser().parse(query); return Optional.ofNullable(unresolvedPlan.accept(new IndexNameExtractor(), null)); } Suggestion importance[1-10]: 7 __ Why: The queryType parameter is indeed unused in the method body after the PR changes, which can lead to confusion. However, removing it would require updating all call sites, and the parameter might be intentionally kept for API consistency or future use.	Medium

---

Previous suggestions

Suggestions up to commit 74f3f80

Category	Suggestion	Impact
Possible issue	Handle parsing exceptions gracefully Wrap the parsing logic in a try-catch block to handle potential parsing exceptions. If parsing fails (e.g., for SHOW or DESCRIBE statements), the method should return Optional.empty() instead of propagating the exception. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [208-212] private static Optional<String> extractIndexName( String query, QueryType queryType, UnifiedQueryContext context) { - UnresolvedPlan unresolvedPlan = (UnresolvedPlan) context.getParser().parse(query); - return Optional.ofNullable(unresolvedPlan.accept(new IndexNameExtractor(), null)); + try { + UnresolvedPlan unresolvedPlan = (UnresolvedPlan) context.getParser().parse(query); + return Optional.ofNullable(unresolvedPlan.accept(new IndexNameExtractor(), null)); + } catch (Exception e) { + return Optional.empty(); + } } Suggestion importance[1-10]: 8 __ Why: This is a critical improvement. The tests show that SHOW and DESCRIBE statements should route to Lucene, which likely means they fail to parse as UnresolvedPlan. Without exception handling, these queries would cause the application to crash rather than gracefully falling back to Lucene routing.	Medium
	Add null check before parsing Add null check for query parameter before parsing to prevent potential NullPointerException. The method should handle null or empty queries gracefully, as evidenced by the test cases that verify this behavior. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [208-212] private static Optional<String> extractIndexName( String query, QueryType queryType, UnifiedQueryContext context) { + if (query == null || query.isEmpty()) { + return Optional.empty(); + } UnresolvedPlan unresolvedPlan = (UnresolvedPlan) context.getParser().parse(query); return Optional.ofNullable(unresolvedPlan.accept(new IndexNameExtractor(), null)); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the method should handle null/empty queries gracefully, as verified by test cases. Adding this check prevents potential NullPointerException during parsing and aligns with the test expectations for null/empty query handling.	Medium

Suggestions up to commit a9ad0bf

Category	Suggestion	Impact
Possible issue	Add null/empty query validation Add null/empty query validation before parsing to prevent potential NullPointerException or parsing errors. The test cases show that null and empty queries should be handled gracefully, but the current implementation attempts to parse them directly. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [210-211] private static Optional<String> extractIndexName( String query, QueryType queryType, UnifiedQueryContext context) { + if (query == null || query.isEmpty()) { + return Optional.empty(); + } UnresolvedPlan unresolvedPlan = (UnresolvedPlan) context.getParser().parse(query); return Optional.ofNullable(unresolvedPlan.accept(new IndexNameExtractor(), null)); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that null/empty queries should be validated before parsing, as evidenced by the test cases. This prevents potential parsing errors and aligns with the expected behavior shown in nullAndEmptyQueriesRouteToLucene() test.	Medium
	Handle potential ClassCastException during parsing Wrap the parsing and casting operations in a try-catch block to handle potential ClassCastException if the parser returns an unexpected type. This is especially important since SQL queries are now being parsed through the same path that was previously PPL-only. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [210-211] -UnresolvedPlan unresolvedPlan = (UnresolvedPlan) context.getParser().parse(query); -return Optional.ofNullable(unresolvedPlan.accept(new IndexNameExtractor(), null)); +try { + UnresolvedPlan unresolvedPlan = (UnresolvedPlan) context.getParser().parse(query); + return Optional.ofNullable(unresolvedPlan.accept(new IndexNameExtractor(), null)); +} catch (ClassCastException e) { + return Optional.empty(); +} Suggestion importance[1-10]: 3 __ Why: While error handling is generally good practice, the PR changes suggest that the parser is expected to handle both PPL and SQL queries uniformly through the UnresolvedPlan path. The removal of SQL-specific logic and addition of SQL test cases indicate this is intentional design, making a ClassCastException unlikely in normal operation.	Low

[github-actions]

Persistent review updated to latest commit 74f3f80

[github-actions]

Persistent review updated to latest commit a58f5ae