[BugFix] Fix SHOW/DESCRIBE statement routing under cluster.pluggable.dataformat setting

[dai-chen]

Description

PR #5486 introduced the cluster.pluggable.dataformat setting. When enabled, all queries are routed to the Analytics Engine, which does not support SHOW or DESCRIBE statement, causing both to fail. This PR fixes the routing by:

1. Under cluster.pluggable.dataformat=composite, SHOW TABLES, DESCRIBE, and SHOW DATASOURCES queries are now routed back to default query pipeline instead of analytics engine.
2. Legacy-syntax SHOW/DESCRIBE statements that the V2 parser rejects (e.g. unquoted SHOW TABLES LIKE %) are also routed back to the default pipeline, which falls back to the legacy engine.

Additionally, added query-routing logging at both the SQL and PPL call sites for traceability when a query is routed to the analytics engine.

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 3a8812a)

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 isLegacySystemCatalogQuery method uses case-insensitive prefix matching but does not account for leading whitespace variations beyond a single trim(). Queries with tabs, newlines, or multiple spaces before "SHOW" or "DESCRIBE" will incorrectly route to the analytics engine. For example, "\n\nSHOW TABLES" would return true from isLegacySystemCatalogQuery, but "\t\tSHOW TABLES" might not if Strings.CI.startsWith does not normalize all whitespace. private static boolean isLegacySystemCatalogQuery(String query) { String trimmed = query.trim(); return Strings.CI.startsWith(trimmed, "SHOW ") || Strings.CI.startsWith(trimmed, "DESCRIBE "); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 3a8812a

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add null check for query parameter The method doesn't handle null input, which could cause NullPointerException if query is null. Add a null check at the beginning to prevent potential crashes when called from the exception handler. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [127-130] private static boolean isLegacySystemCatalogQuery(String query) { + if (query == null) { + return false; + } String trimmed = query.trim(); return Strings.CI.startsWith(trimmed, "SHOW ") || Strings.CI.startsWith(trimmed, "DESCRIBE "); } Suggestion importance[1-10]: 7 __ Why: The null check is important since isLegacySystemCatalogQuery is called from an exception handler where query could theoretically be null. However, the calling context at line 109 already receives query from the method parameter which is checked for null at line 88, reducing the likelihood of this issue.	Medium
General	Log parsing exceptions for debugging The catch block swallows all exceptions without logging, making debugging difficult. Consider logging the exception at debug level before checking for legacy syntax to aid troubleshooting while maintaining the fallback behavior. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [107-110] if ("composite" .equals( IndicesService.CLUSTER_PLUGGABLE_DATAFORMAT_VALUE_SETTING.get( clusterService.getSettings()))) { // Analytics engine can't serve system catalog; SHOW/DESCRIBE fall back to default pipeline try (UnifiedQueryContext context = buildParsingContext(queryType)) { boolean systemCatalog = extractIndexName(query, queryType, context) .map(RestUnifiedQueryAction::isSystemCatalog) .orElse(false); return !systemCatalog; } catch (Exception e) { + LOG.debug("Failed to parse query for routing decision: {}", query, e); // Check legacy-syntax SHOW/DESCRIBE; otherwise let AE handle and surface the error. return !isLegacySystemCatalogQuery(query); } } Suggestion importance[1-10]: 5 __ Why: Adding debug logging for exceptions is a reasonable improvement for troubleshooting, but the impact is moderate since the exception is already handled appropriately with a fallback mechanism. The suggestion correctly identifies the catch block location.	Low

---

Previous suggestions

Suggestions up to commit 9468152

Category	Suggestion	Impact
General	Log exceptions before routing decision The catch block swallows all exceptions and routes to analytics engine. This could hide legitimate errors (e.g., resource exhaustion, NPE) that should be handled differently. Consider logging the exception before returning true, or only catching specific parsing-related exceptions. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [100-109] try (UnifiedQueryContext context = buildParsingContext(queryType)) { boolean systemCatalog = extractIndexName(query, queryType, context) .map(RestUnifiedQueryAction::isSystemCatalog) .orElse(false); return !systemCatalog; } catch (Exception e) { - // Let the analytics engine re-parse and surface the syntax error + LOG.debug("Failed to parse query for routing decision, delegating to analytics engine", e); return true; } Suggestion importance[1-10]: 7 __ Why: Adding logging for exceptions improves debuggability and helps distinguish between parsing errors and other failures. However, the suggestion assumes a LOG variable exists in this class, which is not shown in the PR diff. The score reflects the value of logging while accounting for this potential issue.	Medium

Suggestions up to commit 5d53ddc

Category	Suggestion	Impact
General	Log caught exceptions for debugging Catching all exceptions with a generic Exception handler can mask critical errors and make debugging difficult. Consider catching specific exceptions (e.g., parsing exceptions) or at minimum logging the exception before returning true to aid troubleshooting. plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [103-111] try (UnifiedQueryContext context = buildParsingContext(queryType)) { boolean systemCatalog = extractIndexName(query, queryType, context) .map(SystemIndexUtils::isSystemIndex) .orElse(false); return !systemCatalog; } catch (Exception e) { + // Log parsing failures for diagnostics + log.debug("Failed to parse query for routing decision, defaulting to analytics engine", e); return true; } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that catching generic Exception can mask errors. Adding logging would improve debugging. However, the comment in the code indicates this is intentional behavior (unparseable queries go to analytics engine), and the suggestion doesn't address using more specific exceptions, only adds logging.	Low

[github-actions]

Persistent review updated to latest commit 80233de

[github-actions]

Persistent review updated to latest commit 9468152

[github-actions]

Persistent review updated to latest commit 3a8812a