Skip to content

Fix SQL explain API rejecting ?format=json parameter #5607

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gingeekrishna wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix SQL explain API rejecting ?format=json parameter #5607

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
226e849
Fix SQL explain API rejecting format=json parameter
gingeekrishna Jul 5, 2026
2a4d6a4
Fix explain endpoint crash on format=json at the actual root cause
gingeekrishna Sep 2, 2026
26ce5c5
Add regression IT for explain endpoint with format=json
gingeekrishna Sep 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,11 @@ private boolean isSupportedFormat() {
}

private boolean isSupportedExplainFormat() {
return Stream.of("simple", "standard", "extended", "cost").anyMatch(format::equalsIgnoreCase);
// "json" is accepted for backward compatibility: the explain endpoint always returns JSON

@dai-chen dai-chen Sep 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 gingeekrishna Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// regardless of this parameter, so treating it as valid avoids the 400 regression
// introduced in OpenSearch 3.0. See https://github.com/opensearch-project/sql/issues/4373
return Stream.of("simple", "standard", "extended", "cost", "json")
.anyMatch(format::equalsIgnoreCase);
}

private String getFormat(Map<String, String> params) {
Expand Down
16 changes: 16 additions & 0 deletions sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,22 @@ public void should_support_explain_format() {
() -> assertTrue(explainRequest.isSupported()));
}

@Test
public void should_support_explain_with_json_format() {
// Regression test for https://github.com/opensearch-project/sql/issues/4373.
// ?format=json was accepted before OpenSearch 3.0 and should continue to be valid.
// The explain endpoint always returns JSON regardless of this parameter.
SQLQueryRequest explainRequest =
SQLQueryRequestBuilder.request("SELECT 1")
.path("_plugins/_sql/_explain")
.params(Map.of("format", "json"))
.build();

assertAll(
() -> assertTrue(explainRequest.isExplainRequest()),
() -> assertTrue(explainRequest.isSupported()));
}

@Test
public void should_not_support_explain_with_unsupported_explain_format() {
SQLQueryRequest explainRequest =
Expand Down
Loading