Fix SQL explain API rejecting ?format=json parameter

[gingeekrishna]

Description

Fixes #4373

Problem: Since OpenSearch 3.0, calling POST /_plugins/_sql/_explain?format=json returns:

{
  "error": {
    "reason": "Invalid SQL query",
    "details": "Failed to create executor due to unknown response format: json",
    "type": "IllegalArgumentException"
  },
  "status": 400
}

Root cause (corrected): My first attempt at this fix (still included below) only added "json" to isSupportedExplainFormat() in SQLQueryRequest.java, the V2/sql-module engine's own validation. That turned out to be insufficient - that method is never reached for this bug. RestSqlAction#prepareRequest calls SqlRequestParam.getFormat(request.params()) unconditionally, for every request, before SQLQueryRequest is even constructed. That call throws IllegalArgumentException for any format outside the legacy Format enum (jdbc/csv/raw/table) - "json" included - which is exactly the error above. So the request was failing before either engine (legacy or V2) ever got a chance to look at it.

Fix:

1. RestSqlAction#resolveFormat (new): when SqlRequestParam.getFormat rejects the value AND the request is an explain request AND the rejected value is "json", fall back to a default Format instead of throwing. The explain response body is JSON regardless of this parameter (executeSqlRequest's explain branch calls queryAction.explain().explain() directly, never through a Format-specific executor), so ignoring it here changes nothing else about the response.
2. SQLQueryRequest#isSupportedExplainFormat() (kept from the original attempt): once a request gets past (1), this makes the V2 engine treat format=json as a supported explain format, so it handles the explain natively instead of always falling back to the legacy engine.

Changes

• legacy/src/main/java/.../plugin/RestSqlAction.java — add resolveFormat(), the actual fix for the reported crash
• sql/src/main/java/.../SQLQueryRequest.java — add "json" to isSupportedExplainFormat() (keeps the V2 engine as the primary path once the format-parsing crash is fixed)
• sql/src/test/java/.../SQLQueryRequestTest.java — unit test for isSupportedExplainFormat()
• integ-test/.../legacy/ExplainIT.java — integration test hitting the real /_explain?format=json endpoint end-to-end (per review - a unit test of isSupportedExplainFormat() alone doesn't exercise RestSqlAction, so it wouldn't have caught that the real bug was one layer up)

Testing

• SQLQueryRequestTest: unit coverage for the V2 engine's format validation.
• ExplainIT#testExplainAcceptsJsonFormatForBackwardCompatibility: hits POST /_plugins/_sql/_explain?format=json for real and asserts a 200 with no error key - this is the test that actually exercises the code path the bug lived in.

Note: I don't have a Java/Gradle toolchain available in my current environment, so I couldn't run these locally - would appreciate CI confirming.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 26ce5c5)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[gingeekrishna]

The enforce-label check is failing — a release label is required. Since this PR improves the error message for unsupported window functions (a UX/error-experience fix), the bugFix label would be appropriate. Could a maintainer add it? Thanks!

[dai-chen]

Could you add yaml IT to verify and this may be moved there as context? Ref: https://github.com/opensearch-project/sql/blob/main/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5702.yml

[gingeekrishna]

Thanks for asking for this - it turned up something important. While putting together an integration test I traced the actual request flow and found the original fix (adding "json" to isSupportedExplainFormat()) doesn't actually fix the bug: that method is in SQLQueryRequest (the V2 engine), but RestSqlAction#prepareRequest calls SqlRequestParam.getFormat(request.params()) unconditionally, for every request, before SQLQueryRequest is ever constructed. That call throws for any format outside jdbc/csv/raw/table - "json" included - which is exactly the "Failed to create executor due to unknown response format: json" error from #4373. So the original fix was never reached; the request was already failing a layer up.

Pushed 2a4d6a4c0, which adds the real fix in RestSqlAction (falls back instead of throwing when the rejected value is "json" on an explain request), and kept the original SQLQueryRequest change since it still matters once that's fixed (makes the V2 engine handle it natively instead of always falling back to legacy).

For the test: there's no YAML REST-spec coverage for any SQL endpoint yet (only ppl/ppl.explain/ppl.grammar/query.settings action specs exist), so introducing a new sql.explain action spec felt like a lot of new surface for one regression test. Added 26ce5c514 instead, an integration test in legacy/ExplainIT.java (SQL's existing IT pattern) that hits POST /_plugins/_sql/_explain?format=json for real. It's the test that would have actually caught this - my original unit test on isSupportedExplainFormat() alone gave false confidence since it never touched RestSqlAction at all. Let me know if you'd still prefer a YAML test / new action spec added instead.

[github-actions]

Persistent review updated to latest commit 26ce5c5

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Avoid catching broad exceptions The method catches IllegalArgumentException broadly, which could mask other parameter validation errors unrelated to the format parameter. Consider catching the exception only when it's specifically about an invalid format value, or validate the format parameter before calling getFormat(). legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java [334-344] private static Format resolveFormat(final RestRequest request) { - try { - return SqlRequestParam.getFormat(request.params()); - } catch (IllegalArgumentException e) { - if (isExplainRequest(request) - && "json".equalsIgnoreCase(request.param(SqlRequestParam.QUERY_PARAMS_FORMAT))) { - return Format.JDBC; - } - throw e; + String formatParam = request.param(SqlRequestParam.QUERY_PARAMS_FORMAT); + if (isExplainRequest(request) && "json".equalsIgnoreCase(formatParam)) { + return Format.JDBC; } + return SqlRequestParam.getFormat(request.params()); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the current implementation catches IllegalArgumentException broadly, which could mask other validation errors. The improved code validates the format parameter upfront for explain requests with "json" format, avoiding the exception handling altogether. This is a cleaner approach that improves code clarity and error handling specificity.	Medium