Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,13 @@
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_NESTED_TYPE;
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_PEOPLE;
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_PHRASE;
import static org.opensearch.sql.legacy.plugin.RestSqlAction.EXPLAIN_API_ENDPOINT;

import com.google.common.io.Files;
import java.io.File;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import org.json.JSONObject;
import org.junit.Assert;
import org.junit.Ignore;
import org.junit.Test;
Expand Down Expand Up @@ -253,4 +255,25 @@ public void testContentTypeOfExplainRequestShouldBeJson() throws IOException {

assertEquals("application/json; charset=UTF-8", response.getHeader("content-type"));
}

/**
* Prior to OpenSearch 3.0, {@code ?format=json} was a valid way to request the explain plan.
* "json" was never a real {@code Format} value the query endpoint accepts (only
* jdbc/csv/raw/table are), so this depends on the explain endpoint specifically tolerating it.
* Regression test for https://github.com/opensearch-project/sql/issues/4373: this used to fail
* with "Failed to create executor due to unknown response format: json" before the explain
* endpoint reached any query-specific logic at all.
*/
@Test
public void testExplainAcceptsJsonFormatForBackwardCompatibility() throws IOException {
String query = makeRequest("SELECT firstname FROM opensearch-sql_test_index_account");
Request request = new Request("POST", EXPLAIN_API_ENDPOINT + "?format=json");
request.setJsonEntity(query);

Response response = client().performRequest(request);

assertEquals(200, response.getStatusLine().getStatusCode());
JSONObject explanation = new JSONObject(TestUtils.getResponseBody(response));
Assert.assertFalse("explain response should not contain an error", explanation.has("error"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli

LOG.info("[{}] Incoming request {}", QueryContext.getRequestId(), request.uri());

Format format = SqlRequestParam.getFormat(request.params());
Format format = resolveFormat(request);

SQLQueryRequest newSqlRequest =
new SQLQueryRequest(
Expand Down Expand Up @@ -320,6 +320,29 @@ private static boolean isExplainRequest(final RestRequest request) {
return request.path().endsWith("/_explain");
}

/**
* Resolve the response {@link Format} for this request.
*
* <p>{@code format=json} is accepted for explain requests for backward compatibility: prior to
* OpenSearch 3.0, {@code ?format=json} was a valid way to request the explain plan (see #4373).
* It was never a real member of {@link Format} - {@code SqlRequestParam#getFormat} always
* rejected it - but the explain response body is JSON regardless of this parameter (see {@link
* #executeSqlRequest}, which calls {@code queryAction.explain().explain()} directly instead of
* going through a {@link Format}-specific executor), so the parameter can simply be ignored here
* for explain requests without changing any actual behavior.
*/
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;
}
}

private static boolean isClientError(Exception e) {
return e
instanceof
Expand Down
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

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.

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