From bd434ec5105b90491c1eb0f7129b606655465919 Mon Sep 17 00:00:00 2001 From: Sean Kao Date: Wed, 26 Feb 2025 09:14:40 -0800 Subject: [PATCH 01/21] mark places to change Signed-off-by: Sean Kao --- docs/user/interfaces/protocol.rst | 1 + .../test/java/org/opensearch/sql/legacy/ExplainIT.java | 1 + .../org/opensearch/sql/legacy/GetEndpointQueryIT.java | 1 + .../opensearch/sql/legacy/OpenSearchSQLRestTestCase.java | 1 + .../java/org/opensearch/sql/legacy/SQLIntegTestCase.java | 8 ++++++++ .../legacy/executor/ActionRequestRestExecutorFactory.java | 2 +- .../java/org/opensearch/sql/legacy/executor/Format.java | 2 +- .../sql/legacy/query/OpenSearchActionFactory.java | 2 ++ .../sql/legacy/unittest/SqlRequestParamTest.java | 1 + .../unittest/planner/OpenSearchActionFactoryTest.java | 1 + .../sql/legacy/unittest/query/DefaultQueryActionTest.java | 1 + 11 files changed, 19 insertions(+), 2 deletions(-) diff --git a/docs/user/interfaces/protocol.rst b/docs/user/interfaces/protocol.rst index 52674b99244..24e803ce97c 100644 --- a/docs/user/interfaces/protocol.rst +++ b/docs/user/interfaces/protocol.rst @@ -208,6 +208,7 @@ Result set:: "status" : 400 } +// TODO: deprecate json OpenSearch DSL ============== diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java index 1b740924c44..2059c846244 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java @@ -245,6 +245,7 @@ public void explainNLJoin() throws IOException { result.replaceAll("\\s+", ""), equalTo(expectedOutput.replaceAll("\\s+", ""))); } + // TODO: is this test running without @Test? public void testContentTypeOfExplainRequestShouldBeJson() throws IOException { String query = makeRequest("SELECT firstname FROM opensearch-sql_test_index_account"); Request request = getSqlRequest(query, true); diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java index 6cc4aba8118..a74ddde65f7 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java @@ -13,6 +13,7 @@ import org.junit.rules.ExpectedException; import org.opensearch.client.ResponseException; +// TODO: inaccurate comment? Find out purpose of this test /** Tests to cover requests with "?format=csv" parameter */ public class GetEndpointQueryIT extends SQLIntegTestCase { diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java index ced69d54a07..6b57310a9a4 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java @@ -181,6 +181,7 @@ protected static void wipeAllOpenSearchIndices() throws IOException { } } + // TODO: deprecate json protected static void wipeAllOpenSearchIndices(RestClient client) throws IOException { // include all the indices, included hidden indices. // https://www.elastic.co/guide/en/elasticsearch/reference/current/cat-indices.html#cat-indices-api-query-params diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java index f59fe82fa0c..213401ec526 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java @@ -228,6 +228,8 @@ protected synchronized void loadIndex(Index index) throws IOException { loadIndex(index, client()); } + // TODO: deprecate json + // Note that explain queries are still supposed to return json protected Request getSqlRequest(String request, boolean explain) { return getSqlRequest(request, explain, "json"); } @@ -266,10 +268,12 @@ protected void assertBadRequest(Callable operation) { } } + // DONE: verified all usages have requestType="jdbc" except in CsvFormatIT and RawFormatIT protected String executeQuery(String query, String requestType) { return executeQuery(query, requestType, Map.of()); } + // DONE: verified usages don't contain requestType="json" protected String executeQuery(String query, String requestType, Map params) { try { String endpoint = "/_plugins/_sql?format=" + requestType; @@ -293,6 +297,7 @@ protected JSONObject executeJdbcRequest(String query) { return new JSONObject(executeQuery(query, "jdbc")); } + // DONE: verified usages don't contain requestType="json" protected String executeFetchQuery(String query, int fetchSize, String requestType) throws IOException { String endpoint = "/_plugins/_sql?format=" + requestType; @@ -317,6 +322,7 @@ protected JSONObject executeQueryTemplate(String queryTemplate, String index) th return executeQueryTemplate(queryTemplate, index, 4); } + // DONE: verified usages don't contain requestType="json" protected String executeFetchLessQuery(String query, String requestType) throws IOException { String endpoint = "/_plugins/_sql?format=" + requestType; @@ -342,6 +348,7 @@ protected Request buildGetEndpointRequest(final String sqlQuery) { Assert.fail(utf8CharsetName + " not available"); } + // TODO: deprecate json final String requestUrl = String.format( Locale.ROOT, "%s?sql=%s&format=%s", QUERY_API_ENDPOINT, urlEncodedQuery, "json"); @@ -394,6 +401,7 @@ protected static String executeRequest(final Request request) throws IOException return executeRequest(request, client()); } + // TODO: this returns json. Only used in GetEndpointQueryIT protected JSONObject executeQueryWithGetRequest(final String sqlQuery) throws IOException { final Request request = buildGetEndpointRequest(sqlQuery); diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java index c58bba9e26a..8c6009248d3 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java @@ -25,7 +25,7 @@ public static RestExecutor createExecutor(Format format, QueryAction queryAction switch (format) { case CSV: return new AsyncRestExecutor(new CSVResultRestExecutor()); - case JSON: + case JSON: // TODO: deprecate json return new AsyncRestExecutor( new ElasticDefaultRestExecutor(queryAction), action -> isJoin(action) || isUnionMinus(action)); diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java index c47092f10b5..f17e2a1deda 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java @@ -14,7 +14,7 @@ @RequiredArgsConstructor public enum Format { JDBC("jdbc"), - JSON("json"), + JSON("json"), // TODO: deprecate json CSV("csv"), RAW("raw"), TABLE("table"); diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java index 49aa38ae581..3924b6e41f6 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java @@ -63,6 +63,7 @@ public class OpenSearchActionFactory { public static QueryAction create(Client client, String sql) throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { + // TODO: deprecate json return create(client, new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JSON)); } @@ -192,6 +193,7 @@ private static boolean isJoin(SQLQueryExpr sqlExpr, String sql) { @VisibleForTesting public static boolean shouldMigrateToQueryPlan(SQLQueryExpr expr, Format format) { + // TODO: deprecate json // The JSON format will return the OpenSearch aggregation result, which is not supported by the // QueryPlanner. if (format == Format.JSON) { diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java index 0d29b55106f..b35ee4c99d5 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java @@ -41,6 +41,7 @@ public void shouldReturnFalseIfPrettyParamsIsUnknownValue() { assertFalse(SqlRequestParam.isPrettyFormat(ImmutableMap.of(QUERY_PARAMS_PRETTY, "unknown"))); } + // TODO: deprecate json @Test public void shouldReturnJSONIfFormatParamsIsJSON() { assertEquals( diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java index 3443c2decd9..9b550f379cc 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java @@ -14,6 +14,7 @@ import org.opensearch.sql.legacy.util.SqlParserUtils; public class OpenSearchActionFactoryTest { + // TODO: deprecate json @Test public void josnOutputRequestShouldNotMigrateToQueryPlan() { String sql = "SELECT age, MAX(balance) FROM account GROUP BY age"; diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java index d290e4dd5b0..5471e37dd42 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java @@ -130,6 +130,7 @@ public void testIfScrollShouldBeOpenWithDifferentFormats() { doReturn(settingFetchSize).when(mockSqlRequest).fetchSize(); queryAction.setSqlRequest(mockSqlRequest); + // TODO: deprecate json Format[] formats = new Format[] {Format.CSV, Format.RAW, Format.JSON, Format.TABLE}; for (Format format : formats) { queryAction.setFormat(format); From 613da7c026c6b038af1951ab5489b6f87fe0bc08 Mon Sep 17 00:00:00 2001 From: Sean Kao Date: Thu, 27 Feb 2025 09:25:35 -0800 Subject: [PATCH 02/21] remove dsl format from doc Signed-off-by: Sean Kao --- docs/user/interfaces/protocol.rst | 68 ------------------------------- 1 file changed, 68 deletions(-) diff --git a/docs/user/interfaces/protocol.rst b/docs/user/interfaces/protocol.rst index 24e803ce97c..625eed0f544 100644 --- a/docs/user/interfaces/protocol.rst +++ b/docs/user/interfaces/protocol.rst @@ -208,74 +208,6 @@ Result set:: "status" : 400 } -// TODO: deprecate json -OpenSearch DSL -============== - -Description ------------ - -The plugin returns original response from OpenSearch in JSON. Because this is the native response from OpenSearch, extra efforts are needed to parse and interpret it. - -Example -------- - -SQL query:: - - >> curl -H 'Content-Type: application/json' -X POST localhost:9200/_plugins/_sql?format=json -d '{ - "query" : "SELECT firstname, lastname, age FROM accounts ORDER BY age LIMIT 2" - }' - -Result set:: - - { - "_shards" : { - "total" : 5, - "failed" : 0, - "successful" : 5, - "skipped" : 0 - }, - "hits" : { - "hits" : [ - { - "_index" : "accounts", - "_type" : "_doc", - "_source" : { - "firstname" : "Nanette", - "age" : 28, - "lastname" : "Bates" - }, - "_id" : "13", - "sort" : [ - 28 - ], - "_score" : null - }, - { - "_index" : "accounts", - "_type" : "_doc", - "_source" : { - "firstname" : "Amber", - "age" : 32, - "lastname" : "Duke" - }, - "_id" : "1", - "sort" : [ - 32 - ], - "_score" : null - } - ], - "total" : { - "value" : 4, - "relation" : "eq" - }, - "max_score" : null - }, - "took" : 100, - "timed_out" : false - } - CSV Format ========== From 43fe0fa9cc54538fb38e90382eb9c91f9a88219c Mon Sep 17 00:00:00 2001 From: Sean Kao Date: Thu, 27 Feb 2025 11:56:21 -0800 Subject: [PATCH 03/21] remove Format.JSON Signed-off-by: Sean Kao --- .../org/opensearch/sql/legacy/ExplainIT.java | 1 - .../sql/legacy/GetEndpointQueryIT.java | 2 -- .../sql/legacy/OpenSearchSQLRestTestCase.java | 1 - .../sql/legacy/SQLIntegTestCase.java | 12 ++---------- .../ActionRequestRestExecutorFactory.java | 15 +-------------- .../sql/legacy/executor/Format.java | 1 - .../sql/legacy/plugin/RestSqlAction.java | 3 +-- .../legacy/query/OpenSearchActionFactory.java | 13 +++---------- .../legacy/unittest/SqlRequestParamTest.java | 7 ------- .../planner/OpenSearchActionFactoryTest.java | 19 +++++-------------- .../query/DefaultQueryActionTest.java | 3 +-- 11 files changed, 13 insertions(+), 64 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java index 2059c846244..1b740924c44 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java @@ -245,7 +245,6 @@ public void explainNLJoin() throws IOException { result.replaceAll("\\s+", ""), equalTo(expectedOutput.replaceAll("\\s+", ""))); } - // TODO: is this test running without @Test? public void testContentTypeOfExplainRequestShouldBeJson() throws IOException { String query = makeRequest("SELECT firstname FROM opensearch-sql_test_index_account"); Request request = getSqlRequest(query, true); diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java index a74ddde65f7..6831dcfcc68 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java @@ -13,8 +13,6 @@ import org.junit.rules.ExpectedException; import org.opensearch.client.ResponseException; -// TODO: inaccurate comment? Find out purpose of this test -/** Tests to cover requests with "?format=csv" parameter */ public class GetEndpointQueryIT extends SQLIntegTestCase { @Rule public final ExpectedException rule = ExpectedException.none(); diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java index 6b57310a9a4..ced69d54a07 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java @@ -181,7 +181,6 @@ protected static void wipeAllOpenSearchIndices() throws IOException { } } - // TODO: deprecate json protected static void wipeAllOpenSearchIndices(RestClient client) throws IOException { // include all the indices, included hidden indices. // https://www.elastic.co/guide/en/elasticsearch/reference/current/cat-indices.html#cat-indices-api-query-params diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java index 213401ec526..a9179a7b820 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java @@ -228,10 +228,8 @@ protected synchronized void loadIndex(Index index) throws IOException { loadIndex(index, client()); } - // TODO: deprecate json - // Note that explain queries are still supposed to return json protected Request getSqlRequest(String request, boolean explain) { - return getSqlRequest(request, explain, "json"); + return getSqlRequest(request, explain, "jdbc"); } protected Request getSqlRequest(String request, boolean explain, String requestType) { @@ -268,12 +266,10 @@ protected void assertBadRequest(Callable operation) { } } - // DONE: verified all usages have requestType="jdbc" except in CsvFormatIT and RawFormatIT protected String executeQuery(String query, String requestType) { return executeQuery(query, requestType, Map.of()); } - // DONE: verified usages don't contain requestType="json" protected String executeQuery(String query, String requestType, Map params) { try { String endpoint = "/_plugins/_sql?format=" + requestType; @@ -297,7 +293,6 @@ protected JSONObject executeJdbcRequest(String query) { return new JSONObject(executeQuery(query, "jdbc")); } - // DONE: verified usages don't contain requestType="json" protected String executeFetchQuery(String query, int fetchSize, String requestType) throws IOException { String endpoint = "/_plugins/_sql?format=" + requestType; @@ -322,7 +317,6 @@ protected JSONObject executeQueryTemplate(String queryTemplate, String index) th return executeQueryTemplate(queryTemplate, index, 4); } - // DONE: verified usages don't contain requestType="json" protected String executeFetchLessQuery(String query, String requestType) throws IOException { String endpoint = "/_plugins/_sql?format=" + requestType; @@ -348,10 +342,9 @@ protected Request buildGetEndpointRequest(final String sqlQuery) { Assert.fail(utf8CharsetName + " not available"); } - // TODO: deprecate json final String requestUrl = String.format( - Locale.ROOT, "%s?sql=%s&format=%s", QUERY_API_ENDPOINT, urlEncodedQuery, "json"); + Locale.ROOT, "%s?sql=%s&format=%s", QUERY_API_ENDPOINT, urlEncodedQuery, "jdbc"); return new Request("GET", requestUrl); } @@ -401,7 +394,6 @@ protected static String executeRequest(final Request request) throws IOException return executeRequest(request, client()); } - // TODO: this returns json. Only used in GetEndpointQueryIT protected JSONObject executeQueryWithGetRequest(final String sqlQuery) throws IOException { final Request request = buildGetEndpointRequest(sqlQuery); diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java index 8c6009248d3..a585c3c2cb7 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java @@ -18,17 +18,12 @@ public class ActionRequestRestExecutorFactory { * call if necessary. * * @param format format of response - * @param queryAction query action * @return executor */ - public static RestExecutor createExecutor(Format format, QueryAction queryAction) { + public static RestExecutor createExecutor(Format format) { switch (format) { case CSV: return new AsyncRestExecutor(new CSVResultRestExecutor()); - case JSON: // TODO: deprecate json - return new AsyncRestExecutor( - new ElasticDefaultRestExecutor(queryAction), - action -> isJoin(action) || isUnionMinus(action)); case JDBC: case RAW: case TABLE: @@ -36,12 +31,4 @@ public static RestExecutor createExecutor(Format format, QueryAction queryAction return new AsyncRestExecutor(new PrettyFormatRestExecutor(format.getFormatName())); } } - - private static boolean isJoin(QueryAction queryAction) { - return queryAction instanceof OpenSearchJoinQueryAction; - } - - private static boolean isUnionMinus(QueryAction queryAction) { - return queryAction instanceof MultiQueryAction; - } } diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java index f17e2a1deda..b3060275452 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java @@ -14,7 +14,6 @@ @RequiredArgsConstructor public enum Format { JDBC("jdbc"), - JSON("json"), // TODO: deprecate json CSV("csv"), RAW("raw"), TABLE("table"); diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java index a0e68ff54be..9be2367dcaa 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java @@ -251,8 +251,7 @@ private void executeSqlRequest( channel.sendResponse(new BytesRestResponse(OK, "application/json; charset=UTF-8", result)); } else { RestExecutor restExecutor = - ActionRequestRestExecutorFactory.createExecutor( - SqlRequestParam.getFormat(params), queryAction); + ActionRequestRestExecutorFactory.createExecutor(SqlRequestParam.getFormat(params)); // doing this hack because OpenSearch throws exception for un-consumed props Map additionalParams = new HashMap<>(); for (String paramName : responseParams()) { diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java index 3924b6e41f6..93e721e52d4 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java @@ -63,8 +63,7 @@ public class OpenSearchActionFactory { public static QueryAction create(Client client, String sql) throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { - // TODO: deprecate json - return create(client, new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JSON)); + return create(client, new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JDBC)); } /** @@ -117,7 +116,7 @@ public static QueryAction create(Client client, QueryActionRequest request) } else { sqlExpr.accept(new TermFieldRewriter()); // migrate aggregation to query planner framework. - if (shouldMigrateToQueryPlan(sqlExpr, request.getFormat())) { + if (shouldMigrateToQueryPlan(sqlExpr)) { return new QueryPlanQueryAction( new QueryPlanRequestBuilder( new BindingTupleQueryPlanner(client, sqlExpr, request.getTypeProvider()))); @@ -192,13 +191,7 @@ private static boolean isJoin(SQLQueryExpr sqlExpr, String sql) { } @VisibleForTesting - public static boolean shouldMigrateToQueryPlan(SQLQueryExpr expr, Format format) { - // TODO: deprecate json - // The JSON format will return the OpenSearch aggregation result, which is not supported by the - // QueryPlanner. - if (format == Format.JSON) { - return false; - } + public static boolean shouldMigrateToQueryPlan(SQLQueryExpr expr) { QueryPlannerScopeDecider decider = new QueryPlannerScopeDecider(); return decider.isInScope(expr); } diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java index b35ee4c99d5..ee2f3ac74a7 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java @@ -41,13 +41,6 @@ public void shouldReturnFalseIfPrettyParamsIsUnknownValue() { assertFalse(SqlRequestParam.isPrettyFormat(ImmutableMap.of(QUERY_PARAMS_PRETTY, "unknown"))); } - // TODO: deprecate json - @Test - public void shouldReturnJSONIfFormatParamsIsJSON() { - assertEquals( - Format.JSON, SqlRequestParam.getFormat(ImmutableMap.of(QUERY_PARAMS_FORMAT, "json"))); - } - @Test public void shouldReturnDefaultFormatIfNoFormatParams() { assertEquals(Format.JDBC, SqlRequestParam.getFormat(ImmutableMap.of())); diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java index 9b550f379cc..44ad735ac7f 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java @@ -14,21 +14,12 @@ import org.opensearch.sql.legacy.util.SqlParserUtils; public class OpenSearchActionFactoryTest { - // TODO: deprecate json - @Test - public void josnOutputRequestShouldNotMigrateToQueryPlan() { - String sql = "SELECT age, MAX(balance) FROM account GROUP BY age"; - - assertFalse( - OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql), Format.JSON)); - } - @Test public void nestQueryShouldNotMigrateToQueryPlan() { String sql = "SELECT age, nested(balance) FROM account GROUP BY age"; assertFalse( - OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql), Format.JDBC)); + OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); } @Test @@ -36,7 +27,7 @@ public void nonAggregationQueryShouldNotMigrateToQueryPlan() { String sql = "SELECT age FROM account "; assertFalse( - OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql), Format.JDBC)); + OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); } @Test @@ -44,7 +35,7 @@ public void aggregationQueryWithoutGroupByShouldMigrateToQueryPlan() { String sql = "SELECT age, COUNT(balance) FROM account "; assertTrue( - OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql), Format.JDBC)); + OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); } @Test @@ -52,7 +43,7 @@ public void aggregationQueryWithExpressionByShouldMigrateToQueryPlan() { String sql = "SELECT age, MAX(balance) - MIN(balance) FROM account "; assertTrue( - OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql), Format.JDBC)); + OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); } @Test @@ -60,6 +51,6 @@ public void queryOnlyHasGroupByShouldMigrateToQueryPlan() { String sql = "SELECT CAST(age AS DOUBLE) as alias FROM account GROUP BY alias"; assertTrue( - OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql), Format.JDBC)); + OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); } } diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java index 5471e37dd42..bf5d79855c5 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java @@ -130,8 +130,7 @@ public void testIfScrollShouldBeOpenWithDifferentFormats() { doReturn(settingFetchSize).when(mockSqlRequest).fetchSize(); queryAction.setSqlRequest(mockSqlRequest); - // TODO: deprecate json - Format[] formats = new Format[] {Format.CSV, Format.RAW, Format.JSON, Format.TABLE}; + Format[] formats = new Format[] {Format.CSV, Format.RAW, Format.TABLE}; for (Format format : formats) { queryAction.setFormat(format); queryAction.checkAndSetScroll(); From e3157aa37ac977c14ca37aa9156d055475d6e32c Mon Sep 17 00:00:00 2001 From: Sean Kao Date: Fri, 28 Feb 2025 08:25:00 -0800 Subject: [PATCH 04/21] fix test case wip Signed-off-by: Sean Kao --- .../org/opensearch/sql/legacy/QueryIT.java | 16 +++--- .../sql/legacy/SQLIntegTestCase.java | 50 ++++++++++++++++--- 2 files changed, 53 insertions(+), 13 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java index 3fe8b50eefa..66d79c2e9bc 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java @@ -94,7 +94,6 @@ public void searchTypeTest() throws IOException { executeQuery( String.format( Locale.ROOT, "SELECT * FROM %s LIMIT 1000", TestsConstants.TEST_INDEX_PHRASE)); - Assert.assertTrue(response.has("hits")); Assert.assertEquals(6, getTotalHits(response)); } @@ -107,7 +106,6 @@ public void multipleFromTest() throws IOException { "SELECT * FROM %s, %s LIMIT 2000", TestsConstants.TEST_INDEX_BANK, TestsConstants.TEST_INDEX_BANK_TWO)); - Assert.assertTrue(response.has("hits")); Assert.assertEquals(14, getTotalHits(response)); } @@ -132,7 +130,7 @@ public void selectAllWithFieldReverseOrder() throws IOException { @Test public void selectAllWithMultipleFields() throws IOException { JSONObject response = - executeQuery( + executeQuery( // TODO: Multiple entries with same key: age=32 and age=32 StringUtils.format( "SELECT *, age, address FROM %s LIMIT 5", TestsConstants.TEST_INDEX_BANK)); @@ -152,7 +150,7 @@ public void selectAllWithFieldAndOrderBy() throws IOException { @Test public void selectAllWithFieldAndGroupBy() throws IOException { JSONObject response = - executeQuery( + executeQuery( // TODO: Multiple entries with same key: age=28 and age=28 StringUtils.format( "SELECT *, age FROM %s GROUP BY age LIMIT 10", TestsConstants.TEST_INDEX_BANK)); @@ -650,11 +648,11 @@ public void notBetweenTest() throws IOException { @Test public void inTest() throws IOException { JSONObject response = - executeQuery( + executeQuery( // TODO: can't resolve Symbol(namespace=FIELD_NAME, name=age) in type env String.format( Locale.ROOT, "SELECT age FROM %s WHERE age IN (20, 22) LIMIT 1000", - TestsConstants.TEST_INDEX_PHRASE)); + TestsConstants.TEST_INDEX_PHRASE)); // change to PEOPLE? JSONArray hits = getHits(response); for (int i = 0; i < hits.length(); i++) { @@ -1186,6 +1184,7 @@ public void testWhereWithBoolEqualsFalseAndOrderBy() throws IOException { checkResponseSize(response, BANK_INDEX_MALE_FALSE); } + // TODO: aggregation format is different. check in json format first @Test public void testWhereWithBoolIsFalse() throws IOException { JSONObject response = @@ -1370,6 +1369,7 @@ public void complexObjectReturnField() throws IOException { Assert.assertEquals(1, getTotalHits(response)); JSONObject hit = hits.getJSONObject(0); + // TODO: JSONObject["parents"] not found. Assert.assertEquals("Eddard", getSource(hit).getJSONObject("parents").getString("father")); } @@ -1826,7 +1826,7 @@ public void highlightPreTagsAndPostTags() throws IOException { JSONArray hits = getHits(response); for (int i = 0; i < hits.length(); i++) { JSONObject hit = hits.getJSONObject(i); - JSONObject highlightFields = hit.getJSONObject("highlight"); + JSONObject highlightFields = hit.getJSONObject("highlight"); // TODO: JSONObject["highlight"] not found. String phrase = highlightFields.getJSONArray("phrase").getString(0); Assert.assertTrue(phrase.contains("fox")); @@ -1963,6 +1963,8 @@ public void caseWhenSwitchTest() throws IOException { JSONObject hit = getHits(response).getJSONObject(0); String age = hit.query("/_source/age").toString(); String cases = age.equals("30") ? "1" : age.equals("40") ? "2" : "0"; + // TODO: Cannot invoke "Object.toString()" because the return value of "org.json.JSONObject.query(String)" is null + // TODO: search for hit.query assertThat(cases, equalTo(hit.query("/fields/cases/0"))); } diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java index a9179a7b820..7472c3ecf7e 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java @@ -493,17 +493,55 @@ protected String makeCursorRequest(String cursor) { return String.format("{\"cursor\":\"%s\"}", cursor); } + /** + * Following methods adapts the jdbc response format to maintain compatibility with existing code that + * expects legacy json (hits) structure. Instead of refactoring all dependent testing code to work with + * the jdbc (schema/datarows) format, this adapter transforms the jdbc format into the json structure. + * Jdbc format: + * { + * "schema": [{"name": "field1", "type": "text"}, ...], + * "datarows": [[value1, value2, ...], ...] + * } + * + * Transformed to legacy json format: + * { + * "hits": [{ + * "_source": {"field1": value1, ...} + * }, ...] + * } + */ protected JSONArray getHits(JSONObject response) { - Assert.assertTrue(response.getJSONObject("hits").has("hits")); + Assert.assertTrue(response.has("schema")); + Assert.assertTrue(response.has("datarows")); + + JSONArray schema = response.getJSONArray("schema"); + JSONArray datarows = response.getJSONArray("datarows"); + JSONArray hits = new JSONArray(); + + for (int i = 0; i < datarows.length(); i++) { + JSONObject hit = new JSONObject(); + JSONObject source = new JSONObject(); + JSONArray row = datarows.getJSONArray(i); + + for (int j = 0; j < schema.length(); j++) { + JSONObject schemaField = schema.getJSONObject(j); + String fieldName = schemaField.getString("name"); + Object value = row.get(j); + if (value != JSONObject.NULL) { // TODO: need this? + source.put(fieldName, value); + } + } + + hit.put("_source", source); + hits.put(hit); + } - return response.getJSONObject("hits").getJSONArray("hits"); + return hits; } protected int getTotalHits(JSONObject response) { - Assert.assertTrue(response.getJSONObject("hits").has("total")); - Assert.assertTrue(response.getJSONObject("hits").getJSONObject("total").has("value")); - - return response.getJSONObject("hits").getJSONObject("total").getInt("value"); + Assert.assertTrue(response.has("total")); + return response.getInt("total"); } protected JSONObject getSource(JSONObject hit) { From 85e2566d4c6ae114c7ec9b264ac0dae2d7738d9c Mon Sep 17 00:00:00 2001 From: Sean Kao Date: Fri, 28 Feb 2025 09:29:11 -0800 Subject: [PATCH 05/21] Revert "fix test case wip" This reverts commit e3157aa37ac977c14ca37aa9156d055475d6e32c. Signed-off-by: Sean Kao --- .../org/opensearch/sql/legacy/QueryIT.java | 16 +++--- .../sql/legacy/SQLIntegTestCase.java | 50 +++---------------- 2 files changed, 13 insertions(+), 53 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java index 66d79c2e9bc..3fe8b50eefa 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java @@ -94,6 +94,7 @@ public void searchTypeTest() throws IOException { executeQuery( String.format( Locale.ROOT, "SELECT * FROM %s LIMIT 1000", TestsConstants.TEST_INDEX_PHRASE)); + Assert.assertTrue(response.has("hits")); Assert.assertEquals(6, getTotalHits(response)); } @@ -106,6 +107,7 @@ public void multipleFromTest() throws IOException { "SELECT * FROM %s, %s LIMIT 2000", TestsConstants.TEST_INDEX_BANK, TestsConstants.TEST_INDEX_BANK_TWO)); + Assert.assertTrue(response.has("hits")); Assert.assertEquals(14, getTotalHits(response)); } @@ -130,7 +132,7 @@ public void selectAllWithFieldReverseOrder() throws IOException { @Test public void selectAllWithMultipleFields() throws IOException { JSONObject response = - executeQuery( // TODO: Multiple entries with same key: age=32 and age=32 + executeQuery( StringUtils.format( "SELECT *, age, address FROM %s LIMIT 5", TestsConstants.TEST_INDEX_BANK)); @@ -150,7 +152,7 @@ public void selectAllWithFieldAndOrderBy() throws IOException { @Test public void selectAllWithFieldAndGroupBy() throws IOException { JSONObject response = - executeQuery( // TODO: Multiple entries with same key: age=28 and age=28 + executeQuery( StringUtils.format( "SELECT *, age FROM %s GROUP BY age LIMIT 10", TestsConstants.TEST_INDEX_BANK)); @@ -648,11 +650,11 @@ public void notBetweenTest() throws IOException { @Test public void inTest() throws IOException { JSONObject response = - executeQuery( // TODO: can't resolve Symbol(namespace=FIELD_NAME, name=age) in type env + executeQuery( String.format( Locale.ROOT, "SELECT age FROM %s WHERE age IN (20, 22) LIMIT 1000", - TestsConstants.TEST_INDEX_PHRASE)); // change to PEOPLE? + TestsConstants.TEST_INDEX_PHRASE)); JSONArray hits = getHits(response); for (int i = 0; i < hits.length(); i++) { @@ -1184,7 +1186,6 @@ public void testWhereWithBoolEqualsFalseAndOrderBy() throws IOException { checkResponseSize(response, BANK_INDEX_MALE_FALSE); } - // TODO: aggregation format is different. check in json format first @Test public void testWhereWithBoolIsFalse() throws IOException { JSONObject response = @@ -1369,7 +1370,6 @@ public void complexObjectReturnField() throws IOException { Assert.assertEquals(1, getTotalHits(response)); JSONObject hit = hits.getJSONObject(0); - // TODO: JSONObject["parents"] not found. Assert.assertEquals("Eddard", getSource(hit).getJSONObject("parents").getString("father")); } @@ -1826,7 +1826,7 @@ public void highlightPreTagsAndPostTags() throws IOException { JSONArray hits = getHits(response); for (int i = 0; i < hits.length(); i++) { JSONObject hit = hits.getJSONObject(i); - JSONObject highlightFields = hit.getJSONObject("highlight"); // TODO: JSONObject["highlight"] not found. + JSONObject highlightFields = hit.getJSONObject("highlight"); String phrase = highlightFields.getJSONArray("phrase").getString(0); Assert.assertTrue(phrase.contains("fox")); @@ -1963,8 +1963,6 @@ public void caseWhenSwitchTest() throws IOException { JSONObject hit = getHits(response).getJSONObject(0); String age = hit.query("/_source/age").toString(); String cases = age.equals("30") ? "1" : age.equals("40") ? "2" : "0"; - // TODO: Cannot invoke "Object.toString()" because the return value of "org.json.JSONObject.query(String)" is null - // TODO: search for hit.query assertThat(cases, equalTo(hit.query("/fields/cases/0"))); } diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java index 7472c3ecf7e..a9179a7b820 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java @@ -493,55 +493,17 @@ protected String makeCursorRequest(String cursor) { return String.format("{\"cursor\":\"%s\"}", cursor); } - /** - * Following methods adapts the jdbc response format to maintain compatibility with existing code that - * expects legacy json (hits) structure. Instead of refactoring all dependent testing code to work with - * the jdbc (schema/datarows) format, this adapter transforms the jdbc format into the json structure. - * Jdbc format: - * { - * "schema": [{"name": "field1", "type": "text"}, ...], - * "datarows": [[value1, value2, ...], ...] - * } - * - * Transformed to legacy json format: - * { - * "hits": [{ - * "_source": {"field1": value1, ...} - * }, ...] - * } - */ protected JSONArray getHits(JSONObject response) { - Assert.assertTrue(response.has("schema")); - Assert.assertTrue(response.has("datarows")); - - JSONArray schema = response.getJSONArray("schema"); - JSONArray datarows = response.getJSONArray("datarows"); - JSONArray hits = new JSONArray(); - - for (int i = 0; i < datarows.length(); i++) { - JSONObject hit = new JSONObject(); - JSONObject source = new JSONObject(); - JSONArray row = datarows.getJSONArray(i); - - for (int j = 0; j < schema.length(); j++) { - JSONObject schemaField = schema.getJSONObject(j); - String fieldName = schemaField.getString("name"); - Object value = row.get(j); - if (value != JSONObject.NULL) { // TODO: need this? - source.put(fieldName, value); - } - } - - hit.put("_source", source); - hits.put(hit); - } + Assert.assertTrue(response.getJSONObject("hits").has("hits")); - return hits; + return response.getJSONObject("hits").getJSONArray("hits"); } protected int getTotalHits(JSONObject response) { - Assert.assertTrue(response.has("total")); - return response.getInt("total"); + Assert.assertTrue(response.getJSONObject("hits").has("total")); + Assert.assertTrue(response.getJSONObject("hits").getJSONObject("total").has("value")); + + return response.getJSONObject("hits").getJSONObject("total").getInt("value"); } protected JSONObject getSource(JSONObject hit) { From 5f0ac18fda02a46fe3bb81676e6c110b49164e22 Mon Sep 17 00:00:00 2001 From: Sean Kao Date: Fri, 28 Feb 2025 10:51:29 -0800 Subject: [PATCH 06/21] ignore legacy test using json response format Signed-off-by: Sean Kao --- .../opensearch/sql/legacy/AggregationIT.java | 1 + .../org/opensearch/sql/legacy/DateFormatIT.java | 1 + .../org/opensearch/sql/legacy/HashJoinIT.java | 2 ++ .../java/org/opensearch/sql/legacy/JoinIT.java | 1 + .../org/opensearch/sql/legacy/MultiQueryIT.java | 2 ++ .../java/org/opensearch/sql/legacy/OrderIT.java | 2 ++ .../org/opensearch/sql/legacy/PluginIT.java | 2 ++ .../java/org/opensearch/sql/legacy/QueryIT.java | 1 + .../opensearch/sql/legacy/SQLIntegTestCase.java | 17 +++++++++++++++-- 9 files changed, 27 insertions(+), 2 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/AggregationIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/AggregationIT.java index 490e9eb5102..c2b24c9bbfb 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/AggregationIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/AggregationIT.java @@ -38,6 +38,7 @@ import org.junit.Ignore; import org.junit.Test; +@Ignore public class AggregationIT extends SQLIntegTestCase { @Override diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/DateFormatIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/DateFormatIT.java index 388d9009249..af88c4e5786 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/DateFormatIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/DateFormatIT.java @@ -29,6 +29,7 @@ import org.junit.Test; import org.opensearch.sql.legacy.exception.SqlParseException; +@Ignore public class DateFormatIT extends SQLIntegTestCase { private static final String SELECT_FROM = diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/HashJoinIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/HashJoinIT.java index 02c55d8eb8f..f27c18b2452 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/HashJoinIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/HashJoinIT.java @@ -19,8 +19,10 @@ import org.json.JSONArray; import org.json.JSONObject; import org.junit.Assert; +import org.junit.Ignore; import org.junit.Test; +@Ignore /** Test new hash join algorithm by comparison with old implementation. */ public class HashJoinIT extends SQLIntegTestCase { diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/JoinIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/JoinIT.java index 8c2ea96474d..af5434e9919 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/JoinIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/JoinIT.java @@ -31,6 +31,7 @@ import org.junit.Ignore; import org.junit.Test; +@Ignore public class JoinIT extends SQLIntegTestCase { private static final String USE_NL_HINT = " /*! USE_NL*/"; diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/MultiQueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/MultiQueryIT.java index bee85ac3145..9fdb0f30d55 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/MultiQueryIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/MultiQueryIT.java @@ -14,8 +14,10 @@ import org.json.JSONArray; import org.json.JSONObject; import org.junit.Assert; +import org.junit.Ignore; import org.junit.Test; +@Ignore public class MultiQueryIT extends SQLIntegTestCase { private static final String MINUS_SCROLL_DEFAULT_HINT = diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/OrderIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/OrderIT.java index 20bed5d2edb..ce46de48ecb 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/OrderIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/OrderIT.java @@ -10,8 +10,10 @@ import java.io.IOException; import org.json.JSONArray; import org.json.JSONObject; +import org.junit.Ignore; import org.junit.Test; +@Ignore public class OrderIT extends SQLIntegTestCase { @Override diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java index 9305b1655cc..83fe0b94b4c 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java @@ -12,6 +12,7 @@ import java.io.IOException; import java.util.Locale; import org.json.JSONObject; +import org.junit.Ignore; import org.junit.Test; import org.opensearch.client.Request; import org.opensearch.client.RequestOptions; @@ -19,6 +20,7 @@ import org.opensearch.client.ResponseException; import org.opensearch.sql.plugin.rest.RestQuerySettingsAction; +@Ignore public class PluginIT extends SQLIntegTestCase { @Override diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java index 3fe8b50eefa..aa905c6c411 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java @@ -40,6 +40,7 @@ import org.opensearch.core.rest.RestStatus; import org.opensearch.sql.legacy.utils.StringUtils; +@Ignore public class QueryIT extends SQLIntegTestCase { /** diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java index a9179a7b820..c324b109045 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java @@ -228,8 +228,13 @@ protected synchronized void loadIndex(Index index) throws IOException { loadIndex(index, client()); } + /** + * TODO: Decide what to do with legacy tests using json response format. + * OpenSearch DSL format is deprecated. Need to ensure that requests in legacy tests using the json response format + * are not invoked. + */ protected Request getSqlRequest(String request, boolean explain) { - return getSqlRequest(request, explain, "jdbc"); + return getSqlRequest(request, explain, "json"); } protected Request getSqlRequest(String request, boolean explain, String requestType) { @@ -330,6 +335,11 @@ protected String executeFetchLessQuery(String query, String requestType) throws return responseString; } + /** + * TODO: Decide what to do with legacy tests using json response format. + * OpenSearch DSL format is deprecated. Need to ensure that requests in legacy tests using the json response format + * are not invoked. + */ protected Request buildGetEndpointRequest(final String sqlQuery) { final String utf8CharsetName = StandardCharsets.UTF_8.name(); @@ -344,7 +354,7 @@ protected Request buildGetEndpointRequest(final String sqlQuery) { final String requestUrl = String.format( - Locale.ROOT, "%s?sql=%s&format=%s", QUERY_API_ENDPOINT, urlEncodedQuery, "jdbc"); + Locale.ROOT, "%s?sql=%s&format=%s", QUERY_API_ENDPOINT, urlEncodedQuery, "json"); return new Request("GET", requestUrl); } @@ -385,6 +395,9 @@ private String executeRequest(final String requestBody, final boolean isExplainQ protected static String executeRequest(final Request request, RestClient client) throws IOException { + if (request.getParameters().get("format") == "json") { + throw new IOException("request type is json"); + } Response response = client.performRequest(request); Assert.assertEquals(200, response.getStatusLine().getStatusCode()); return getResponseBody(response); From 3cbaf825356abba4b4de34d8b96bd13d73f0c124 Mon Sep 17 00:00:00 2001 From: Sean Kao Date: Fri, 28 Feb 2025 11:00:18 -0800 Subject: [PATCH 07/21] spotlessApply Signed-off-by: Sean Kao --- .tool-versions | 1 + .../opensearch/sql/legacy/SQLIntegTestCase.java | 12 ++++++------ .../ActionRequestRestExecutorFactory.java | 3 --- .../planner/OpenSearchActionFactoryTest.java | 16 +++++----------- 4 files changed, 12 insertions(+), 20 deletions(-) create mode 100644 .tool-versions diff --git a/.tool-versions b/.tool-versions new file mode 100644 index 00000000000..e95b9595eaf --- /dev/null +++ b/.tool-versions @@ -0,0 +1 @@ +java adoptopenjdk-21.0.5+11.0.LTS diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java index c324b109045..852675c8c5f 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java @@ -229,9 +229,9 @@ protected synchronized void loadIndex(Index index) throws IOException { } /** - * TODO: Decide what to do with legacy tests using json response format. - * OpenSearch DSL format is deprecated. Need to ensure that requests in legacy tests using the json response format - * are not invoked. + * TODO: Decide what to do with legacy tests using json response format. OpenSearch DSL format is + * deprecated. Need to ensure that requests in legacy tests using the json response format are not + * invoked. */ protected Request getSqlRequest(String request, boolean explain) { return getSqlRequest(request, explain, "json"); @@ -336,9 +336,9 @@ protected String executeFetchLessQuery(String query, String requestType) throws } /** - * TODO: Decide what to do with legacy tests using json response format. - * OpenSearch DSL format is deprecated. Need to ensure that requests in legacy tests using the json response format - * are not invoked. + * TODO: Decide what to do with legacy tests using json response format. OpenSearch DSL format is + * deprecated. Need to ensure that requests in legacy tests using the json response format are not + * invoked. */ protected Request buildGetEndpointRequest(final String sqlQuery) { diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java index a585c3c2cb7..068c1b433c4 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java @@ -7,9 +7,6 @@ import org.opensearch.sql.legacy.executor.csv.CSVResultRestExecutor; import org.opensearch.sql.legacy.executor.format.PrettyFormatRestExecutor; -import org.opensearch.sql.legacy.query.QueryAction; -import org.opensearch.sql.legacy.query.join.OpenSearchJoinQueryAction; -import org.opensearch.sql.legacy.query.multi.MultiQueryAction; /** Created by Eliran on 26/12/2015. */ public class ActionRequestRestExecutorFactory { diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java index 44ad735ac7f..008b0d618df 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java @@ -9,7 +9,6 @@ import static org.junit.Assert.assertTrue; import org.junit.Test; -import org.opensearch.sql.legacy.executor.Format; import org.opensearch.sql.legacy.query.OpenSearchActionFactory; import org.opensearch.sql.legacy.util.SqlParserUtils; @@ -18,39 +17,34 @@ public class OpenSearchActionFactoryTest { public void nestQueryShouldNotMigrateToQueryPlan() { String sql = "SELECT age, nested(balance) FROM account GROUP BY age"; - assertFalse( - OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); + assertFalse(OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); } @Test public void nonAggregationQueryShouldNotMigrateToQueryPlan() { String sql = "SELECT age FROM account "; - assertFalse( - OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); + assertFalse(OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); } @Test public void aggregationQueryWithoutGroupByShouldMigrateToQueryPlan() { String sql = "SELECT age, COUNT(balance) FROM account "; - assertTrue( - OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); + assertTrue(OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); } @Test public void aggregationQueryWithExpressionByShouldMigrateToQueryPlan() { String sql = "SELECT age, MAX(balance) - MIN(balance) FROM account "; - assertTrue( - OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); + assertTrue(OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); } @Test public void queryOnlyHasGroupByShouldMigrateToQueryPlan() { String sql = "SELECT CAST(age AS DOUBLE) as alias FROM account GROUP BY alias"; - assertTrue( - OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); + assertTrue(OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); } } From c74b96b8a7e9f2e35a012eaf80f93c0a249b430d Mon Sep 17 00:00:00 2001 From: Sean Kao Date: Fri, 28 Feb 2025 11:58:15 -0800 Subject: [PATCH 08/21] remove json param check in executeRequest Signed-off-by: Sean Kao --- .../test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java index 852675c8c5f..dc96e30cb80 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java @@ -395,9 +395,6 @@ private String executeRequest(final String requestBody, final boolean isExplainQ protected static String executeRequest(final Request request, RestClient client) throws IOException { - if (request.getParameters().get("format") == "json") { - throw new IOException("request type is json"); - } Response response = client.performRequest(request); Assert.assertEquals(200, response.getStatusLine().getStatusCode()); return getResponseBody(response); From 48d512aa3632998409359ee4465a6c621bf3bbd8 Mon Sep 17 00:00:00 2001 From: Sean Kao Date: Fri, 28 Feb 2025 15:43:19 -0800 Subject: [PATCH 09/21] fix getScriptFieldFromQuery can't get requestBuilder from QueryPlan Signed-off-by: Sean Kao --- .../org/opensearch/sql/legacy/plugin/SearchDao.java | 2 +- .../sql/legacy/query/OpenSearchActionFactory.java | 12 +++++++++--- .../sql/legacy/unittest/JSONRequestTest.java | 2 +- .../sql/legacy/util/CheckScriptContents.java | 3 ++- 4 files changed, 13 insertions(+), 6 deletions(-) diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/SearchDao.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/SearchDao.java index 415b98f6b89..10ca9ddae2e 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/SearchDao.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/SearchDao.java @@ -45,6 +45,6 @@ public Client getClient() { */ public QueryAction explain(QueryActionRequest queryActionRequest) throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { - return OpenSearchActionFactory.create(client, queryActionRequest); + return OpenSearchActionFactory.create(client, queryActionRequest, false); } } diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java index 93e721e52d4..2f6ea1379c4 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java @@ -63,16 +63,22 @@ public class OpenSearchActionFactory { public static QueryAction create(Client client, String sql) throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { - return create(client, new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JDBC)); + return create(client, sql, false); + } + + public static QueryAction create(Client client, String sql, boolean bypassMigrateToQueryPlan) + throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { + return create(client, new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JDBC), bypassMigrateToQueryPlan); } /** * Create the compatible Query object based on the SQL query. * * @param request The SQL query. + * @param bypassMigrateToQueryPlan Avoid using QueryPlan. * @return Query object. */ - public static QueryAction create(Client client, QueryActionRequest request) + public static QueryAction create(Client client, QueryActionRequest request, boolean bypassMigrateToQueryPlan) throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { String sql = request.getSql(); // Remove line breaker anywhere and semicolon at the end @@ -116,7 +122,7 @@ public static QueryAction create(Client client, QueryActionRequest request) } else { sqlExpr.accept(new TermFieldRewriter()); // migrate aggregation to query planner framework. - if (shouldMigrateToQueryPlan(sqlExpr)) { + if (!bypassMigrateToQueryPlan && shouldMigrateToQueryPlan(sqlExpr)) { return new QueryPlanQueryAction( new QueryPlanRequestBuilder( new BindingTupleQueryPlanner(client, sqlExpr, request.getTypeProvider()))); diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/JSONRequestTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/JSONRequestTest.java index 5cf9c602a26..c0ea8581e13 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/JSONRequestTest.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/JSONRequestTest.java @@ -435,7 +435,7 @@ private String translate(String sql, JSONObject jsonRequest) CheckScriptContents.stubMockClient(mockClient); QueryAction queryAction = OpenSearchActionFactory.create( - mockClient, new QueryActionRequest(sql, columnTypeProvider, Format.JDBC)); + mockClient, new QueryActionRequest(sql, columnTypeProvider, Format.JDBC), false); SqlRequest sqlRequest = new SqlRequest(sql, jsonRequest); queryAction.setSqlRequest(sqlRequest); diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/util/CheckScriptContents.java b/legacy/src/test/java/org/opensearch/sql/legacy/util/CheckScriptContents.java index 2fe711c3b06..c15bbc39f1b 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/util/CheckScriptContents.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/util/CheckScriptContents.java @@ -63,7 +63,8 @@ public static ScriptField getScriptFieldFromQuery(String query) { try { Client mockClient = mock(Client.class); stubMockClient(mockClient); - QueryAction queryAction = OpenSearchActionFactory.create(mockClient, query); + // Avoid creating QueryPlanQueryAction so that scriptFields is available + QueryAction queryAction = OpenSearchActionFactory.create(mockClient, query, true); SqlElasticRequestBuilder requestBuilder = queryAction.explain(); SearchRequestBuilder request = (SearchRequestBuilder) requestBuilder.getBuilder(); From d0c1ef1b070c3c14817d2fef07bf9b2a2cef7855 Mon Sep 17 00:00:00 2001 From: Sean Kao Date: Fri, 28 Feb 2025 15:52:57 -0800 Subject: [PATCH 10/21] spotlessApply Signed-off-by: Sean Kao --- .../sql/legacy/query/OpenSearchActionFactory.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java index 2f6ea1379c4..ffa5b24fd31 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java @@ -68,7 +68,10 @@ public static QueryAction create(Client client, String sql) public static QueryAction create(Client client, String sql, boolean bypassMigrateToQueryPlan) throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { - return create(client, new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JDBC), bypassMigrateToQueryPlan); + return create( + client, + new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JDBC), + bypassMigrateToQueryPlan); } /** @@ -78,7 +81,8 @@ public static QueryAction create(Client client, String sql, boolean bypassMigrat * @param bypassMigrateToQueryPlan Avoid using QueryPlan. * @return Query object. */ - public static QueryAction create(Client client, QueryActionRequest request, boolean bypassMigrateToQueryPlan) + public static QueryAction create( + Client client, QueryActionRequest request, boolean bypassMigrateToQueryPlan) throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { String sql = request.getSql(); // Remove line breaker anywhere and semicolon at the end From 82cfce09b145f8f3abad284bb61a5f7be058073c Mon Sep 17 00:00:00 2001 From: Sean Kao Date: Fri, 28 Feb 2025 16:45:42 -0800 Subject: [PATCH 11/21] ignore test using json Signed-off-by: Sean Kao --- .../test/java/org/opensearch/sql/legacy/DateFunctionsIT.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/DateFunctionsIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/DateFunctionsIT.java index d9a6849fc80..81206cf765b 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/DateFunctionsIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/DateFunctionsIT.java @@ -17,6 +17,7 @@ import org.joda.time.format.DateTimeFormat; import org.joda.time.format.DateTimeFormatter; import org.json.JSONObject; +import org.junit.Ignore; import org.junit.Test; import org.opensearch.action.search.SearchResponse; import org.opensearch.common.xcontent.LoggingDeprecationHandler; @@ -25,6 +26,7 @@ import org.opensearch.core.xcontent.XContentParser; import org.opensearch.search.SearchHit; +@Ignore public class DateFunctionsIT extends SQLIntegTestCase { private static final String FROM = "FROM " + TestsConstants.TEST_INDEX_ONLINE; From 5f38dd52f8c870a408a9ea9f6323df4e1bc90c22 Mon Sep 17 00:00:00 2001 From: Sean Kao Date: Fri, 28 Feb 2025 16:53:47 -0800 Subject: [PATCH 12/21] fix test case Signed-off-by: Sean Kao --- .../sql/legacy/unittest/query/DefaultQueryActionTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java index bf5d79855c5..4c6061ff5e7 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java @@ -136,7 +136,7 @@ public void testIfScrollShouldBeOpenWithDifferentFormats() { queryAction.checkAndSetScroll(); } - Mockito.verify(mockRequestBuilder, times(4)).setSize(limit); + Mockito.verify(mockRequestBuilder, times(3)).setSize(limit); Mockito.verify(mockRequestBuilder, never()).setScroll(any(TimeValue.class)); queryAction.setFormat(Format.JDBC); From 1cdeccd72bbac4ad606cc5ae6f2b23b5d02d325e Mon Sep 17 00:00:00 2001 From: Sean Kao Date: Fri, 28 Feb 2025 18:08:58 -0800 Subject: [PATCH 13/21] ignore legacy tests affected Signed-off-by: Sean Kao --- .../opensearch/sql/legacy/AggregationIT.java | 4 +- .../opensearch/sql/legacy/DateFormatIT.java | 4 +- .../sql/legacy/DateFunctionsIT.java | 4 +- .../org/opensearch/sql/legacy/HashJoinIT.java | 4 +- .../org/opensearch/sql/legacy/HavingIT.java | 4 + .../opensearch/sql/legacy/JSONRequestIT.java | 4 + .../org/opensearch/sql/legacy/JoinIT.java | 4 +- .../sql/legacy/MalformedQueryIT.java | 4 + .../sql/legacy/MathFunctionsIT.java | 4 + .../opensearch/sql/legacy/MultiQueryIT.java | 4 +- .../sql/legacy/NestedFieldQueryIT.java | 3 + .../org/opensearch/sql/legacy/OrderIT.java | 4 +- .../org/opensearch/sql/legacy/PluginIT.java | 4 +- .../sql/legacy/QueryFunctionsIT.java | 4 + .../org/opensearch/sql/legacy/QueryIT.java | 4 +- .../opensearch/sql/legacy/SQLFunctionsIT.java | 3 + .../org/opensearch/sql/legacy/ShowIT.java | 4 + .../opensearch/sql/legacy/SourceFieldIT.java | 4 + .../org/opensearch/sql/legacy/SubqueryIT.java | 3 + .../org/opensearch/sql/sql/ConditionalIT.java | 7 + my.patch | 1495 +++++++++++++++++ 21 files changed, 1566 insertions(+), 9 deletions(-) create mode 100644 my.patch diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/AggregationIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/AggregationIT.java index c2b24c9bbfb..a5f47ad1d17 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/AggregationIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/AggregationIT.java @@ -38,7 +38,9 @@ import org.junit.Ignore; import org.junit.Test; -@Ignore +@Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") public class AggregationIT extends SQLIntegTestCase { @Override diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/DateFormatIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/DateFormatIT.java index af88c4e5786..863adccbd18 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/DateFormatIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/DateFormatIT.java @@ -29,7 +29,9 @@ import org.junit.Test; import org.opensearch.sql.legacy.exception.SqlParseException; -@Ignore +@Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") public class DateFormatIT extends SQLIntegTestCase { private static final String SELECT_FROM = diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/DateFunctionsIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/DateFunctionsIT.java index 81206cf765b..56c60eb91e6 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/DateFunctionsIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/DateFunctionsIT.java @@ -26,7 +26,9 @@ import org.opensearch.core.xcontent.XContentParser; import org.opensearch.search.SearchHit; -@Ignore +@Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") public class DateFunctionsIT extends SQLIntegTestCase { private static final String FROM = "FROM " + TestsConstants.TEST_INDEX_ONLINE; diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/HashJoinIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/HashJoinIT.java index f27c18b2452..c83cbfc063d 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/HashJoinIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/HashJoinIT.java @@ -22,8 +22,10 @@ import org.junit.Ignore; import org.junit.Test; -@Ignore /** Test new hash join algorithm by comparison with old implementation. */ +@Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") public class HashJoinIT extends SQLIntegTestCase { /** Hint to use old join algorithm */ diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/HavingIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/HavingIT.java index 3bd2195a89e..b7c998dfc9f 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/HavingIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/HavingIT.java @@ -20,8 +20,12 @@ import org.hamcrest.Matcher; import org.json.JSONArray; import org.json.JSONObject; +import org.junit.Ignore; import org.junit.Test; +@Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") public class HavingIT extends SQLIntegTestCase { private static final String SELECT_FROM_WHERE_GROUP_BY = diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/JSONRequestIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/JSONRequestIT.java index b6c0942ba4d..aa182144852 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/JSONRequestIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/JSONRequestIT.java @@ -14,6 +14,7 @@ import java.io.IOException; import java.util.Map; import org.json.JSONObject; +import org.junit.Ignore; import org.junit.Test; import org.opensearch.action.search.SearchResponse; import org.opensearch.common.xcontent.LoggingDeprecationHandler; @@ -23,6 +24,9 @@ import org.opensearch.search.SearchHit; import org.opensearch.search.SearchHits; +@Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") public class JSONRequestIT extends SQLIntegTestCase { @Override diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/JoinIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/JoinIT.java index af5434e9919..e06a1a2126d 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/JoinIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/JoinIT.java @@ -31,7 +31,9 @@ import org.junit.Ignore; import org.junit.Test; -@Ignore +@Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") public class JoinIT extends SQLIntegTestCase { private static final String USE_NL_HINT = " /*! USE_NL*/"; diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/MalformedQueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/MalformedQueryIT.java index 84b60fdabd7..4c2f3c56d7b 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/MalformedQueryIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/MalformedQueryIT.java @@ -11,9 +11,13 @@ import org.apache.hc.core5.http.io.entity.EntityUtils; import org.json.JSONObject; import org.junit.Assert; +import org.junit.Ignore; import org.opensearch.client.ResponseException; /** Tests for clean handling of various types of invalid queries */ +@Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") public class MalformedQueryIT extends SQLIntegTestCase { @Override protected void init() throws Exception { diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/MathFunctionsIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/MathFunctionsIT.java index fcf1edf3e09..5f2dee6c0b8 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/MathFunctionsIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/MathFunctionsIT.java @@ -12,6 +12,7 @@ import com.fasterxml.jackson.core.JsonFactory; import java.io.IOException; +import org.junit.Ignore; import org.junit.Test; import org.opensearch.action.search.SearchResponse; import org.opensearch.common.xcontent.LoggingDeprecationHandler; @@ -20,6 +21,9 @@ import org.opensearch.core.xcontent.XContentParser; import org.opensearch.search.SearchHit; +@Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") public class MathFunctionsIT extends SQLIntegTestCase { private static final String FROM = "FROM " + TestsConstants.TEST_INDEX_ACCOUNT; diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/MultiQueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/MultiQueryIT.java index 9fdb0f30d55..11432490adf 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/MultiQueryIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/MultiQueryIT.java @@ -17,7 +17,9 @@ import org.junit.Ignore; import org.junit.Test; -@Ignore +@Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") public class MultiQueryIT extends SQLIntegTestCase { private static final String MINUS_SCROLL_DEFAULT_HINT = diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/NestedFieldQueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/NestedFieldQueryIT.java index 2108bf6867f..82d94ca1163 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/NestedFieldQueryIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/NestedFieldQueryIT.java @@ -60,6 +60,9 @@ * 6) Verification for conditions mixed with regular and nested fields * */ +@Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") public class NestedFieldQueryIT extends SQLIntegTestCase { private static final String FROM = diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/OrderIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/OrderIT.java index ce46de48ecb..f91ae132c65 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/OrderIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/OrderIT.java @@ -13,7 +13,9 @@ import org.junit.Ignore; import org.junit.Test; -@Ignore +@Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") public class OrderIT extends SQLIntegTestCase { @Override diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java index 83fe0b94b4c..909ffef2c30 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java @@ -20,7 +20,9 @@ import org.opensearch.client.ResponseException; import org.opensearch.sql.plugin.rest.RestQuerySettingsAction; -@Ignore +@Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") public class PluginIT extends SQLIntegTestCase { @Override diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryFunctionsIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryFunctionsIT.java index 3cf45f7419e..4090f03ed8f 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryFunctionsIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryFunctionsIT.java @@ -26,6 +26,7 @@ import org.hamcrest.FeatureMatcher; import org.hamcrest.Matcher; import org.json.JSONObject; +import org.junit.Ignore; import org.junit.Test; import org.opensearch.action.search.SearchResponse; import org.opensearch.common.xcontent.LoggingDeprecationHandler; @@ -35,6 +36,9 @@ import org.opensearch.search.SearchHit; import org.opensearch.sql.legacy.utils.StringUtils; +@Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") public class QueryFunctionsIT extends SQLIntegTestCase { private static final String SELECT_ALL = "SELECT *"; diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java index aa905c6c411..f27e2030927 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java @@ -40,7 +40,9 @@ import org.opensearch.core.rest.RestStatus; import org.opensearch.sql.legacy.utils.StringUtils; -@Ignore +@Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") public class QueryIT extends SQLIntegTestCase { /** diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLFunctionsIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLFunctionsIT.java index 356b910d5fc..f3a6d03bc77 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLFunctionsIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLFunctionsIT.java @@ -44,6 +44,9 @@ import org.opensearch.search.SearchHit; import org.opensearch.search.SearchHits; +@Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") /** Created by allwefantasy on 8/25/16. */ public class SQLFunctionsIT extends SQLIntegTestCase { diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/ShowIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/ShowIT.java index fa86bbbc221..018140c0395 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/ShowIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/ShowIT.java @@ -13,8 +13,12 @@ import java.io.IOException; import org.json.JSONObject; import org.junit.Assert; +import org.junit.Ignore; import org.junit.Test; +@Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") public class ShowIT extends SQLIntegTestCase { @Override diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SourceFieldIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SourceFieldIT.java index bf288262b6f..a7d8acb3a7d 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/SourceFieldIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SourceFieldIT.java @@ -12,6 +12,7 @@ import java.util.Set; import org.json.JSONObject; import org.junit.Assert; +import org.junit.Ignore; import org.junit.Test; import org.opensearch.action.search.SearchResponse; import org.opensearch.common.xcontent.LoggingDeprecationHandler; @@ -21,6 +22,9 @@ import org.opensearch.search.SearchHit; import org.opensearch.search.SearchHits; +@Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") public class SourceFieldIT extends SQLIntegTestCase { @Override diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SubqueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SubqueryIT.java index 39abad92df9..16814ca33fb 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/SubqueryIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SubqueryIT.java @@ -35,6 +35,9 @@ import org.opensearch.client.ResponseException; import org.opensearch.sql.legacy.utils.StringUtils; +@Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") public class SubqueryIT extends SQLIntegTestCase { @Rule public final ExpectedException exceptionRule = ExpectedException.none(); diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/ConditionalIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/ConditionalIT.java index 9cf4fa2e8a5..72faeec2afe 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/ConditionalIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/ConditionalIT.java @@ -24,6 +24,7 @@ import java.util.List; import org.json.JSONArray; import org.json.JSONObject; +import org.junit.Ignore; import org.junit.Test; import org.opensearch.action.search.SearchResponse; import org.opensearch.common.xcontent.LoggingDeprecationHandler; @@ -157,6 +158,9 @@ public void isnullShouldPassJDBC() throws IOException { assertEquals("boolean", response.query("/schema/0/type")); } + @Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") @Test public void isnullWithNotNullInputTest() throws IOException { assertThat( @@ -188,6 +192,9 @@ public void isnullWithNullInputTest() { rows(LITERAL_TRUE.value(), LITERAL_FALSE.value())); } + @Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") @Test public void isnullWithMathExpr() throws IOException { assertThat( diff --git a/my.patch b/my.patch new file mode 100644 index 00000000000..c0e4bb9bbc9 --- /dev/null +++ b/my.patch @@ -0,0 +1,1495 @@ +From bd434ec5105b90491c1eb0f7129b606655465919 Mon Sep 17 00:00:00 2001 +From: Sean Kao +Date: Wed, 26 Feb 2025 09:14:40 -0800 +Subject: [PATCH 01/12] mark places to change + +Signed-off-by: Sean Kao +--- + docs/user/interfaces/protocol.rst | 1 + + .../test/java/org/opensearch/sql/legacy/ExplainIT.java | 1 + + .../org/opensearch/sql/legacy/GetEndpointQueryIT.java | 1 + + .../opensearch/sql/legacy/OpenSearchSQLRestTestCase.java | 1 + + .../java/org/opensearch/sql/legacy/SQLIntegTestCase.java | 8 ++++++++ + .../legacy/executor/ActionRequestRestExecutorFactory.java | 2 +- + .../java/org/opensearch/sql/legacy/executor/Format.java | 2 +- + .../sql/legacy/query/OpenSearchActionFactory.java | 2 ++ + .../sql/legacy/unittest/SqlRequestParamTest.java | 1 + + .../unittest/planner/OpenSearchActionFactoryTest.java | 1 + + .../sql/legacy/unittest/query/DefaultQueryActionTest.java | 1 + + 11 files changed, 19 insertions(+), 2 deletions(-) + +diff --git a/docs/user/interfaces/protocol.rst b/docs/user/interfaces/protocol.rst +index 52674b992..24e803ce9 100644 +--- a/docs/user/interfaces/protocol.rst ++++ b/docs/user/interfaces/protocol.rst +@@ -208,6 +208,7 @@ Result set:: + "status" : 400 + } + ++// TODO: deprecate json + OpenSearch DSL + ============== + +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java +index 1b740924c..2059c8462 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java +@@ -245,6 +245,7 @@ public class ExplainIT extends SQLIntegTestCase { + result.replaceAll("\\s+", ""), equalTo(expectedOutput.replaceAll("\\s+", ""))); + } + ++ // TODO: is this test running without @Test? + public void testContentTypeOfExplainRequestShouldBeJson() throws IOException { + String query = makeRequest("SELECT firstname FROM opensearch-sql_test_index_account"); + Request request = getSqlRequest(query, true); +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java +index 6cc4aba81..a74ddde65 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java +@@ -13,6 +13,7 @@ import org.junit.Test; + import org.junit.rules.ExpectedException; + import org.opensearch.client.ResponseException; + ++// TODO: inaccurate comment? Find out purpose of this test + /** Tests to cover requests with "?format=csv" parameter */ + public class GetEndpointQueryIT extends SQLIntegTestCase { + +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java +index ced69d54a..6b57310a9 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java +@@ -181,6 +181,7 @@ public abstract class OpenSearchSQLRestTestCase extends OpenSearchRestTestCase { + } + } + ++ // TODO: deprecate json + protected static void wipeAllOpenSearchIndices(RestClient client) throws IOException { + // include all the indices, included hidden indices. + // https://www.elastic.co/guide/en/elasticsearch/reference/current/cat-indices.html#cat-indices-api-query-params +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +index f59fe82fa..213401ec5 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +@@ -228,6 +228,8 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { + loadIndex(index, client()); + } + ++ // TODO: deprecate json ++ // Note that explain queries are still supposed to return json + protected Request getSqlRequest(String request, boolean explain) { + return getSqlRequest(request, explain, "json"); + } +@@ -266,10 +268,12 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { + } + } + ++ // DONE: verified all usages have requestType="jdbc" except in CsvFormatIT and RawFormatIT + protected String executeQuery(String query, String requestType) { + return executeQuery(query, requestType, Map.of()); + } + ++ // DONE: verified usages don't contain requestType="json" + protected String executeQuery(String query, String requestType, Map params) { + try { + String endpoint = "/_plugins/_sql?format=" + requestType; +@@ -293,6 +297,7 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { + return new JSONObject(executeQuery(query, "jdbc")); + } + ++ // DONE: verified usages don't contain requestType="json" + protected String executeFetchQuery(String query, int fetchSize, String requestType) + throws IOException { + String endpoint = "/_plugins/_sql?format=" + requestType; +@@ -317,6 +322,7 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { + return executeQueryTemplate(queryTemplate, index, 4); + } + ++ // DONE: verified usages don't contain requestType="json" + protected String executeFetchLessQuery(String query, String requestType) throws IOException { + + String endpoint = "/_plugins/_sql?format=" + requestType; +@@ -342,6 +348,7 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { + Assert.fail(utf8CharsetName + " not available"); + } + ++ // TODO: deprecate json + final String requestUrl = + String.format( + Locale.ROOT, "%s?sql=%s&format=%s", QUERY_API_ENDPOINT, urlEncodedQuery, "json"); +@@ -394,6 +401,7 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { + return executeRequest(request, client()); + } + ++ // TODO: this returns json. Only used in GetEndpointQueryIT + protected JSONObject executeQueryWithGetRequest(final String sqlQuery) throws IOException { + + final Request request = buildGetEndpointRequest(sqlQuery); +diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java +index c58bba9e2..8c6009248 100644 +--- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java ++++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java +@@ -25,7 +25,7 @@ public class ActionRequestRestExecutorFactory { + switch (format) { + case CSV: + return new AsyncRestExecutor(new CSVResultRestExecutor()); +- case JSON: ++ case JSON: // TODO: deprecate json + return new AsyncRestExecutor( + new ElasticDefaultRestExecutor(queryAction), + action -> isJoin(action) || isUnionMinus(action)); +diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java +index c47092f10..f17e2a1de 100644 +--- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java ++++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java +@@ -14,7 +14,7 @@ import lombok.RequiredArgsConstructor; + @RequiredArgsConstructor + public enum Format { + JDBC("jdbc"), +- JSON("json"), ++ JSON("json"), // TODO: deprecate json + CSV("csv"), + RAW("raw"), + TABLE("table"); +diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java +index 49aa38ae5..3924b6e41 100644 +--- a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java ++++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java +@@ -63,6 +63,7 @@ public class OpenSearchActionFactory { + + public static QueryAction create(Client client, String sql) + throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { ++ // TODO: deprecate json + return create(client, new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JSON)); + } + +@@ -192,6 +193,7 @@ public class OpenSearchActionFactory { + + @VisibleForTesting + public static boolean shouldMigrateToQueryPlan(SQLQueryExpr expr, Format format) { ++ // TODO: deprecate json + // The JSON format will return the OpenSearch aggregation result, which is not supported by the + // QueryPlanner. + if (format == Format.JSON) { +diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java +index 0d29b5510..b35ee4c99 100644 +--- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java ++++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java +@@ -41,6 +41,7 @@ public class SqlRequestParamTest { + assertFalse(SqlRequestParam.isPrettyFormat(ImmutableMap.of(QUERY_PARAMS_PRETTY, "unknown"))); + } + ++ // TODO: deprecate json + @Test + public void shouldReturnJSONIfFormatParamsIsJSON() { + assertEquals( +diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java +index 3443c2dec..9b550f379 100644 +--- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java ++++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java +@@ -14,6 +14,7 @@ import org.opensearch.sql.legacy.query.OpenSearchActionFactory; + import org.opensearch.sql.legacy.util.SqlParserUtils; + + public class OpenSearchActionFactoryTest { ++ // TODO: deprecate json + @Test + public void josnOutputRequestShouldNotMigrateToQueryPlan() { + String sql = "SELECT age, MAX(balance) FROM account GROUP BY age"; +diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java +index d290e4dd5..5471e37dd 100644 +--- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java ++++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java +@@ -130,6 +130,7 @@ public class DefaultQueryActionTest { + doReturn(settingFetchSize).when(mockSqlRequest).fetchSize(); + queryAction.setSqlRequest(mockSqlRequest); + ++ // TODO: deprecate json + Format[] formats = new Format[] {Format.CSV, Format.RAW, Format.JSON, Format.TABLE}; + for (Format format : formats) { + queryAction.setFormat(format); +-- +2.41.0 + + +From 613da7c026c6b038af1951ab5489b6f87fe0bc08 Mon Sep 17 00:00:00 2001 +From: Sean Kao +Date: Thu, 27 Feb 2025 09:25:35 -0800 +Subject: [PATCH 02/12] remove dsl format from doc + +Signed-off-by: Sean Kao +--- + docs/user/interfaces/protocol.rst | 68 ------------------------------- + 1 file changed, 68 deletions(-) + +diff --git a/docs/user/interfaces/protocol.rst b/docs/user/interfaces/protocol.rst +index 24e803ce9..625eed0f5 100644 +--- a/docs/user/interfaces/protocol.rst ++++ b/docs/user/interfaces/protocol.rst +@@ -208,74 +208,6 @@ Result set:: + "status" : 400 + } + +-// TODO: deprecate json +-OpenSearch DSL +-============== +- +-Description +------------ +- +-The plugin returns original response from OpenSearch in JSON. Because this is the native response from OpenSearch, extra efforts are needed to parse and interpret it. +- +-Example +-------- +- +-SQL query:: +- +- >> curl -H 'Content-Type: application/json' -X POST localhost:9200/_plugins/_sql?format=json -d '{ +- "query" : "SELECT firstname, lastname, age FROM accounts ORDER BY age LIMIT 2" +- }' +- +-Result set:: +- +- { +- "_shards" : { +- "total" : 5, +- "failed" : 0, +- "successful" : 5, +- "skipped" : 0 +- }, +- "hits" : { +- "hits" : [ +- { +- "_index" : "accounts", +- "_type" : "_doc", +- "_source" : { +- "firstname" : "Nanette", +- "age" : 28, +- "lastname" : "Bates" +- }, +- "_id" : "13", +- "sort" : [ +- 28 +- ], +- "_score" : null +- }, +- { +- "_index" : "accounts", +- "_type" : "_doc", +- "_source" : { +- "firstname" : "Amber", +- "age" : 32, +- "lastname" : "Duke" +- }, +- "_id" : "1", +- "sort" : [ +- 32 +- ], +- "_score" : null +- } +- ], +- "total" : { +- "value" : 4, +- "relation" : "eq" +- }, +- "max_score" : null +- }, +- "took" : 100, +- "timed_out" : false +- } +- + CSV Format + ========== + +-- +2.41.0 + + +From 43fe0fa9cc54538fb38e90382eb9c91f9a88219c Mon Sep 17 00:00:00 2001 +From: Sean Kao +Date: Thu, 27 Feb 2025 11:56:21 -0800 +Subject: [PATCH 03/12] remove Format.JSON + +Signed-off-by: Sean Kao +--- + .../org/opensearch/sql/legacy/ExplainIT.java | 1 - + .../sql/legacy/GetEndpointQueryIT.java | 2 -- + .../sql/legacy/OpenSearchSQLRestTestCase.java | 1 - + .../sql/legacy/SQLIntegTestCase.java | 12 ++---------- + .../ActionRequestRestExecutorFactory.java | 15 +-------------- + .../sql/legacy/executor/Format.java | 1 - + .../sql/legacy/plugin/RestSqlAction.java | 3 +-- + .../legacy/query/OpenSearchActionFactory.java | 13 +++---------- + .../legacy/unittest/SqlRequestParamTest.java | 7 ------- + .../planner/OpenSearchActionFactoryTest.java | 19 +++++-------------- + .../query/DefaultQueryActionTest.java | 3 +-- + 11 files changed, 13 insertions(+), 64 deletions(-) + +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java +index 2059c8462..1b740924c 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java +@@ -245,7 +245,6 @@ public class ExplainIT extends SQLIntegTestCase { + result.replaceAll("\\s+", ""), equalTo(expectedOutput.replaceAll("\\s+", ""))); + } + +- // TODO: is this test running without @Test? + public void testContentTypeOfExplainRequestShouldBeJson() throws IOException { + String query = makeRequest("SELECT firstname FROM opensearch-sql_test_index_account"); + Request request = getSqlRequest(query, true); +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java +index a74ddde65..6831dcfcc 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java +@@ -13,8 +13,6 @@ import org.junit.Test; + import org.junit.rules.ExpectedException; + import org.opensearch.client.ResponseException; + +-// TODO: inaccurate comment? Find out purpose of this test +-/** Tests to cover requests with "?format=csv" parameter */ + public class GetEndpointQueryIT extends SQLIntegTestCase { + + @Rule public final ExpectedException rule = ExpectedException.none(); +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java +index 6b57310a9..ced69d54a 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java +@@ -181,7 +181,6 @@ public abstract class OpenSearchSQLRestTestCase extends OpenSearchRestTestCase { + } + } + +- // TODO: deprecate json + protected static void wipeAllOpenSearchIndices(RestClient client) throws IOException { + // include all the indices, included hidden indices. + // https://www.elastic.co/guide/en/elasticsearch/reference/current/cat-indices.html#cat-indices-api-query-params +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +index 213401ec5..a9179a7b8 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +@@ -228,10 +228,8 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { + loadIndex(index, client()); + } + +- // TODO: deprecate json +- // Note that explain queries are still supposed to return json + protected Request getSqlRequest(String request, boolean explain) { +- return getSqlRequest(request, explain, "json"); ++ return getSqlRequest(request, explain, "jdbc"); + } + + protected Request getSqlRequest(String request, boolean explain, String requestType) { +@@ -268,12 +266,10 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { + } + } + +- // DONE: verified all usages have requestType="jdbc" except in CsvFormatIT and RawFormatIT + protected String executeQuery(String query, String requestType) { + return executeQuery(query, requestType, Map.of()); + } + +- // DONE: verified usages don't contain requestType="json" + protected String executeQuery(String query, String requestType, Map params) { + try { + String endpoint = "/_plugins/_sql?format=" + requestType; +@@ -297,7 +293,6 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { + return new JSONObject(executeQuery(query, "jdbc")); + } + +- // DONE: verified usages don't contain requestType="json" + protected String executeFetchQuery(String query, int fetchSize, String requestType) + throws IOException { + String endpoint = "/_plugins/_sql?format=" + requestType; +@@ -322,7 +317,6 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { + return executeQueryTemplate(queryTemplate, index, 4); + } + +- // DONE: verified usages don't contain requestType="json" + protected String executeFetchLessQuery(String query, String requestType) throws IOException { + + String endpoint = "/_plugins/_sql?format=" + requestType; +@@ -348,10 +342,9 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { + Assert.fail(utf8CharsetName + " not available"); + } + +- // TODO: deprecate json + final String requestUrl = + String.format( +- Locale.ROOT, "%s?sql=%s&format=%s", QUERY_API_ENDPOINT, urlEncodedQuery, "json"); ++ Locale.ROOT, "%s?sql=%s&format=%s", QUERY_API_ENDPOINT, urlEncodedQuery, "jdbc"); + return new Request("GET", requestUrl); + } + +@@ -401,7 +394,6 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { + return executeRequest(request, client()); + } + +- // TODO: this returns json. Only used in GetEndpointQueryIT + protected JSONObject executeQueryWithGetRequest(final String sqlQuery) throws IOException { + + final Request request = buildGetEndpointRequest(sqlQuery); +diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java +index 8c6009248..a585c3c2c 100644 +--- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java ++++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java +@@ -18,17 +18,12 @@ public class ActionRequestRestExecutorFactory { + * call if necessary. + * + * @param format format of response +- * @param queryAction query action + * @return executor + */ +- public static RestExecutor createExecutor(Format format, QueryAction queryAction) { ++ public static RestExecutor createExecutor(Format format) { + switch (format) { + case CSV: + return new AsyncRestExecutor(new CSVResultRestExecutor()); +- case JSON: // TODO: deprecate json +- return new AsyncRestExecutor( +- new ElasticDefaultRestExecutor(queryAction), +- action -> isJoin(action) || isUnionMinus(action)); + case JDBC: + case RAW: + case TABLE: +@@ -36,12 +31,4 @@ public class ActionRequestRestExecutorFactory { + return new AsyncRestExecutor(new PrettyFormatRestExecutor(format.getFormatName())); + } + } +- +- private static boolean isJoin(QueryAction queryAction) { +- return queryAction instanceof OpenSearchJoinQueryAction; +- } +- +- private static boolean isUnionMinus(QueryAction queryAction) { +- return queryAction instanceof MultiQueryAction; +- } + } +diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java +index f17e2a1de..b30602754 100644 +--- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java ++++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java +@@ -14,7 +14,6 @@ import lombok.RequiredArgsConstructor; + @RequiredArgsConstructor + public enum Format { + JDBC("jdbc"), +- JSON("json"), // TODO: deprecate json + CSV("csv"), + RAW("raw"), + TABLE("table"); +diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java +index a0e68ff54..9be2367dc 100644 +--- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java ++++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java +@@ -251,8 +251,7 @@ public class RestSqlAction extends BaseRestHandler { + channel.sendResponse(new BytesRestResponse(OK, "application/json; charset=UTF-8", result)); + } else { + RestExecutor restExecutor = +- ActionRequestRestExecutorFactory.createExecutor( +- SqlRequestParam.getFormat(params), queryAction); ++ ActionRequestRestExecutorFactory.createExecutor(SqlRequestParam.getFormat(params)); + // doing this hack because OpenSearch throws exception for un-consumed props + Map additionalParams = new HashMap<>(); + for (String paramName : responseParams()) { +diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java +index 3924b6e41..93e721e52 100644 +--- a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java ++++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java +@@ -63,8 +63,7 @@ public class OpenSearchActionFactory { + + public static QueryAction create(Client client, String sql) + throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { +- // TODO: deprecate json +- return create(client, new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JSON)); ++ return create(client, new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JDBC)); + } + + /** +@@ -117,7 +116,7 @@ public class OpenSearchActionFactory { + } else { + sqlExpr.accept(new TermFieldRewriter()); + // migrate aggregation to query planner framework. +- if (shouldMigrateToQueryPlan(sqlExpr, request.getFormat())) { ++ if (shouldMigrateToQueryPlan(sqlExpr)) { + return new QueryPlanQueryAction( + new QueryPlanRequestBuilder( + new BindingTupleQueryPlanner(client, sqlExpr, request.getTypeProvider()))); +@@ -192,13 +191,7 @@ public class OpenSearchActionFactory { + } + + @VisibleForTesting +- public static boolean shouldMigrateToQueryPlan(SQLQueryExpr expr, Format format) { +- // TODO: deprecate json +- // The JSON format will return the OpenSearch aggregation result, which is not supported by the +- // QueryPlanner. +- if (format == Format.JSON) { +- return false; +- } ++ public static boolean shouldMigrateToQueryPlan(SQLQueryExpr expr) { + QueryPlannerScopeDecider decider = new QueryPlannerScopeDecider(); + return decider.isInScope(expr); + } +diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java +index b35ee4c99..ee2f3ac74 100644 +--- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java ++++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java +@@ -41,13 +41,6 @@ public class SqlRequestParamTest { + assertFalse(SqlRequestParam.isPrettyFormat(ImmutableMap.of(QUERY_PARAMS_PRETTY, "unknown"))); + } + +- // TODO: deprecate json +- @Test +- public void shouldReturnJSONIfFormatParamsIsJSON() { +- assertEquals( +- Format.JSON, SqlRequestParam.getFormat(ImmutableMap.of(QUERY_PARAMS_FORMAT, "json"))); +- } +- + @Test + public void shouldReturnDefaultFormatIfNoFormatParams() { + assertEquals(Format.JDBC, SqlRequestParam.getFormat(ImmutableMap.of())); +diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java +index 9b550f379..44ad735ac 100644 +--- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java ++++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java +@@ -14,21 +14,12 @@ import org.opensearch.sql.legacy.query.OpenSearchActionFactory; + import org.opensearch.sql.legacy.util.SqlParserUtils; + + public class OpenSearchActionFactoryTest { +- // TODO: deprecate json +- @Test +- public void josnOutputRequestShouldNotMigrateToQueryPlan() { +- String sql = "SELECT age, MAX(balance) FROM account GROUP BY age"; +- +- assertFalse( +- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql), Format.JSON)); +- } +- + @Test + public void nestQueryShouldNotMigrateToQueryPlan() { + String sql = "SELECT age, nested(balance) FROM account GROUP BY age"; + + assertFalse( +- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql), Format.JDBC)); ++ OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); + } + + @Test +@@ -36,7 +27,7 @@ public class OpenSearchActionFactoryTest { + String sql = "SELECT age FROM account "; + + assertFalse( +- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql), Format.JDBC)); ++ OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); + } + + @Test +@@ -44,7 +35,7 @@ public class OpenSearchActionFactoryTest { + String sql = "SELECT age, COUNT(balance) FROM account "; + + assertTrue( +- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql), Format.JDBC)); ++ OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); + } + + @Test +@@ -52,7 +43,7 @@ public class OpenSearchActionFactoryTest { + String sql = "SELECT age, MAX(balance) - MIN(balance) FROM account "; + + assertTrue( +- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql), Format.JDBC)); ++ OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); + } + + @Test +@@ -60,6 +51,6 @@ public class OpenSearchActionFactoryTest { + String sql = "SELECT CAST(age AS DOUBLE) as alias FROM account GROUP BY alias"; + + assertTrue( +- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql), Format.JDBC)); ++ OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); + } + } +diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java +index 5471e37dd..bf5d79855 100644 +--- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java ++++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java +@@ -130,8 +130,7 @@ public class DefaultQueryActionTest { + doReturn(settingFetchSize).when(mockSqlRequest).fetchSize(); + queryAction.setSqlRequest(mockSqlRequest); + +- // TODO: deprecate json +- Format[] formats = new Format[] {Format.CSV, Format.RAW, Format.JSON, Format.TABLE}; ++ Format[] formats = new Format[] {Format.CSV, Format.RAW, Format.TABLE}; + for (Format format : formats) { + queryAction.setFormat(format); + queryAction.checkAndSetScroll(); +-- +2.41.0 + + +From e3157aa37ac977c14ca37aa9156d055475d6e32c Mon Sep 17 00:00:00 2001 +From: Sean Kao +Date: Fri, 28 Feb 2025 08:25:00 -0800 +Subject: [PATCH 04/12] fix test case wip + +Signed-off-by: Sean Kao +--- + .../org/opensearch/sql/legacy/QueryIT.java | 16 +++--- + .../sql/legacy/SQLIntegTestCase.java | 50 ++++++++++++++++--- + 2 files changed, 53 insertions(+), 13 deletions(-) + +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java +index 3fe8b50ee..66d79c2e9 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java +@@ -94,7 +94,6 @@ public class QueryIT extends SQLIntegTestCase { + executeQuery( + String.format( + Locale.ROOT, "SELECT * FROM %s LIMIT 1000", TestsConstants.TEST_INDEX_PHRASE)); +- Assert.assertTrue(response.has("hits")); + Assert.assertEquals(6, getTotalHits(response)); + } + +@@ -107,7 +106,6 @@ public class QueryIT extends SQLIntegTestCase { + "SELECT * FROM %s, %s LIMIT 2000", + TestsConstants.TEST_INDEX_BANK, + TestsConstants.TEST_INDEX_BANK_TWO)); +- Assert.assertTrue(response.has("hits")); + Assert.assertEquals(14, getTotalHits(response)); + } + +@@ -132,7 +130,7 @@ public class QueryIT extends SQLIntegTestCase { + @Test + public void selectAllWithMultipleFields() throws IOException { + JSONObject response = +- executeQuery( ++ executeQuery( // TODO: Multiple entries with same key: age=32 and age=32 + StringUtils.format( + "SELECT *, age, address FROM %s LIMIT 5", TestsConstants.TEST_INDEX_BANK)); + +@@ -152,7 +150,7 @@ public class QueryIT extends SQLIntegTestCase { + @Test + public void selectAllWithFieldAndGroupBy() throws IOException { + JSONObject response = +- executeQuery( ++ executeQuery( // TODO: Multiple entries with same key: age=28 and age=28 + StringUtils.format( + "SELECT *, age FROM %s GROUP BY age LIMIT 10", TestsConstants.TEST_INDEX_BANK)); + +@@ -650,11 +648,11 @@ public class QueryIT extends SQLIntegTestCase { + @Test + public void inTest() throws IOException { + JSONObject response = +- executeQuery( ++ executeQuery( // TODO: can't resolve Symbol(namespace=FIELD_NAME, name=age) in type env + String.format( + Locale.ROOT, + "SELECT age FROM %s WHERE age IN (20, 22) LIMIT 1000", +- TestsConstants.TEST_INDEX_PHRASE)); ++ TestsConstants.TEST_INDEX_PHRASE)); // change to PEOPLE? + + JSONArray hits = getHits(response); + for (int i = 0; i < hits.length(); i++) { +@@ -1186,6 +1184,7 @@ public class QueryIT extends SQLIntegTestCase { + checkResponseSize(response, BANK_INDEX_MALE_FALSE); + } + ++ // TODO: aggregation format is different. check in json format first + @Test + public void testWhereWithBoolIsFalse() throws IOException { + JSONObject response = +@@ -1370,6 +1369,7 @@ public class QueryIT extends SQLIntegTestCase { + Assert.assertEquals(1, getTotalHits(response)); + + JSONObject hit = hits.getJSONObject(0); ++ // TODO: JSONObject["parents"] not found. + Assert.assertEquals("Eddard", getSource(hit).getJSONObject("parents").getString("father")); + } + +@@ -1826,7 +1826,7 @@ public class QueryIT extends SQLIntegTestCase { + JSONArray hits = getHits(response); + for (int i = 0; i < hits.length(); i++) { + JSONObject hit = hits.getJSONObject(i); +- JSONObject highlightFields = hit.getJSONObject("highlight"); ++ JSONObject highlightFields = hit.getJSONObject("highlight"); // TODO: JSONObject["highlight"] not found. + + String phrase = highlightFields.getJSONArray("phrase").getString(0); + Assert.assertTrue(phrase.contains("fox")); +@@ -1963,6 +1963,8 @@ public class QueryIT extends SQLIntegTestCase { + JSONObject hit = getHits(response).getJSONObject(0); + String age = hit.query("/_source/age").toString(); + String cases = age.equals("30") ? "1" : age.equals("40") ? "2" : "0"; ++ // TODO: Cannot invoke "Object.toString()" because the return value of "org.json.JSONObject.query(String)" is null ++ // TODO: search for hit.query + + assertThat(cases, equalTo(hit.query("/fields/cases/0"))); + } +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +index a9179a7b8..7472c3ecf 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +@@ -493,17 +493,55 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { + return String.format("{\"cursor\":\"%s\"}", cursor); + } + ++ /** ++ * Following methods adapts the jdbc response format to maintain compatibility with existing code that ++ * expects legacy json (hits) structure. Instead of refactoring all dependent testing code to work with ++ * the jdbc (schema/datarows) format, this adapter transforms the jdbc format into the json structure. ++ * Jdbc format: ++ * { ++ * "schema": [{"name": "field1", "type": "text"}, ...], ++ * "datarows": [[value1, value2, ...], ...] ++ * } ++ * ++ * Transformed to legacy json format: ++ * { ++ * "hits": [{ ++ * "_source": {"field1": value1, ...} ++ * }, ...] ++ * } ++ */ + protected JSONArray getHits(JSONObject response) { +- Assert.assertTrue(response.getJSONObject("hits").has("hits")); ++ Assert.assertTrue(response.has("schema")); ++ Assert.assertTrue(response.has("datarows")); ++ ++ JSONArray schema = response.getJSONArray("schema"); ++ JSONArray datarows = response.getJSONArray("datarows"); ++ JSONArray hits = new JSONArray(); ++ ++ for (int i = 0; i < datarows.length(); i++) { ++ JSONObject hit = new JSONObject(); ++ JSONObject source = new JSONObject(); ++ JSONArray row = datarows.getJSONArray(i); ++ ++ for (int j = 0; j < schema.length(); j++) { ++ JSONObject schemaField = schema.getJSONObject(j); ++ String fieldName = schemaField.getString("name"); ++ Object value = row.get(j); ++ if (value != JSONObject.NULL) { // TODO: need this? ++ source.put(fieldName, value); ++ } ++ } ++ ++ hit.put("_source", source); ++ hits.put(hit); ++ } + +- return response.getJSONObject("hits").getJSONArray("hits"); ++ return hits; + } + + protected int getTotalHits(JSONObject response) { +- Assert.assertTrue(response.getJSONObject("hits").has("total")); +- Assert.assertTrue(response.getJSONObject("hits").getJSONObject("total").has("value")); +- +- return response.getJSONObject("hits").getJSONObject("total").getInt("value"); ++ Assert.assertTrue(response.has("total")); ++ return response.getInt("total"); + } + + protected JSONObject getSource(JSONObject hit) { +-- +2.41.0 + + +From 85e2566d4c6ae114c7ec9b264ac0dae2d7738d9c Mon Sep 17 00:00:00 2001 +From: Sean Kao +Date: Fri, 28 Feb 2025 09:29:11 -0800 +Subject: [PATCH 05/12] Revert "fix test case wip" + +This reverts commit e3157aa37ac977c14ca37aa9156d055475d6e32c. + +Signed-off-by: Sean Kao +--- + .../org/opensearch/sql/legacy/QueryIT.java | 16 +++--- + .../sql/legacy/SQLIntegTestCase.java | 50 +++---------------- + 2 files changed, 13 insertions(+), 53 deletions(-) + +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java +index 66d79c2e9..3fe8b50ee 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java +@@ -94,6 +94,7 @@ public class QueryIT extends SQLIntegTestCase { + executeQuery( + String.format( + Locale.ROOT, "SELECT * FROM %s LIMIT 1000", TestsConstants.TEST_INDEX_PHRASE)); ++ Assert.assertTrue(response.has("hits")); + Assert.assertEquals(6, getTotalHits(response)); + } + +@@ -106,6 +107,7 @@ public class QueryIT extends SQLIntegTestCase { + "SELECT * FROM %s, %s LIMIT 2000", + TestsConstants.TEST_INDEX_BANK, + TestsConstants.TEST_INDEX_BANK_TWO)); ++ Assert.assertTrue(response.has("hits")); + Assert.assertEquals(14, getTotalHits(response)); + } + +@@ -130,7 +132,7 @@ public class QueryIT extends SQLIntegTestCase { + @Test + public void selectAllWithMultipleFields() throws IOException { + JSONObject response = +- executeQuery( // TODO: Multiple entries with same key: age=32 and age=32 ++ executeQuery( + StringUtils.format( + "SELECT *, age, address FROM %s LIMIT 5", TestsConstants.TEST_INDEX_BANK)); + +@@ -150,7 +152,7 @@ public class QueryIT extends SQLIntegTestCase { + @Test + public void selectAllWithFieldAndGroupBy() throws IOException { + JSONObject response = +- executeQuery( // TODO: Multiple entries with same key: age=28 and age=28 ++ executeQuery( + StringUtils.format( + "SELECT *, age FROM %s GROUP BY age LIMIT 10", TestsConstants.TEST_INDEX_BANK)); + +@@ -648,11 +650,11 @@ public class QueryIT extends SQLIntegTestCase { + @Test + public void inTest() throws IOException { + JSONObject response = +- executeQuery( // TODO: can't resolve Symbol(namespace=FIELD_NAME, name=age) in type env ++ executeQuery( + String.format( + Locale.ROOT, + "SELECT age FROM %s WHERE age IN (20, 22) LIMIT 1000", +- TestsConstants.TEST_INDEX_PHRASE)); // change to PEOPLE? ++ TestsConstants.TEST_INDEX_PHRASE)); + + JSONArray hits = getHits(response); + for (int i = 0; i < hits.length(); i++) { +@@ -1184,7 +1186,6 @@ public class QueryIT extends SQLIntegTestCase { + checkResponseSize(response, BANK_INDEX_MALE_FALSE); + } + +- // TODO: aggregation format is different. check in json format first + @Test + public void testWhereWithBoolIsFalse() throws IOException { + JSONObject response = +@@ -1369,7 +1370,6 @@ public class QueryIT extends SQLIntegTestCase { + Assert.assertEquals(1, getTotalHits(response)); + + JSONObject hit = hits.getJSONObject(0); +- // TODO: JSONObject["parents"] not found. + Assert.assertEquals("Eddard", getSource(hit).getJSONObject("parents").getString("father")); + } + +@@ -1826,7 +1826,7 @@ public class QueryIT extends SQLIntegTestCase { + JSONArray hits = getHits(response); + for (int i = 0; i < hits.length(); i++) { + JSONObject hit = hits.getJSONObject(i); +- JSONObject highlightFields = hit.getJSONObject("highlight"); // TODO: JSONObject["highlight"] not found. ++ JSONObject highlightFields = hit.getJSONObject("highlight"); + + String phrase = highlightFields.getJSONArray("phrase").getString(0); + Assert.assertTrue(phrase.contains("fox")); +@@ -1963,8 +1963,6 @@ public class QueryIT extends SQLIntegTestCase { + JSONObject hit = getHits(response).getJSONObject(0); + String age = hit.query("/_source/age").toString(); + String cases = age.equals("30") ? "1" : age.equals("40") ? "2" : "0"; +- // TODO: Cannot invoke "Object.toString()" because the return value of "org.json.JSONObject.query(String)" is null +- // TODO: search for hit.query + + assertThat(cases, equalTo(hit.query("/fields/cases/0"))); + } +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +index 7472c3ecf..a9179a7b8 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +@@ -493,55 +493,17 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { + return String.format("{\"cursor\":\"%s\"}", cursor); + } + +- /** +- * Following methods adapts the jdbc response format to maintain compatibility with existing code that +- * expects legacy json (hits) structure. Instead of refactoring all dependent testing code to work with +- * the jdbc (schema/datarows) format, this adapter transforms the jdbc format into the json structure. +- * Jdbc format: +- * { +- * "schema": [{"name": "field1", "type": "text"}, ...], +- * "datarows": [[value1, value2, ...], ...] +- * } +- * +- * Transformed to legacy json format: +- * { +- * "hits": [{ +- * "_source": {"field1": value1, ...} +- * }, ...] +- * } +- */ + protected JSONArray getHits(JSONObject response) { +- Assert.assertTrue(response.has("schema")); +- Assert.assertTrue(response.has("datarows")); +- +- JSONArray schema = response.getJSONArray("schema"); +- JSONArray datarows = response.getJSONArray("datarows"); +- JSONArray hits = new JSONArray(); +- +- for (int i = 0; i < datarows.length(); i++) { +- JSONObject hit = new JSONObject(); +- JSONObject source = new JSONObject(); +- JSONArray row = datarows.getJSONArray(i); +- +- for (int j = 0; j < schema.length(); j++) { +- JSONObject schemaField = schema.getJSONObject(j); +- String fieldName = schemaField.getString("name"); +- Object value = row.get(j); +- if (value != JSONObject.NULL) { // TODO: need this? +- source.put(fieldName, value); +- } +- } +- +- hit.put("_source", source); +- hits.put(hit); +- } ++ Assert.assertTrue(response.getJSONObject("hits").has("hits")); + +- return hits; ++ return response.getJSONObject("hits").getJSONArray("hits"); + } + + protected int getTotalHits(JSONObject response) { +- Assert.assertTrue(response.has("total")); +- return response.getInt("total"); ++ Assert.assertTrue(response.getJSONObject("hits").has("total")); ++ Assert.assertTrue(response.getJSONObject("hits").getJSONObject("total").has("value")); ++ ++ return response.getJSONObject("hits").getJSONObject("total").getInt("value"); + } + + protected JSONObject getSource(JSONObject hit) { +-- +2.41.0 + + +From 5f0ac18fda02a46fe3bb81676e6c110b49164e22 Mon Sep 17 00:00:00 2001 +From: Sean Kao +Date: Fri, 28 Feb 2025 10:51:29 -0800 +Subject: [PATCH 06/12] ignore legacy test using json response format + +Signed-off-by: Sean Kao +--- + .../opensearch/sql/legacy/AggregationIT.java | 1 + + .../org/opensearch/sql/legacy/DateFormatIT.java | 1 + + .../org/opensearch/sql/legacy/HashJoinIT.java | 2 ++ + .../java/org/opensearch/sql/legacy/JoinIT.java | 1 + + .../org/opensearch/sql/legacy/MultiQueryIT.java | 2 ++ + .../java/org/opensearch/sql/legacy/OrderIT.java | 2 ++ + .../org/opensearch/sql/legacy/PluginIT.java | 2 ++ + .../java/org/opensearch/sql/legacy/QueryIT.java | 1 + + .../opensearch/sql/legacy/SQLIntegTestCase.java | 17 +++++++++++++++-- + 9 files changed, 27 insertions(+), 2 deletions(-) + +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/AggregationIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/AggregationIT.java +index 490e9eb51..c2b24c9bb 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/AggregationIT.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/AggregationIT.java +@@ -38,6 +38,7 @@ import org.junit.Assert; + import org.junit.Ignore; + import org.junit.Test; + ++@Ignore + public class AggregationIT extends SQLIntegTestCase { + + @Override +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/DateFormatIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/DateFormatIT.java +index 388d90092..af88c4e57 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/DateFormatIT.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/DateFormatIT.java +@@ -29,6 +29,7 @@ import org.junit.Ignore; + import org.junit.Test; + import org.opensearch.sql.legacy.exception.SqlParseException; + ++@Ignore + public class DateFormatIT extends SQLIntegTestCase { + + private static final String SELECT_FROM = +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/HashJoinIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/HashJoinIT.java +index 02c55d8eb..f27c18b24 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/HashJoinIT.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/HashJoinIT.java +@@ -19,8 +19,10 @@ import java.util.Set; + import org.json.JSONArray; + import org.json.JSONObject; + import org.junit.Assert; ++import org.junit.Ignore; + import org.junit.Test; + ++@Ignore + /** Test new hash join algorithm by comparison with old implementation. */ + public class HashJoinIT extends SQLIntegTestCase { + +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/JoinIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/JoinIT.java +index 8c2ea9647..af5434e99 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/JoinIT.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/JoinIT.java +@@ -31,6 +31,7 @@ import org.junit.Assert; + import org.junit.Ignore; + import org.junit.Test; + ++@Ignore + public class JoinIT extends SQLIntegTestCase { + + private static final String USE_NL_HINT = " /*! USE_NL*/"; +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/MultiQueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/MultiQueryIT.java +index bee85ac31..9fdb0f30d 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/MultiQueryIT.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/MultiQueryIT.java +@@ -14,8 +14,10 @@ import java.util.Set; + import org.json.JSONArray; + import org.json.JSONObject; + import org.junit.Assert; ++import org.junit.Ignore; + import org.junit.Test; + ++@Ignore + public class MultiQueryIT extends SQLIntegTestCase { + + private static final String MINUS_SCROLL_DEFAULT_HINT = +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/OrderIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/OrderIT.java +index 20bed5d2e..ce46de48e 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/OrderIT.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/OrderIT.java +@@ -10,8 +10,10 @@ import static org.hamcrest.Matchers.equalTo; + import java.io.IOException; + import org.json.JSONArray; + import org.json.JSONObject; ++import org.junit.Ignore; + import org.junit.Test; + ++@Ignore + public class OrderIT extends SQLIntegTestCase { + + @Override +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java +index 9305b1655..83fe0b94b 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java +@@ -12,6 +12,7 @@ import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_ACCOUNT; + import java.io.IOException; + import java.util.Locale; + import org.json.JSONObject; ++import org.junit.Ignore; + import org.junit.Test; + import org.opensearch.client.Request; + import org.opensearch.client.RequestOptions; +@@ -19,6 +20,7 @@ import org.opensearch.client.Response; + import org.opensearch.client.ResponseException; + import org.opensearch.sql.plugin.rest.RestQuerySettingsAction; + ++@Ignore + public class PluginIT extends SQLIntegTestCase { + + @Override +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java +index 3fe8b50ee..aa905c6c4 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java +@@ -40,6 +40,7 @@ import org.opensearch.client.ResponseException; + import org.opensearch.core.rest.RestStatus; + import org.opensearch.sql.legacy.utils.StringUtils; + ++@Ignore + public class QueryIT extends SQLIntegTestCase { + + /** +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +index a9179a7b8..c324b1090 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +@@ -228,8 +228,13 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { + loadIndex(index, client()); + } + ++ /** ++ * TODO: Decide what to do with legacy tests using json response format. ++ * OpenSearch DSL format is deprecated. Need to ensure that requests in legacy tests using the json response format ++ * are not invoked. ++ */ + protected Request getSqlRequest(String request, boolean explain) { +- return getSqlRequest(request, explain, "jdbc"); ++ return getSqlRequest(request, explain, "json"); + } + + protected Request getSqlRequest(String request, boolean explain, String requestType) { +@@ -330,6 +335,11 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { + return responseString; + } + ++ /** ++ * TODO: Decide what to do with legacy tests using json response format. ++ * OpenSearch DSL format is deprecated. Need to ensure that requests in legacy tests using the json response format ++ * are not invoked. ++ */ + protected Request buildGetEndpointRequest(final String sqlQuery) { + + final String utf8CharsetName = StandardCharsets.UTF_8.name(); +@@ -344,7 +354,7 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { + + final String requestUrl = + String.format( +- Locale.ROOT, "%s?sql=%s&format=%s", QUERY_API_ENDPOINT, urlEncodedQuery, "jdbc"); ++ Locale.ROOT, "%s?sql=%s&format=%s", QUERY_API_ENDPOINT, urlEncodedQuery, "json"); + return new Request("GET", requestUrl); + } + +@@ -385,6 +395,9 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { + + protected static String executeRequest(final Request request, RestClient client) + throws IOException { ++ if (request.getParameters().get("format") == "json") { ++ throw new IOException("request type is json"); ++ } + Response response = client.performRequest(request); + Assert.assertEquals(200, response.getStatusLine().getStatusCode()); + return getResponseBody(response); +-- +2.41.0 + + +From 3cbaf825356abba4b4de34d8b96bd13d73f0c124 Mon Sep 17 00:00:00 2001 +From: Sean Kao +Date: Fri, 28 Feb 2025 11:00:18 -0800 +Subject: [PATCH 07/12] spotlessApply + +Signed-off-by: Sean Kao +--- + .tool-versions | 1 + + .../opensearch/sql/legacy/SQLIntegTestCase.java | 12 ++++++------ + .../ActionRequestRestExecutorFactory.java | 3 --- + .../planner/OpenSearchActionFactoryTest.java | 16 +++++----------- + 4 files changed, 12 insertions(+), 20 deletions(-) + create mode 100644 .tool-versions + +diff --git a/.tool-versions b/.tool-versions +new file mode 100644 +index 000000000..e95b9595e +--- /dev/null ++++ b/.tool-versions +@@ -0,0 +1 @@ ++java adoptopenjdk-21.0.5+11.0.LTS +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +index c324b1090..852675c8c 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +@@ -229,9 +229,9 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { + } + + /** +- * TODO: Decide what to do with legacy tests using json response format. +- * OpenSearch DSL format is deprecated. Need to ensure that requests in legacy tests using the json response format +- * are not invoked. ++ * TODO: Decide what to do with legacy tests using json response format. OpenSearch DSL format is ++ * deprecated. Need to ensure that requests in legacy tests using the json response format are not ++ * invoked. + */ + protected Request getSqlRequest(String request, boolean explain) { + return getSqlRequest(request, explain, "json"); +@@ -336,9 +336,9 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { + } + + /** +- * TODO: Decide what to do with legacy tests using json response format. +- * OpenSearch DSL format is deprecated. Need to ensure that requests in legacy tests using the json response format +- * are not invoked. ++ * TODO: Decide what to do with legacy tests using json response format. OpenSearch DSL format is ++ * deprecated. Need to ensure that requests in legacy tests using the json response format are not ++ * invoked. + */ + protected Request buildGetEndpointRequest(final String sqlQuery) { + +diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java +index a585c3c2c..068c1b433 100644 +--- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java ++++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java +@@ -7,9 +7,6 @@ package org.opensearch.sql.legacy.executor; + + import org.opensearch.sql.legacy.executor.csv.CSVResultRestExecutor; + import org.opensearch.sql.legacy.executor.format.PrettyFormatRestExecutor; +-import org.opensearch.sql.legacy.query.QueryAction; +-import org.opensearch.sql.legacy.query.join.OpenSearchJoinQueryAction; +-import org.opensearch.sql.legacy.query.multi.MultiQueryAction; + + /** Created by Eliran on 26/12/2015. */ + public class ActionRequestRestExecutorFactory { +diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java +index 44ad735ac..008b0d618 100644 +--- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java ++++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java +@@ -9,7 +9,6 @@ import static org.junit.Assert.assertFalse; + import static org.junit.Assert.assertTrue; + + import org.junit.Test; +-import org.opensearch.sql.legacy.executor.Format; + import org.opensearch.sql.legacy.query.OpenSearchActionFactory; + import org.opensearch.sql.legacy.util.SqlParserUtils; + +@@ -18,39 +17,34 @@ public class OpenSearchActionFactoryTest { + public void nestQueryShouldNotMigrateToQueryPlan() { + String sql = "SELECT age, nested(balance) FROM account GROUP BY age"; + +- assertFalse( +- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); ++ assertFalse(OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); + } + + @Test + public void nonAggregationQueryShouldNotMigrateToQueryPlan() { + String sql = "SELECT age FROM account "; + +- assertFalse( +- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); ++ assertFalse(OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); + } + + @Test + public void aggregationQueryWithoutGroupByShouldMigrateToQueryPlan() { + String sql = "SELECT age, COUNT(balance) FROM account "; + +- assertTrue( +- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); ++ assertTrue(OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); + } + + @Test + public void aggregationQueryWithExpressionByShouldMigrateToQueryPlan() { + String sql = "SELECT age, MAX(balance) - MIN(balance) FROM account "; + +- assertTrue( +- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); ++ assertTrue(OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); + } + + @Test + public void queryOnlyHasGroupByShouldMigrateToQueryPlan() { + String sql = "SELECT CAST(age AS DOUBLE) as alias FROM account GROUP BY alias"; + +- assertTrue( +- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); ++ assertTrue(OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); + } + } +-- +2.41.0 + + +From c74b96b8a7e9f2e35a012eaf80f93c0a249b430d Mon Sep 17 00:00:00 2001 +From: Sean Kao +Date: Fri, 28 Feb 2025 11:58:15 -0800 +Subject: [PATCH 08/12] remove json param check in executeRequest + +Signed-off-by: Sean Kao +--- + .../test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java | 3 --- + 1 file changed, 3 deletions(-) + +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +index 852675c8c..dc96e30cb 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +@@ -395,9 +395,6 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { + + protected static String executeRequest(final Request request, RestClient client) + throws IOException { +- if (request.getParameters().get("format") == "json") { +- throw new IOException("request type is json"); +- } + Response response = client.performRequest(request); + Assert.assertEquals(200, response.getStatusLine().getStatusCode()); + return getResponseBody(response); +-- +2.41.0 + + +From 48d512aa3632998409359ee4465a6c621bf3bbd8 Mon Sep 17 00:00:00 2001 +From: Sean Kao +Date: Fri, 28 Feb 2025 15:43:19 -0800 +Subject: [PATCH 09/12] fix getScriptFieldFromQuery can't get requestBuilder + from QueryPlan + +Signed-off-by: Sean Kao +--- + .../org/opensearch/sql/legacy/plugin/SearchDao.java | 2 +- + .../sql/legacy/query/OpenSearchActionFactory.java | 12 +++++++++--- + .../sql/legacy/unittest/JSONRequestTest.java | 2 +- + .../sql/legacy/util/CheckScriptContents.java | 3 ++- + 4 files changed, 13 insertions(+), 6 deletions(-) + +diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/SearchDao.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/SearchDao.java +index 415b98f6b..10ca9ddae 100644 +--- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/SearchDao.java ++++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/SearchDao.java +@@ -45,6 +45,6 @@ public class SearchDao { + */ + public QueryAction explain(QueryActionRequest queryActionRequest) + throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { +- return OpenSearchActionFactory.create(client, queryActionRequest); ++ return OpenSearchActionFactory.create(client, queryActionRequest, false); + } + } +diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java +index 93e721e52..2f6ea1379 100644 +--- a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java ++++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java +@@ -63,16 +63,22 @@ public class OpenSearchActionFactory { + + public static QueryAction create(Client client, String sql) + throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { +- return create(client, new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JDBC)); ++ return create(client, sql, false); ++ } ++ ++ public static QueryAction create(Client client, String sql, boolean bypassMigrateToQueryPlan) ++ throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { ++ return create(client, new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JDBC), bypassMigrateToQueryPlan); + } + + /** + * Create the compatible Query object based on the SQL query. + * + * @param request The SQL query. ++ * @param bypassMigrateToQueryPlan Avoid using QueryPlan. + * @return Query object. + */ +- public static QueryAction create(Client client, QueryActionRequest request) ++ public static QueryAction create(Client client, QueryActionRequest request, boolean bypassMigrateToQueryPlan) + throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { + String sql = request.getSql(); + // Remove line breaker anywhere and semicolon at the end +@@ -116,7 +122,7 @@ public class OpenSearchActionFactory { + } else { + sqlExpr.accept(new TermFieldRewriter()); + // migrate aggregation to query planner framework. +- if (shouldMigrateToQueryPlan(sqlExpr)) { ++ if (!bypassMigrateToQueryPlan && shouldMigrateToQueryPlan(sqlExpr)) { + return new QueryPlanQueryAction( + new QueryPlanRequestBuilder( + new BindingTupleQueryPlanner(client, sqlExpr, request.getTypeProvider()))); +diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/JSONRequestTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/JSONRequestTest.java +index 5cf9c602a..c0ea8581e 100644 +--- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/JSONRequestTest.java ++++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/JSONRequestTest.java +@@ -435,7 +435,7 @@ public class JSONRequestTest { + CheckScriptContents.stubMockClient(mockClient); + QueryAction queryAction = + OpenSearchActionFactory.create( +- mockClient, new QueryActionRequest(sql, columnTypeProvider, Format.JDBC)); ++ mockClient, new QueryActionRequest(sql, columnTypeProvider, Format.JDBC), false); + + SqlRequest sqlRequest = new SqlRequest(sql, jsonRequest); + queryAction.setSqlRequest(sqlRequest); +diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/util/CheckScriptContents.java b/legacy/src/test/java/org/opensearch/sql/legacy/util/CheckScriptContents.java +index 2fe711c3b..c15bbc39f 100644 +--- a/legacy/src/test/java/org/opensearch/sql/legacy/util/CheckScriptContents.java ++++ b/legacy/src/test/java/org/opensearch/sql/legacy/util/CheckScriptContents.java +@@ -63,7 +63,8 @@ public class CheckScriptContents { + try { + Client mockClient = mock(Client.class); + stubMockClient(mockClient); +- QueryAction queryAction = OpenSearchActionFactory.create(mockClient, query); ++ // Avoid creating QueryPlanQueryAction so that scriptFields is available ++ QueryAction queryAction = OpenSearchActionFactory.create(mockClient, query, true); + SqlElasticRequestBuilder requestBuilder = queryAction.explain(); + + SearchRequestBuilder request = (SearchRequestBuilder) requestBuilder.getBuilder(); +-- +2.41.0 + + +From d0c1ef1b070c3c14817d2fef07bf9b2a2cef7855 Mon Sep 17 00:00:00 2001 +From: Sean Kao +Date: Fri, 28 Feb 2025 15:52:57 -0800 +Subject: [PATCH 10/12] spotlessApply + +Signed-off-by: Sean Kao +--- + .../sql/legacy/query/OpenSearchActionFactory.java | 8 ++++++-- + 1 file changed, 6 insertions(+), 2 deletions(-) + +diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java +index 2f6ea1379..ffa5b24fd 100644 +--- a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java ++++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java +@@ -68,7 +68,10 @@ public class OpenSearchActionFactory { + + public static QueryAction create(Client client, String sql, boolean bypassMigrateToQueryPlan) + throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { +- return create(client, new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JDBC), bypassMigrateToQueryPlan); ++ return create( ++ client, ++ new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JDBC), ++ bypassMigrateToQueryPlan); + } + + /** +@@ -78,7 +81,8 @@ public class OpenSearchActionFactory { + * @param bypassMigrateToQueryPlan Avoid using QueryPlan. + * @return Query object. + */ +- public static QueryAction create(Client client, QueryActionRequest request, boolean bypassMigrateToQueryPlan) ++ public static QueryAction create( ++ Client client, QueryActionRequest request, boolean bypassMigrateToQueryPlan) + throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { + String sql = request.getSql(); + // Remove line breaker anywhere and semicolon at the end +-- +2.41.0 + + +From 82cfce09b145f8f3abad284bb61a5f7be058073c Mon Sep 17 00:00:00 2001 +From: Sean Kao +Date: Fri, 28 Feb 2025 16:45:42 -0800 +Subject: [PATCH 11/12] ignore test using json + +Signed-off-by: Sean Kao +--- + .../test/java/org/opensearch/sql/legacy/DateFunctionsIT.java | 2 ++ + 1 file changed, 2 insertions(+) + +diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/DateFunctionsIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/DateFunctionsIT.java +index d9a6849fc..81206cf76 100644 +--- a/integ-test/src/test/java/org/opensearch/sql/legacy/DateFunctionsIT.java ++++ b/integ-test/src/test/java/org/opensearch/sql/legacy/DateFunctionsIT.java +@@ -17,6 +17,7 @@ import org.joda.time.DateTime; + import org.joda.time.format.DateTimeFormat; + import org.joda.time.format.DateTimeFormatter; + import org.json.JSONObject; ++import org.junit.Ignore; + import org.junit.Test; + import org.opensearch.action.search.SearchResponse; + import org.opensearch.common.xcontent.LoggingDeprecationHandler; +@@ -25,6 +26,7 @@ import org.opensearch.core.xcontent.NamedXContentRegistry; + import org.opensearch.core.xcontent.XContentParser; + import org.opensearch.search.SearchHit; + ++@Ignore + public class DateFunctionsIT extends SQLIntegTestCase { + + private static final String FROM = "FROM " + TestsConstants.TEST_INDEX_ONLINE; +-- +2.41.0 + + +From 5f38dd52f8c870a408a9ea9f6323df4e1bc90c22 Mon Sep 17 00:00:00 2001 +From: Sean Kao +Date: Fri, 28 Feb 2025 16:53:47 -0800 +Subject: [PATCH 12/12] fix test case + +Signed-off-by: Sean Kao +--- + .../sql/legacy/unittest/query/DefaultQueryActionTest.java | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java +index bf5d79855..4c6061ff5 100644 +--- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java ++++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java +@@ -136,7 +136,7 @@ public class DefaultQueryActionTest { + queryAction.checkAndSetScroll(); + } + +- Mockito.verify(mockRequestBuilder, times(4)).setSize(limit); ++ Mockito.verify(mockRequestBuilder, times(3)).setSize(limit); + Mockito.verify(mockRequestBuilder, never()).setScroll(any(TimeValue.class)); + + queryAction.setFormat(Format.JDBC); +-- +2.41.0 + From 3b50834b445a2d44d97bfcfdd584637a94638e2c Mon Sep 17 00:00:00 2001 From: Sean Kao Date: Sat, 1 Mar 2025 10:15:48 -0800 Subject: [PATCH 14/21] fix json call Signed-off-by: Sean Kao --- .../opensearch/sql/legacy/PreparedStatementIT.java | 4 ++++ .../opensearch/sql/legacy/SQLIntegTestCase.java | 14 ++------------ 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/PreparedStatementIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/PreparedStatementIT.java index dd177ec1f14..ca7fccda187 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/PreparedStatementIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/PreparedStatementIT.java @@ -9,10 +9,14 @@ import org.json.JSONArray; import org.json.JSONObject; import org.junit.Assert; +import org.junit.Ignore; import org.junit.Test; // Refer to https://www.elastic.co/guide/en/elasticsearch/reference/6.5/integration-tests.html // for detailed OpenSearchIntegTestCase usages doc. +@Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") public class PreparedStatementIT extends SQLIntegTestCase { @Override diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java index dc96e30cb80..a9179a7b820 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java @@ -228,13 +228,8 @@ protected synchronized void loadIndex(Index index) throws IOException { loadIndex(index, client()); } - /** - * TODO: Decide what to do with legacy tests using json response format. OpenSearch DSL format is - * deprecated. Need to ensure that requests in legacy tests using the json response format are not - * invoked. - */ protected Request getSqlRequest(String request, boolean explain) { - return getSqlRequest(request, explain, "json"); + return getSqlRequest(request, explain, "jdbc"); } protected Request getSqlRequest(String request, boolean explain, String requestType) { @@ -335,11 +330,6 @@ protected String executeFetchLessQuery(String query, String requestType) throws return responseString; } - /** - * TODO: Decide what to do with legacy tests using json response format. OpenSearch DSL format is - * deprecated. Need to ensure that requests in legacy tests using the json response format are not - * invoked. - */ protected Request buildGetEndpointRequest(final String sqlQuery) { final String utf8CharsetName = StandardCharsets.UTF_8.name(); @@ -354,7 +344,7 @@ protected Request buildGetEndpointRequest(final String sqlQuery) { final String requestUrl = String.format( - Locale.ROOT, "%s?sql=%s&format=%s", QUERY_API_ENDPOINT, urlEncodedQuery, "json"); + Locale.ROOT, "%s?sql=%s&format=%s", QUERY_API_ENDPOINT, urlEncodedQuery, "jdbc"); return new Request("GET", requestUrl); } From bf12b0f7339a1c7710f69bed7b74813851a853db Mon Sep 17 00:00:00 2001 From: Sean Kao Date: Mon, 3 Mar 2025 09:12:03 -0800 Subject: [PATCH 15/21] remove patch file Signed-off-by: Sean Kao --- my.patch | 1495 ------------------------------------------------------ 1 file changed, 1495 deletions(-) delete mode 100644 my.patch diff --git a/my.patch b/my.patch deleted file mode 100644 index c0e4bb9bbc9..00000000000 --- a/my.patch +++ /dev/null @@ -1,1495 +0,0 @@ -From bd434ec5105b90491c1eb0f7129b606655465919 Mon Sep 17 00:00:00 2001 -From: Sean Kao -Date: Wed, 26 Feb 2025 09:14:40 -0800 -Subject: [PATCH 01/12] mark places to change - -Signed-off-by: Sean Kao ---- - docs/user/interfaces/protocol.rst | 1 + - .../test/java/org/opensearch/sql/legacy/ExplainIT.java | 1 + - .../org/opensearch/sql/legacy/GetEndpointQueryIT.java | 1 + - .../opensearch/sql/legacy/OpenSearchSQLRestTestCase.java | 1 + - .../java/org/opensearch/sql/legacy/SQLIntegTestCase.java | 8 ++++++++ - .../legacy/executor/ActionRequestRestExecutorFactory.java | 2 +- - .../java/org/opensearch/sql/legacy/executor/Format.java | 2 +- - .../sql/legacy/query/OpenSearchActionFactory.java | 2 ++ - .../sql/legacy/unittest/SqlRequestParamTest.java | 1 + - .../unittest/planner/OpenSearchActionFactoryTest.java | 1 + - .../sql/legacy/unittest/query/DefaultQueryActionTest.java | 1 + - 11 files changed, 19 insertions(+), 2 deletions(-) - -diff --git a/docs/user/interfaces/protocol.rst b/docs/user/interfaces/protocol.rst -index 52674b992..24e803ce9 100644 ---- a/docs/user/interfaces/protocol.rst -+++ b/docs/user/interfaces/protocol.rst -@@ -208,6 +208,7 @@ Result set:: - "status" : 400 - } - -+// TODO: deprecate json - OpenSearch DSL - ============== - -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java -index 1b740924c..2059c8462 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java -@@ -245,6 +245,7 @@ public class ExplainIT extends SQLIntegTestCase { - result.replaceAll("\\s+", ""), equalTo(expectedOutput.replaceAll("\\s+", ""))); - } - -+ // TODO: is this test running without @Test? - public void testContentTypeOfExplainRequestShouldBeJson() throws IOException { - String query = makeRequest("SELECT firstname FROM opensearch-sql_test_index_account"); - Request request = getSqlRequest(query, true); -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java -index 6cc4aba81..a74ddde65 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java -@@ -13,6 +13,7 @@ import org.junit.Test; - import org.junit.rules.ExpectedException; - import org.opensearch.client.ResponseException; - -+// TODO: inaccurate comment? Find out purpose of this test - /** Tests to cover requests with "?format=csv" parameter */ - public class GetEndpointQueryIT extends SQLIntegTestCase { - -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java -index ced69d54a..6b57310a9 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java -@@ -181,6 +181,7 @@ public abstract class OpenSearchSQLRestTestCase extends OpenSearchRestTestCase { - } - } - -+ // TODO: deprecate json - protected static void wipeAllOpenSearchIndices(RestClient client) throws IOException { - // include all the indices, included hidden indices. - // https://www.elastic.co/guide/en/elasticsearch/reference/current/cat-indices.html#cat-indices-api-query-params -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java -index f59fe82fa..213401ec5 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java -@@ -228,6 +228,8 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { - loadIndex(index, client()); - } - -+ // TODO: deprecate json -+ // Note that explain queries are still supposed to return json - protected Request getSqlRequest(String request, boolean explain) { - return getSqlRequest(request, explain, "json"); - } -@@ -266,10 +268,12 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { - } - } - -+ // DONE: verified all usages have requestType="jdbc" except in CsvFormatIT and RawFormatIT - protected String executeQuery(String query, String requestType) { - return executeQuery(query, requestType, Map.of()); - } - -+ // DONE: verified usages don't contain requestType="json" - protected String executeQuery(String query, String requestType, Map params) { - try { - String endpoint = "/_plugins/_sql?format=" + requestType; -@@ -293,6 +297,7 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { - return new JSONObject(executeQuery(query, "jdbc")); - } - -+ // DONE: verified usages don't contain requestType="json" - protected String executeFetchQuery(String query, int fetchSize, String requestType) - throws IOException { - String endpoint = "/_plugins/_sql?format=" + requestType; -@@ -317,6 +322,7 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { - return executeQueryTemplate(queryTemplate, index, 4); - } - -+ // DONE: verified usages don't contain requestType="json" - protected String executeFetchLessQuery(String query, String requestType) throws IOException { - - String endpoint = "/_plugins/_sql?format=" + requestType; -@@ -342,6 +348,7 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { - Assert.fail(utf8CharsetName + " not available"); - } - -+ // TODO: deprecate json - final String requestUrl = - String.format( - Locale.ROOT, "%s?sql=%s&format=%s", QUERY_API_ENDPOINT, urlEncodedQuery, "json"); -@@ -394,6 +401,7 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { - return executeRequest(request, client()); - } - -+ // TODO: this returns json. Only used in GetEndpointQueryIT - protected JSONObject executeQueryWithGetRequest(final String sqlQuery) throws IOException { - - final Request request = buildGetEndpointRequest(sqlQuery); -diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java -index c58bba9e2..8c6009248 100644 ---- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java -+++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java -@@ -25,7 +25,7 @@ public class ActionRequestRestExecutorFactory { - switch (format) { - case CSV: - return new AsyncRestExecutor(new CSVResultRestExecutor()); -- case JSON: -+ case JSON: // TODO: deprecate json - return new AsyncRestExecutor( - new ElasticDefaultRestExecutor(queryAction), - action -> isJoin(action) || isUnionMinus(action)); -diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java -index c47092f10..f17e2a1de 100644 ---- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java -+++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java -@@ -14,7 +14,7 @@ import lombok.RequiredArgsConstructor; - @RequiredArgsConstructor - public enum Format { - JDBC("jdbc"), -- JSON("json"), -+ JSON("json"), // TODO: deprecate json - CSV("csv"), - RAW("raw"), - TABLE("table"); -diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java -index 49aa38ae5..3924b6e41 100644 ---- a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java -+++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java -@@ -63,6 +63,7 @@ public class OpenSearchActionFactory { - - public static QueryAction create(Client client, String sql) - throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { -+ // TODO: deprecate json - return create(client, new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JSON)); - } - -@@ -192,6 +193,7 @@ public class OpenSearchActionFactory { - - @VisibleForTesting - public static boolean shouldMigrateToQueryPlan(SQLQueryExpr expr, Format format) { -+ // TODO: deprecate json - // The JSON format will return the OpenSearch aggregation result, which is not supported by the - // QueryPlanner. - if (format == Format.JSON) { -diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java -index 0d29b5510..b35ee4c99 100644 ---- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java -+++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java -@@ -41,6 +41,7 @@ public class SqlRequestParamTest { - assertFalse(SqlRequestParam.isPrettyFormat(ImmutableMap.of(QUERY_PARAMS_PRETTY, "unknown"))); - } - -+ // TODO: deprecate json - @Test - public void shouldReturnJSONIfFormatParamsIsJSON() { - assertEquals( -diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java -index 3443c2dec..9b550f379 100644 ---- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java -+++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java -@@ -14,6 +14,7 @@ import org.opensearch.sql.legacy.query.OpenSearchActionFactory; - import org.opensearch.sql.legacy.util.SqlParserUtils; - - public class OpenSearchActionFactoryTest { -+ // TODO: deprecate json - @Test - public void josnOutputRequestShouldNotMigrateToQueryPlan() { - String sql = "SELECT age, MAX(balance) FROM account GROUP BY age"; -diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java -index d290e4dd5..5471e37dd 100644 ---- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java -+++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java -@@ -130,6 +130,7 @@ public class DefaultQueryActionTest { - doReturn(settingFetchSize).when(mockSqlRequest).fetchSize(); - queryAction.setSqlRequest(mockSqlRequest); - -+ // TODO: deprecate json - Format[] formats = new Format[] {Format.CSV, Format.RAW, Format.JSON, Format.TABLE}; - for (Format format : formats) { - queryAction.setFormat(format); --- -2.41.0 - - -From 613da7c026c6b038af1951ab5489b6f87fe0bc08 Mon Sep 17 00:00:00 2001 -From: Sean Kao -Date: Thu, 27 Feb 2025 09:25:35 -0800 -Subject: [PATCH 02/12] remove dsl format from doc - -Signed-off-by: Sean Kao ---- - docs/user/interfaces/protocol.rst | 68 ------------------------------- - 1 file changed, 68 deletions(-) - -diff --git a/docs/user/interfaces/protocol.rst b/docs/user/interfaces/protocol.rst -index 24e803ce9..625eed0f5 100644 ---- a/docs/user/interfaces/protocol.rst -+++ b/docs/user/interfaces/protocol.rst -@@ -208,74 +208,6 @@ Result set:: - "status" : 400 - } - --// TODO: deprecate json --OpenSearch DSL --============== -- --Description ------------- -- --The plugin returns original response from OpenSearch in JSON. Because this is the native response from OpenSearch, extra efforts are needed to parse and interpret it. -- --Example --------- -- --SQL query:: -- -- >> curl -H 'Content-Type: application/json' -X POST localhost:9200/_plugins/_sql?format=json -d '{ -- "query" : "SELECT firstname, lastname, age FROM accounts ORDER BY age LIMIT 2" -- }' -- --Result set:: -- -- { -- "_shards" : { -- "total" : 5, -- "failed" : 0, -- "successful" : 5, -- "skipped" : 0 -- }, -- "hits" : { -- "hits" : [ -- { -- "_index" : "accounts", -- "_type" : "_doc", -- "_source" : { -- "firstname" : "Nanette", -- "age" : 28, -- "lastname" : "Bates" -- }, -- "_id" : "13", -- "sort" : [ -- 28 -- ], -- "_score" : null -- }, -- { -- "_index" : "accounts", -- "_type" : "_doc", -- "_source" : { -- "firstname" : "Amber", -- "age" : 32, -- "lastname" : "Duke" -- }, -- "_id" : "1", -- "sort" : [ -- 32 -- ], -- "_score" : null -- } -- ], -- "total" : { -- "value" : 4, -- "relation" : "eq" -- }, -- "max_score" : null -- }, -- "took" : 100, -- "timed_out" : false -- } -- - CSV Format - ========== - --- -2.41.0 - - -From 43fe0fa9cc54538fb38e90382eb9c91f9a88219c Mon Sep 17 00:00:00 2001 -From: Sean Kao -Date: Thu, 27 Feb 2025 11:56:21 -0800 -Subject: [PATCH 03/12] remove Format.JSON - -Signed-off-by: Sean Kao ---- - .../org/opensearch/sql/legacy/ExplainIT.java | 1 - - .../sql/legacy/GetEndpointQueryIT.java | 2 -- - .../sql/legacy/OpenSearchSQLRestTestCase.java | 1 - - .../sql/legacy/SQLIntegTestCase.java | 12 ++---------- - .../ActionRequestRestExecutorFactory.java | 15 +-------------- - .../sql/legacy/executor/Format.java | 1 - - .../sql/legacy/plugin/RestSqlAction.java | 3 +-- - .../legacy/query/OpenSearchActionFactory.java | 13 +++---------- - .../legacy/unittest/SqlRequestParamTest.java | 7 ------- - .../planner/OpenSearchActionFactoryTest.java | 19 +++++-------------- - .../query/DefaultQueryActionTest.java | 3 +-- - 11 files changed, 13 insertions(+), 64 deletions(-) - -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java -index 2059c8462..1b740924c 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/ExplainIT.java -@@ -245,7 +245,6 @@ public class ExplainIT extends SQLIntegTestCase { - result.replaceAll("\\s+", ""), equalTo(expectedOutput.replaceAll("\\s+", ""))); - } - -- // TODO: is this test running without @Test? - public void testContentTypeOfExplainRequestShouldBeJson() throws IOException { - String query = makeRequest("SELECT firstname FROM opensearch-sql_test_index_account"); - Request request = getSqlRequest(query, true); -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java -index a74ddde65..6831dcfcc 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/GetEndpointQueryIT.java -@@ -13,8 +13,6 @@ import org.junit.Test; - import org.junit.rules.ExpectedException; - import org.opensearch.client.ResponseException; - --// TODO: inaccurate comment? Find out purpose of this test --/** Tests to cover requests with "?format=csv" parameter */ - public class GetEndpointQueryIT extends SQLIntegTestCase { - - @Rule public final ExpectedException rule = ExpectedException.none(); -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java -index 6b57310a9..ced69d54a 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java -@@ -181,7 +181,6 @@ public abstract class OpenSearchSQLRestTestCase extends OpenSearchRestTestCase { - } - } - -- // TODO: deprecate json - protected static void wipeAllOpenSearchIndices(RestClient client) throws IOException { - // include all the indices, included hidden indices. - // https://www.elastic.co/guide/en/elasticsearch/reference/current/cat-indices.html#cat-indices-api-query-params -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java -index 213401ec5..a9179a7b8 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java -@@ -228,10 +228,8 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { - loadIndex(index, client()); - } - -- // TODO: deprecate json -- // Note that explain queries are still supposed to return json - protected Request getSqlRequest(String request, boolean explain) { -- return getSqlRequest(request, explain, "json"); -+ return getSqlRequest(request, explain, "jdbc"); - } - - protected Request getSqlRequest(String request, boolean explain, String requestType) { -@@ -268,12 +266,10 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { - } - } - -- // DONE: verified all usages have requestType="jdbc" except in CsvFormatIT and RawFormatIT - protected String executeQuery(String query, String requestType) { - return executeQuery(query, requestType, Map.of()); - } - -- // DONE: verified usages don't contain requestType="json" - protected String executeQuery(String query, String requestType, Map params) { - try { - String endpoint = "/_plugins/_sql?format=" + requestType; -@@ -297,7 +293,6 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { - return new JSONObject(executeQuery(query, "jdbc")); - } - -- // DONE: verified usages don't contain requestType="json" - protected String executeFetchQuery(String query, int fetchSize, String requestType) - throws IOException { - String endpoint = "/_plugins/_sql?format=" + requestType; -@@ -322,7 +317,6 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { - return executeQueryTemplate(queryTemplate, index, 4); - } - -- // DONE: verified usages don't contain requestType="json" - protected String executeFetchLessQuery(String query, String requestType) throws IOException { - - String endpoint = "/_plugins/_sql?format=" + requestType; -@@ -348,10 +342,9 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { - Assert.fail(utf8CharsetName + " not available"); - } - -- // TODO: deprecate json - final String requestUrl = - String.format( -- Locale.ROOT, "%s?sql=%s&format=%s", QUERY_API_ENDPOINT, urlEncodedQuery, "json"); -+ Locale.ROOT, "%s?sql=%s&format=%s", QUERY_API_ENDPOINT, urlEncodedQuery, "jdbc"); - return new Request("GET", requestUrl); - } - -@@ -401,7 +394,6 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { - return executeRequest(request, client()); - } - -- // TODO: this returns json. Only used in GetEndpointQueryIT - protected JSONObject executeQueryWithGetRequest(final String sqlQuery) throws IOException { - - final Request request = buildGetEndpointRequest(sqlQuery); -diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java -index 8c6009248..a585c3c2c 100644 ---- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java -+++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java -@@ -18,17 +18,12 @@ public class ActionRequestRestExecutorFactory { - * call if necessary. - * - * @param format format of response -- * @param queryAction query action - * @return executor - */ -- public static RestExecutor createExecutor(Format format, QueryAction queryAction) { -+ public static RestExecutor createExecutor(Format format) { - switch (format) { - case CSV: - return new AsyncRestExecutor(new CSVResultRestExecutor()); -- case JSON: // TODO: deprecate json -- return new AsyncRestExecutor( -- new ElasticDefaultRestExecutor(queryAction), -- action -> isJoin(action) || isUnionMinus(action)); - case JDBC: - case RAW: - case TABLE: -@@ -36,12 +31,4 @@ public class ActionRequestRestExecutorFactory { - return new AsyncRestExecutor(new PrettyFormatRestExecutor(format.getFormatName())); - } - } -- -- private static boolean isJoin(QueryAction queryAction) { -- return queryAction instanceof OpenSearchJoinQueryAction; -- } -- -- private static boolean isUnionMinus(QueryAction queryAction) { -- return queryAction instanceof MultiQueryAction; -- } - } -diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java -index f17e2a1de..b30602754 100644 ---- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java -+++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/Format.java -@@ -14,7 +14,6 @@ import lombok.RequiredArgsConstructor; - @RequiredArgsConstructor - public enum Format { - JDBC("jdbc"), -- JSON("json"), // TODO: deprecate json - CSV("csv"), - RAW("raw"), - TABLE("table"); -diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java -index a0e68ff54..9be2367dc 100644 ---- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java -+++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java -@@ -251,8 +251,7 @@ public class RestSqlAction extends BaseRestHandler { - channel.sendResponse(new BytesRestResponse(OK, "application/json; charset=UTF-8", result)); - } else { - RestExecutor restExecutor = -- ActionRequestRestExecutorFactory.createExecutor( -- SqlRequestParam.getFormat(params), queryAction); -+ ActionRequestRestExecutorFactory.createExecutor(SqlRequestParam.getFormat(params)); - // doing this hack because OpenSearch throws exception for un-consumed props - Map additionalParams = new HashMap<>(); - for (String paramName : responseParams()) { -diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java -index 3924b6e41..93e721e52 100644 ---- a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java -+++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java -@@ -63,8 +63,7 @@ public class OpenSearchActionFactory { - - public static QueryAction create(Client client, String sql) - throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { -- // TODO: deprecate json -- return create(client, new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JSON)); -+ return create(client, new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JDBC)); - } - - /** -@@ -117,7 +116,7 @@ public class OpenSearchActionFactory { - } else { - sqlExpr.accept(new TermFieldRewriter()); - // migrate aggregation to query planner framework. -- if (shouldMigrateToQueryPlan(sqlExpr, request.getFormat())) { -+ if (shouldMigrateToQueryPlan(sqlExpr)) { - return new QueryPlanQueryAction( - new QueryPlanRequestBuilder( - new BindingTupleQueryPlanner(client, sqlExpr, request.getTypeProvider()))); -@@ -192,13 +191,7 @@ public class OpenSearchActionFactory { - } - - @VisibleForTesting -- public static boolean shouldMigrateToQueryPlan(SQLQueryExpr expr, Format format) { -- // TODO: deprecate json -- // The JSON format will return the OpenSearch aggregation result, which is not supported by the -- // QueryPlanner. -- if (format == Format.JSON) { -- return false; -- } -+ public static boolean shouldMigrateToQueryPlan(SQLQueryExpr expr) { - QueryPlannerScopeDecider decider = new QueryPlannerScopeDecider(); - return decider.isInScope(expr); - } -diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java -index b35ee4c99..ee2f3ac74 100644 ---- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java -+++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/SqlRequestParamTest.java -@@ -41,13 +41,6 @@ public class SqlRequestParamTest { - assertFalse(SqlRequestParam.isPrettyFormat(ImmutableMap.of(QUERY_PARAMS_PRETTY, "unknown"))); - } - -- // TODO: deprecate json -- @Test -- public void shouldReturnJSONIfFormatParamsIsJSON() { -- assertEquals( -- Format.JSON, SqlRequestParam.getFormat(ImmutableMap.of(QUERY_PARAMS_FORMAT, "json"))); -- } -- - @Test - public void shouldReturnDefaultFormatIfNoFormatParams() { - assertEquals(Format.JDBC, SqlRequestParam.getFormat(ImmutableMap.of())); -diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java -index 9b550f379..44ad735ac 100644 ---- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java -+++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java -@@ -14,21 +14,12 @@ import org.opensearch.sql.legacy.query.OpenSearchActionFactory; - import org.opensearch.sql.legacy.util.SqlParserUtils; - - public class OpenSearchActionFactoryTest { -- // TODO: deprecate json -- @Test -- public void josnOutputRequestShouldNotMigrateToQueryPlan() { -- String sql = "SELECT age, MAX(balance) FROM account GROUP BY age"; -- -- assertFalse( -- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql), Format.JSON)); -- } -- - @Test - public void nestQueryShouldNotMigrateToQueryPlan() { - String sql = "SELECT age, nested(balance) FROM account GROUP BY age"; - - assertFalse( -- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql), Format.JDBC)); -+ OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); - } - - @Test -@@ -36,7 +27,7 @@ public class OpenSearchActionFactoryTest { - String sql = "SELECT age FROM account "; - - assertFalse( -- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql), Format.JDBC)); -+ OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); - } - - @Test -@@ -44,7 +35,7 @@ public class OpenSearchActionFactoryTest { - String sql = "SELECT age, COUNT(balance) FROM account "; - - assertTrue( -- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql), Format.JDBC)); -+ OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); - } - - @Test -@@ -52,7 +43,7 @@ public class OpenSearchActionFactoryTest { - String sql = "SELECT age, MAX(balance) - MIN(balance) FROM account "; - - assertTrue( -- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql), Format.JDBC)); -+ OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); - } - - @Test -@@ -60,6 +51,6 @@ public class OpenSearchActionFactoryTest { - String sql = "SELECT CAST(age AS DOUBLE) as alias FROM account GROUP BY alias"; - - assertTrue( -- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql), Format.JDBC)); -+ OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); - } - } -diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java -index 5471e37dd..bf5d79855 100644 ---- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java -+++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java -@@ -130,8 +130,7 @@ public class DefaultQueryActionTest { - doReturn(settingFetchSize).when(mockSqlRequest).fetchSize(); - queryAction.setSqlRequest(mockSqlRequest); - -- // TODO: deprecate json -- Format[] formats = new Format[] {Format.CSV, Format.RAW, Format.JSON, Format.TABLE}; -+ Format[] formats = new Format[] {Format.CSV, Format.RAW, Format.TABLE}; - for (Format format : formats) { - queryAction.setFormat(format); - queryAction.checkAndSetScroll(); --- -2.41.0 - - -From e3157aa37ac977c14ca37aa9156d055475d6e32c Mon Sep 17 00:00:00 2001 -From: Sean Kao -Date: Fri, 28 Feb 2025 08:25:00 -0800 -Subject: [PATCH 04/12] fix test case wip - -Signed-off-by: Sean Kao ---- - .../org/opensearch/sql/legacy/QueryIT.java | 16 +++--- - .../sql/legacy/SQLIntegTestCase.java | 50 ++++++++++++++++--- - 2 files changed, 53 insertions(+), 13 deletions(-) - -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java -index 3fe8b50ee..66d79c2e9 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java -@@ -94,7 +94,6 @@ public class QueryIT extends SQLIntegTestCase { - executeQuery( - String.format( - Locale.ROOT, "SELECT * FROM %s LIMIT 1000", TestsConstants.TEST_INDEX_PHRASE)); -- Assert.assertTrue(response.has("hits")); - Assert.assertEquals(6, getTotalHits(response)); - } - -@@ -107,7 +106,6 @@ public class QueryIT extends SQLIntegTestCase { - "SELECT * FROM %s, %s LIMIT 2000", - TestsConstants.TEST_INDEX_BANK, - TestsConstants.TEST_INDEX_BANK_TWO)); -- Assert.assertTrue(response.has("hits")); - Assert.assertEquals(14, getTotalHits(response)); - } - -@@ -132,7 +130,7 @@ public class QueryIT extends SQLIntegTestCase { - @Test - public void selectAllWithMultipleFields() throws IOException { - JSONObject response = -- executeQuery( -+ executeQuery( // TODO: Multiple entries with same key: age=32 and age=32 - StringUtils.format( - "SELECT *, age, address FROM %s LIMIT 5", TestsConstants.TEST_INDEX_BANK)); - -@@ -152,7 +150,7 @@ public class QueryIT extends SQLIntegTestCase { - @Test - public void selectAllWithFieldAndGroupBy() throws IOException { - JSONObject response = -- executeQuery( -+ executeQuery( // TODO: Multiple entries with same key: age=28 and age=28 - StringUtils.format( - "SELECT *, age FROM %s GROUP BY age LIMIT 10", TestsConstants.TEST_INDEX_BANK)); - -@@ -650,11 +648,11 @@ public class QueryIT extends SQLIntegTestCase { - @Test - public void inTest() throws IOException { - JSONObject response = -- executeQuery( -+ executeQuery( // TODO: can't resolve Symbol(namespace=FIELD_NAME, name=age) in type env - String.format( - Locale.ROOT, - "SELECT age FROM %s WHERE age IN (20, 22) LIMIT 1000", -- TestsConstants.TEST_INDEX_PHRASE)); -+ TestsConstants.TEST_INDEX_PHRASE)); // change to PEOPLE? - - JSONArray hits = getHits(response); - for (int i = 0; i < hits.length(); i++) { -@@ -1186,6 +1184,7 @@ public class QueryIT extends SQLIntegTestCase { - checkResponseSize(response, BANK_INDEX_MALE_FALSE); - } - -+ // TODO: aggregation format is different. check in json format first - @Test - public void testWhereWithBoolIsFalse() throws IOException { - JSONObject response = -@@ -1370,6 +1369,7 @@ public class QueryIT extends SQLIntegTestCase { - Assert.assertEquals(1, getTotalHits(response)); - - JSONObject hit = hits.getJSONObject(0); -+ // TODO: JSONObject["parents"] not found. - Assert.assertEquals("Eddard", getSource(hit).getJSONObject("parents").getString("father")); - } - -@@ -1826,7 +1826,7 @@ public class QueryIT extends SQLIntegTestCase { - JSONArray hits = getHits(response); - for (int i = 0; i < hits.length(); i++) { - JSONObject hit = hits.getJSONObject(i); -- JSONObject highlightFields = hit.getJSONObject("highlight"); -+ JSONObject highlightFields = hit.getJSONObject("highlight"); // TODO: JSONObject["highlight"] not found. - - String phrase = highlightFields.getJSONArray("phrase").getString(0); - Assert.assertTrue(phrase.contains("fox")); -@@ -1963,6 +1963,8 @@ public class QueryIT extends SQLIntegTestCase { - JSONObject hit = getHits(response).getJSONObject(0); - String age = hit.query("/_source/age").toString(); - String cases = age.equals("30") ? "1" : age.equals("40") ? "2" : "0"; -+ // TODO: Cannot invoke "Object.toString()" because the return value of "org.json.JSONObject.query(String)" is null -+ // TODO: search for hit.query - - assertThat(cases, equalTo(hit.query("/fields/cases/0"))); - } -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java -index a9179a7b8..7472c3ecf 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java -@@ -493,17 +493,55 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { - return String.format("{\"cursor\":\"%s\"}", cursor); - } - -+ /** -+ * Following methods adapts the jdbc response format to maintain compatibility with existing code that -+ * expects legacy json (hits) structure. Instead of refactoring all dependent testing code to work with -+ * the jdbc (schema/datarows) format, this adapter transforms the jdbc format into the json structure. -+ * Jdbc format: -+ * { -+ * "schema": [{"name": "field1", "type": "text"}, ...], -+ * "datarows": [[value1, value2, ...], ...] -+ * } -+ * -+ * Transformed to legacy json format: -+ * { -+ * "hits": [{ -+ * "_source": {"field1": value1, ...} -+ * }, ...] -+ * } -+ */ - protected JSONArray getHits(JSONObject response) { -- Assert.assertTrue(response.getJSONObject("hits").has("hits")); -+ Assert.assertTrue(response.has("schema")); -+ Assert.assertTrue(response.has("datarows")); -+ -+ JSONArray schema = response.getJSONArray("schema"); -+ JSONArray datarows = response.getJSONArray("datarows"); -+ JSONArray hits = new JSONArray(); -+ -+ for (int i = 0; i < datarows.length(); i++) { -+ JSONObject hit = new JSONObject(); -+ JSONObject source = new JSONObject(); -+ JSONArray row = datarows.getJSONArray(i); -+ -+ for (int j = 0; j < schema.length(); j++) { -+ JSONObject schemaField = schema.getJSONObject(j); -+ String fieldName = schemaField.getString("name"); -+ Object value = row.get(j); -+ if (value != JSONObject.NULL) { // TODO: need this? -+ source.put(fieldName, value); -+ } -+ } -+ -+ hit.put("_source", source); -+ hits.put(hit); -+ } - -- return response.getJSONObject("hits").getJSONArray("hits"); -+ return hits; - } - - protected int getTotalHits(JSONObject response) { -- Assert.assertTrue(response.getJSONObject("hits").has("total")); -- Assert.assertTrue(response.getJSONObject("hits").getJSONObject("total").has("value")); -- -- return response.getJSONObject("hits").getJSONObject("total").getInt("value"); -+ Assert.assertTrue(response.has("total")); -+ return response.getInt("total"); - } - - protected JSONObject getSource(JSONObject hit) { --- -2.41.0 - - -From 85e2566d4c6ae114c7ec9b264ac0dae2d7738d9c Mon Sep 17 00:00:00 2001 -From: Sean Kao -Date: Fri, 28 Feb 2025 09:29:11 -0800 -Subject: [PATCH 05/12] Revert "fix test case wip" - -This reverts commit e3157aa37ac977c14ca37aa9156d055475d6e32c. - -Signed-off-by: Sean Kao ---- - .../org/opensearch/sql/legacy/QueryIT.java | 16 +++--- - .../sql/legacy/SQLIntegTestCase.java | 50 +++---------------- - 2 files changed, 13 insertions(+), 53 deletions(-) - -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java -index 66d79c2e9..3fe8b50ee 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java -@@ -94,6 +94,7 @@ public class QueryIT extends SQLIntegTestCase { - executeQuery( - String.format( - Locale.ROOT, "SELECT * FROM %s LIMIT 1000", TestsConstants.TEST_INDEX_PHRASE)); -+ Assert.assertTrue(response.has("hits")); - Assert.assertEquals(6, getTotalHits(response)); - } - -@@ -106,6 +107,7 @@ public class QueryIT extends SQLIntegTestCase { - "SELECT * FROM %s, %s LIMIT 2000", - TestsConstants.TEST_INDEX_BANK, - TestsConstants.TEST_INDEX_BANK_TWO)); -+ Assert.assertTrue(response.has("hits")); - Assert.assertEquals(14, getTotalHits(response)); - } - -@@ -130,7 +132,7 @@ public class QueryIT extends SQLIntegTestCase { - @Test - public void selectAllWithMultipleFields() throws IOException { - JSONObject response = -- executeQuery( // TODO: Multiple entries with same key: age=32 and age=32 -+ executeQuery( - StringUtils.format( - "SELECT *, age, address FROM %s LIMIT 5", TestsConstants.TEST_INDEX_BANK)); - -@@ -150,7 +152,7 @@ public class QueryIT extends SQLIntegTestCase { - @Test - public void selectAllWithFieldAndGroupBy() throws IOException { - JSONObject response = -- executeQuery( // TODO: Multiple entries with same key: age=28 and age=28 -+ executeQuery( - StringUtils.format( - "SELECT *, age FROM %s GROUP BY age LIMIT 10", TestsConstants.TEST_INDEX_BANK)); - -@@ -648,11 +650,11 @@ public class QueryIT extends SQLIntegTestCase { - @Test - public void inTest() throws IOException { - JSONObject response = -- executeQuery( // TODO: can't resolve Symbol(namespace=FIELD_NAME, name=age) in type env -+ executeQuery( - String.format( - Locale.ROOT, - "SELECT age FROM %s WHERE age IN (20, 22) LIMIT 1000", -- TestsConstants.TEST_INDEX_PHRASE)); // change to PEOPLE? -+ TestsConstants.TEST_INDEX_PHRASE)); - - JSONArray hits = getHits(response); - for (int i = 0; i < hits.length(); i++) { -@@ -1184,7 +1186,6 @@ public class QueryIT extends SQLIntegTestCase { - checkResponseSize(response, BANK_INDEX_MALE_FALSE); - } - -- // TODO: aggregation format is different. check in json format first - @Test - public void testWhereWithBoolIsFalse() throws IOException { - JSONObject response = -@@ -1369,7 +1370,6 @@ public class QueryIT extends SQLIntegTestCase { - Assert.assertEquals(1, getTotalHits(response)); - - JSONObject hit = hits.getJSONObject(0); -- // TODO: JSONObject["parents"] not found. - Assert.assertEquals("Eddard", getSource(hit).getJSONObject("parents").getString("father")); - } - -@@ -1826,7 +1826,7 @@ public class QueryIT extends SQLIntegTestCase { - JSONArray hits = getHits(response); - for (int i = 0; i < hits.length(); i++) { - JSONObject hit = hits.getJSONObject(i); -- JSONObject highlightFields = hit.getJSONObject("highlight"); // TODO: JSONObject["highlight"] not found. -+ JSONObject highlightFields = hit.getJSONObject("highlight"); - - String phrase = highlightFields.getJSONArray("phrase").getString(0); - Assert.assertTrue(phrase.contains("fox")); -@@ -1963,8 +1963,6 @@ public class QueryIT extends SQLIntegTestCase { - JSONObject hit = getHits(response).getJSONObject(0); - String age = hit.query("/_source/age").toString(); - String cases = age.equals("30") ? "1" : age.equals("40") ? "2" : "0"; -- // TODO: Cannot invoke "Object.toString()" because the return value of "org.json.JSONObject.query(String)" is null -- // TODO: search for hit.query - - assertThat(cases, equalTo(hit.query("/fields/cases/0"))); - } -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java -index 7472c3ecf..a9179a7b8 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java -@@ -493,55 +493,17 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { - return String.format("{\"cursor\":\"%s\"}", cursor); - } - -- /** -- * Following methods adapts the jdbc response format to maintain compatibility with existing code that -- * expects legacy json (hits) structure. Instead of refactoring all dependent testing code to work with -- * the jdbc (schema/datarows) format, this adapter transforms the jdbc format into the json structure. -- * Jdbc format: -- * { -- * "schema": [{"name": "field1", "type": "text"}, ...], -- * "datarows": [[value1, value2, ...], ...] -- * } -- * -- * Transformed to legacy json format: -- * { -- * "hits": [{ -- * "_source": {"field1": value1, ...} -- * }, ...] -- * } -- */ - protected JSONArray getHits(JSONObject response) { -- Assert.assertTrue(response.has("schema")); -- Assert.assertTrue(response.has("datarows")); -- -- JSONArray schema = response.getJSONArray("schema"); -- JSONArray datarows = response.getJSONArray("datarows"); -- JSONArray hits = new JSONArray(); -- -- for (int i = 0; i < datarows.length(); i++) { -- JSONObject hit = new JSONObject(); -- JSONObject source = new JSONObject(); -- JSONArray row = datarows.getJSONArray(i); -- -- for (int j = 0; j < schema.length(); j++) { -- JSONObject schemaField = schema.getJSONObject(j); -- String fieldName = schemaField.getString("name"); -- Object value = row.get(j); -- if (value != JSONObject.NULL) { // TODO: need this? -- source.put(fieldName, value); -- } -- } -- -- hit.put("_source", source); -- hits.put(hit); -- } -+ Assert.assertTrue(response.getJSONObject("hits").has("hits")); - -- return hits; -+ return response.getJSONObject("hits").getJSONArray("hits"); - } - - protected int getTotalHits(JSONObject response) { -- Assert.assertTrue(response.has("total")); -- return response.getInt("total"); -+ Assert.assertTrue(response.getJSONObject("hits").has("total")); -+ Assert.assertTrue(response.getJSONObject("hits").getJSONObject("total").has("value")); -+ -+ return response.getJSONObject("hits").getJSONObject("total").getInt("value"); - } - - protected JSONObject getSource(JSONObject hit) { --- -2.41.0 - - -From 5f0ac18fda02a46fe3bb81676e6c110b49164e22 Mon Sep 17 00:00:00 2001 -From: Sean Kao -Date: Fri, 28 Feb 2025 10:51:29 -0800 -Subject: [PATCH 06/12] ignore legacy test using json response format - -Signed-off-by: Sean Kao ---- - .../opensearch/sql/legacy/AggregationIT.java | 1 + - .../org/opensearch/sql/legacy/DateFormatIT.java | 1 + - .../org/opensearch/sql/legacy/HashJoinIT.java | 2 ++ - .../java/org/opensearch/sql/legacy/JoinIT.java | 1 + - .../org/opensearch/sql/legacy/MultiQueryIT.java | 2 ++ - .../java/org/opensearch/sql/legacy/OrderIT.java | 2 ++ - .../org/opensearch/sql/legacy/PluginIT.java | 2 ++ - .../java/org/opensearch/sql/legacy/QueryIT.java | 1 + - .../opensearch/sql/legacy/SQLIntegTestCase.java | 17 +++++++++++++++-- - 9 files changed, 27 insertions(+), 2 deletions(-) - -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/AggregationIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/AggregationIT.java -index 490e9eb51..c2b24c9bb 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/AggregationIT.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/AggregationIT.java -@@ -38,6 +38,7 @@ import org.junit.Assert; - import org.junit.Ignore; - import org.junit.Test; - -+@Ignore - public class AggregationIT extends SQLIntegTestCase { - - @Override -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/DateFormatIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/DateFormatIT.java -index 388d90092..af88c4e57 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/DateFormatIT.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/DateFormatIT.java -@@ -29,6 +29,7 @@ import org.junit.Ignore; - import org.junit.Test; - import org.opensearch.sql.legacy.exception.SqlParseException; - -+@Ignore - public class DateFormatIT extends SQLIntegTestCase { - - private static final String SELECT_FROM = -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/HashJoinIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/HashJoinIT.java -index 02c55d8eb..f27c18b24 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/HashJoinIT.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/HashJoinIT.java -@@ -19,8 +19,10 @@ import java.util.Set; - import org.json.JSONArray; - import org.json.JSONObject; - import org.junit.Assert; -+import org.junit.Ignore; - import org.junit.Test; - -+@Ignore - /** Test new hash join algorithm by comparison with old implementation. */ - public class HashJoinIT extends SQLIntegTestCase { - -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/JoinIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/JoinIT.java -index 8c2ea9647..af5434e99 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/JoinIT.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/JoinIT.java -@@ -31,6 +31,7 @@ import org.junit.Assert; - import org.junit.Ignore; - import org.junit.Test; - -+@Ignore - public class JoinIT extends SQLIntegTestCase { - - private static final String USE_NL_HINT = " /*! USE_NL*/"; -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/MultiQueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/MultiQueryIT.java -index bee85ac31..9fdb0f30d 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/MultiQueryIT.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/MultiQueryIT.java -@@ -14,8 +14,10 @@ import java.util.Set; - import org.json.JSONArray; - import org.json.JSONObject; - import org.junit.Assert; -+import org.junit.Ignore; - import org.junit.Test; - -+@Ignore - public class MultiQueryIT extends SQLIntegTestCase { - - private static final String MINUS_SCROLL_DEFAULT_HINT = -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/OrderIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/OrderIT.java -index 20bed5d2e..ce46de48e 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/OrderIT.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/OrderIT.java -@@ -10,8 +10,10 @@ import static org.hamcrest.Matchers.equalTo; - import java.io.IOException; - import org.json.JSONArray; - import org.json.JSONObject; -+import org.junit.Ignore; - import org.junit.Test; - -+@Ignore - public class OrderIT extends SQLIntegTestCase { - - @Override -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java -index 9305b1655..83fe0b94b 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java -@@ -12,6 +12,7 @@ import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_ACCOUNT; - import java.io.IOException; - import java.util.Locale; - import org.json.JSONObject; -+import org.junit.Ignore; - import org.junit.Test; - import org.opensearch.client.Request; - import org.opensearch.client.RequestOptions; -@@ -19,6 +20,7 @@ import org.opensearch.client.Response; - import org.opensearch.client.ResponseException; - import org.opensearch.sql.plugin.rest.RestQuerySettingsAction; - -+@Ignore - public class PluginIT extends SQLIntegTestCase { - - @Override -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java -index 3fe8b50ee..aa905c6c4 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/QueryIT.java -@@ -40,6 +40,7 @@ import org.opensearch.client.ResponseException; - import org.opensearch.core.rest.RestStatus; - import org.opensearch.sql.legacy.utils.StringUtils; - -+@Ignore - public class QueryIT extends SQLIntegTestCase { - - /** -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java -index a9179a7b8..c324b1090 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java -@@ -228,8 +228,13 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { - loadIndex(index, client()); - } - -+ /** -+ * TODO: Decide what to do with legacy tests using json response format. -+ * OpenSearch DSL format is deprecated. Need to ensure that requests in legacy tests using the json response format -+ * are not invoked. -+ */ - protected Request getSqlRequest(String request, boolean explain) { -- return getSqlRequest(request, explain, "jdbc"); -+ return getSqlRequest(request, explain, "json"); - } - - protected Request getSqlRequest(String request, boolean explain, String requestType) { -@@ -330,6 +335,11 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { - return responseString; - } - -+ /** -+ * TODO: Decide what to do with legacy tests using json response format. -+ * OpenSearch DSL format is deprecated. Need to ensure that requests in legacy tests using the json response format -+ * are not invoked. -+ */ - protected Request buildGetEndpointRequest(final String sqlQuery) { - - final String utf8CharsetName = StandardCharsets.UTF_8.name(); -@@ -344,7 +354,7 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { - - final String requestUrl = - String.format( -- Locale.ROOT, "%s?sql=%s&format=%s", QUERY_API_ENDPOINT, urlEncodedQuery, "jdbc"); -+ Locale.ROOT, "%s?sql=%s&format=%s", QUERY_API_ENDPOINT, urlEncodedQuery, "json"); - return new Request("GET", requestUrl); - } - -@@ -385,6 +395,9 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { - - protected static String executeRequest(final Request request, RestClient client) - throws IOException { -+ if (request.getParameters().get("format") == "json") { -+ throw new IOException("request type is json"); -+ } - Response response = client.performRequest(request); - Assert.assertEquals(200, response.getStatusLine().getStatusCode()); - return getResponseBody(response); --- -2.41.0 - - -From 3cbaf825356abba4b4de34d8b96bd13d73f0c124 Mon Sep 17 00:00:00 2001 -From: Sean Kao -Date: Fri, 28 Feb 2025 11:00:18 -0800 -Subject: [PATCH 07/12] spotlessApply - -Signed-off-by: Sean Kao ---- - .tool-versions | 1 + - .../opensearch/sql/legacy/SQLIntegTestCase.java | 12 ++++++------ - .../ActionRequestRestExecutorFactory.java | 3 --- - .../planner/OpenSearchActionFactoryTest.java | 16 +++++----------- - 4 files changed, 12 insertions(+), 20 deletions(-) - create mode 100644 .tool-versions - -diff --git a/.tool-versions b/.tool-versions -new file mode 100644 -index 000000000..e95b9595e ---- /dev/null -+++ b/.tool-versions -@@ -0,0 +1 @@ -+java adoptopenjdk-21.0.5+11.0.LTS -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java -index c324b1090..852675c8c 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java -@@ -229,9 +229,9 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { - } - - /** -- * TODO: Decide what to do with legacy tests using json response format. -- * OpenSearch DSL format is deprecated. Need to ensure that requests in legacy tests using the json response format -- * are not invoked. -+ * TODO: Decide what to do with legacy tests using json response format. OpenSearch DSL format is -+ * deprecated. Need to ensure that requests in legacy tests using the json response format are not -+ * invoked. - */ - protected Request getSqlRequest(String request, boolean explain) { - return getSqlRequest(request, explain, "json"); -@@ -336,9 +336,9 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { - } - - /** -- * TODO: Decide what to do with legacy tests using json response format. -- * OpenSearch DSL format is deprecated. Need to ensure that requests in legacy tests using the json response format -- * are not invoked. -+ * TODO: Decide what to do with legacy tests using json response format. OpenSearch DSL format is -+ * deprecated. Need to ensure that requests in legacy tests using the json response format are not -+ * invoked. - */ - protected Request buildGetEndpointRequest(final String sqlQuery) { - -diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java -index a585c3c2c..068c1b433 100644 ---- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java -+++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/ActionRequestRestExecutorFactory.java -@@ -7,9 +7,6 @@ package org.opensearch.sql.legacy.executor; - - import org.opensearch.sql.legacy.executor.csv.CSVResultRestExecutor; - import org.opensearch.sql.legacy.executor.format.PrettyFormatRestExecutor; --import org.opensearch.sql.legacy.query.QueryAction; --import org.opensearch.sql.legacy.query.join.OpenSearchJoinQueryAction; --import org.opensearch.sql.legacy.query.multi.MultiQueryAction; - - /** Created by Eliran on 26/12/2015. */ - public class ActionRequestRestExecutorFactory { -diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java -index 44ad735ac..008b0d618 100644 ---- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java -+++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/OpenSearchActionFactoryTest.java -@@ -9,7 +9,6 @@ import static org.junit.Assert.assertFalse; - import static org.junit.Assert.assertTrue; - - import org.junit.Test; --import org.opensearch.sql.legacy.executor.Format; - import org.opensearch.sql.legacy.query.OpenSearchActionFactory; - import org.opensearch.sql.legacy.util.SqlParserUtils; - -@@ -18,39 +17,34 @@ public class OpenSearchActionFactoryTest { - public void nestQueryShouldNotMigrateToQueryPlan() { - String sql = "SELECT age, nested(balance) FROM account GROUP BY age"; - -- assertFalse( -- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); -+ assertFalse(OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); - } - - @Test - public void nonAggregationQueryShouldNotMigrateToQueryPlan() { - String sql = "SELECT age FROM account "; - -- assertFalse( -- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); -+ assertFalse(OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); - } - - @Test - public void aggregationQueryWithoutGroupByShouldMigrateToQueryPlan() { - String sql = "SELECT age, COUNT(balance) FROM account "; - -- assertTrue( -- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); -+ assertTrue(OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); - } - - @Test - public void aggregationQueryWithExpressionByShouldMigrateToQueryPlan() { - String sql = "SELECT age, MAX(balance) - MIN(balance) FROM account "; - -- assertTrue( -- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); -+ assertTrue(OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); - } - - @Test - public void queryOnlyHasGroupByShouldMigrateToQueryPlan() { - String sql = "SELECT CAST(age AS DOUBLE) as alias FROM account GROUP BY alias"; - -- assertTrue( -- OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); -+ assertTrue(OpenSearchActionFactory.shouldMigrateToQueryPlan(SqlParserUtils.parse(sql))); - } - } --- -2.41.0 - - -From c74b96b8a7e9f2e35a012eaf80f93c0a249b430d Mon Sep 17 00:00:00 2001 -From: Sean Kao -Date: Fri, 28 Feb 2025 11:58:15 -0800 -Subject: [PATCH 08/12] remove json param check in executeRequest - -Signed-off-by: Sean Kao ---- - .../test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java | 3 --- - 1 file changed, 3 deletions(-) - -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java -index 852675c8c..dc96e30cb 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/SQLIntegTestCase.java -@@ -395,9 +395,6 @@ public abstract class SQLIntegTestCase extends OpenSearchSQLRestTestCase { - - protected static String executeRequest(final Request request, RestClient client) - throws IOException { -- if (request.getParameters().get("format") == "json") { -- throw new IOException("request type is json"); -- } - Response response = client.performRequest(request); - Assert.assertEquals(200, response.getStatusLine().getStatusCode()); - return getResponseBody(response); --- -2.41.0 - - -From 48d512aa3632998409359ee4465a6c621bf3bbd8 Mon Sep 17 00:00:00 2001 -From: Sean Kao -Date: Fri, 28 Feb 2025 15:43:19 -0800 -Subject: [PATCH 09/12] fix getScriptFieldFromQuery can't get requestBuilder - from QueryPlan - -Signed-off-by: Sean Kao ---- - .../org/opensearch/sql/legacy/plugin/SearchDao.java | 2 +- - .../sql/legacy/query/OpenSearchActionFactory.java | 12 +++++++++--- - .../sql/legacy/unittest/JSONRequestTest.java | 2 +- - .../sql/legacy/util/CheckScriptContents.java | 3 ++- - 4 files changed, 13 insertions(+), 6 deletions(-) - -diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/SearchDao.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/SearchDao.java -index 415b98f6b..10ca9ddae 100644 ---- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/SearchDao.java -+++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/SearchDao.java -@@ -45,6 +45,6 @@ public class SearchDao { - */ - public QueryAction explain(QueryActionRequest queryActionRequest) - throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { -- return OpenSearchActionFactory.create(client, queryActionRequest); -+ return OpenSearchActionFactory.create(client, queryActionRequest, false); - } - } -diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java -index 93e721e52..2f6ea1379 100644 ---- a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java -+++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java -@@ -63,16 +63,22 @@ public class OpenSearchActionFactory { - - public static QueryAction create(Client client, String sql) - throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { -- return create(client, new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JDBC)); -+ return create(client, sql, false); -+ } -+ -+ public static QueryAction create(Client client, String sql, boolean bypassMigrateToQueryPlan) -+ throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { -+ return create(client, new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JDBC), bypassMigrateToQueryPlan); - } - - /** - * Create the compatible Query object based on the SQL query. - * - * @param request The SQL query. -+ * @param bypassMigrateToQueryPlan Avoid using QueryPlan. - * @return Query object. - */ -- public static QueryAction create(Client client, QueryActionRequest request) -+ public static QueryAction create(Client client, QueryActionRequest request, boolean bypassMigrateToQueryPlan) - throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { - String sql = request.getSql(); - // Remove line breaker anywhere and semicolon at the end -@@ -116,7 +122,7 @@ public class OpenSearchActionFactory { - } else { - sqlExpr.accept(new TermFieldRewriter()); - // migrate aggregation to query planner framework. -- if (shouldMigrateToQueryPlan(sqlExpr)) { -+ if (!bypassMigrateToQueryPlan && shouldMigrateToQueryPlan(sqlExpr)) { - return new QueryPlanQueryAction( - new QueryPlanRequestBuilder( - new BindingTupleQueryPlanner(client, sqlExpr, request.getTypeProvider()))); -diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/JSONRequestTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/JSONRequestTest.java -index 5cf9c602a..c0ea8581e 100644 ---- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/JSONRequestTest.java -+++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/JSONRequestTest.java -@@ -435,7 +435,7 @@ public class JSONRequestTest { - CheckScriptContents.stubMockClient(mockClient); - QueryAction queryAction = - OpenSearchActionFactory.create( -- mockClient, new QueryActionRequest(sql, columnTypeProvider, Format.JDBC)); -+ mockClient, new QueryActionRequest(sql, columnTypeProvider, Format.JDBC), false); - - SqlRequest sqlRequest = new SqlRequest(sql, jsonRequest); - queryAction.setSqlRequest(sqlRequest); -diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/util/CheckScriptContents.java b/legacy/src/test/java/org/opensearch/sql/legacy/util/CheckScriptContents.java -index 2fe711c3b..c15bbc39f 100644 ---- a/legacy/src/test/java/org/opensearch/sql/legacy/util/CheckScriptContents.java -+++ b/legacy/src/test/java/org/opensearch/sql/legacy/util/CheckScriptContents.java -@@ -63,7 +63,8 @@ public class CheckScriptContents { - try { - Client mockClient = mock(Client.class); - stubMockClient(mockClient); -- QueryAction queryAction = OpenSearchActionFactory.create(mockClient, query); -+ // Avoid creating QueryPlanQueryAction so that scriptFields is available -+ QueryAction queryAction = OpenSearchActionFactory.create(mockClient, query, true); - SqlElasticRequestBuilder requestBuilder = queryAction.explain(); - - SearchRequestBuilder request = (SearchRequestBuilder) requestBuilder.getBuilder(); --- -2.41.0 - - -From d0c1ef1b070c3c14817d2fef07bf9b2a2cef7855 Mon Sep 17 00:00:00 2001 -From: Sean Kao -Date: Fri, 28 Feb 2025 15:52:57 -0800 -Subject: [PATCH 10/12] spotlessApply - -Signed-off-by: Sean Kao ---- - .../sql/legacy/query/OpenSearchActionFactory.java | 8 ++++++-- - 1 file changed, 6 insertions(+), 2 deletions(-) - -diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java -index 2f6ea1379..ffa5b24fd 100644 ---- a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java -+++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java -@@ -68,7 +68,10 @@ public class OpenSearchActionFactory { - - public static QueryAction create(Client client, String sql, boolean bypassMigrateToQueryPlan) - throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { -- return create(client, new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JDBC), bypassMigrateToQueryPlan); -+ return create( -+ client, -+ new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JDBC), -+ bypassMigrateToQueryPlan); - } - - /** -@@ -78,7 +81,8 @@ public class OpenSearchActionFactory { - * @param bypassMigrateToQueryPlan Avoid using QueryPlan. - * @return Query object. - */ -- public static QueryAction create(Client client, QueryActionRequest request, boolean bypassMigrateToQueryPlan) -+ public static QueryAction create( -+ Client client, QueryActionRequest request, boolean bypassMigrateToQueryPlan) - throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { - String sql = request.getSql(); - // Remove line breaker anywhere and semicolon at the end --- -2.41.0 - - -From 82cfce09b145f8f3abad284bb61a5f7be058073c Mon Sep 17 00:00:00 2001 -From: Sean Kao -Date: Fri, 28 Feb 2025 16:45:42 -0800 -Subject: [PATCH 11/12] ignore test using json - -Signed-off-by: Sean Kao ---- - .../test/java/org/opensearch/sql/legacy/DateFunctionsIT.java | 2 ++ - 1 file changed, 2 insertions(+) - -diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/DateFunctionsIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/DateFunctionsIT.java -index d9a6849fc..81206cf76 100644 ---- a/integ-test/src/test/java/org/opensearch/sql/legacy/DateFunctionsIT.java -+++ b/integ-test/src/test/java/org/opensearch/sql/legacy/DateFunctionsIT.java -@@ -17,6 +17,7 @@ import org.joda.time.DateTime; - import org.joda.time.format.DateTimeFormat; - import org.joda.time.format.DateTimeFormatter; - import org.json.JSONObject; -+import org.junit.Ignore; - import org.junit.Test; - import org.opensearch.action.search.SearchResponse; - import org.opensearch.common.xcontent.LoggingDeprecationHandler; -@@ -25,6 +26,7 @@ import org.opensearch.core.xcontent.NamedXContentRegistry; - import org.opensearch.core.xcontent.XContentParser; - import org.opensearch.search.SearchHit; - -+@Ignore - public class DateFunctionsIT extends SQLIntegTestCase { - - private static final String FROM = "FROM " + TestsConstants.TEST_INDEX_ONLINE; --- -2.41.0 - - -From 5f38dd52f8c870a408a9ea9f6323df4e1bc90c22 Mon Sep 17 00:00:00 2001 -From: Sean Kao -Date: Fri, 28 Feb 2025 16:53:47 -0800 -Subject: [PATCH 12/12] fix test case - -Signed-off-by: Sean Kao ---- - .../sql/legacy/unittest/query/DefaultQueryActionTest.java | 2 +- - 1 file changed, 1 insertion(+), 1 deletion(-) - -diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java -index bf5d79855..4c6061ff5 100644 ---- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java -+++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/query/DefaultQueryActionTest.java -@@ -136,7 +136,7 @@ public class DefaultQueryActionTest { - queryAction.checkAndSetScroll(); - } - -- Mockito.verify(mockRequestBuilder, times(4)).setSize(limit); -+ Mockito.verify(mockRequestBuilder, times(3)).setSize(limit); - Mockito.verify(mockRequestBuilder, never()).setScroll(any(TimeValue.class)); - - queryAction.setFormat(Format.JDBC); --- -2.41.0 - From 681554f217b6703cb2e7cdf69bb94e1540073755 Mon Sep 17 00:00:00 2001 From: Sean Kao Date: Mon, 3 Mar 2025 10:22:34 -0800 Subject: [PATCH 16/21] comment Signed-off-by: Sean Kao --- .../opensearch/sql/legacy/query/OpenSearchActionFactory.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java index ffa5b24fd31..52a6d25b294 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java @@ -78,7 +78,7 @@ public static QueryAction create(Client client, String sql, boolean bypassMigrat * Create the compatible Query object based on the SQL query. * * @param request The SQL query. - * @param bypassMigrateToQueryPlan Avoid using QueryPlan. + * @param bypassMigrateToQueryPlan Avoid using QueryPlan for testing. * @return Query object. */ public static QueryAction create( From e449c444d81fbd1227f04589a39aba860c0f6084 Mon Sep 17 00:00:00 2001 From: Sean Kao Date: Mon, 3 Mar 2025 10:54:20 -0800 Subject: [PATCH 17/21] remove unneeded file Signed-off-by: Sean Kao --- .tool-versions | 1 - 1 file changed, 1 deletion(-) delete mode 100644 .tool-versions diff --git a/.tool-versions b/.tool-versions deleted file mode 100644 index e95b9595eaf..00000000000 --- a/.tool-versions +++ /dev/null @@ -1 +0,0 @@ -java adoptopenjdk-21.0.5+11.0.LTS From b10d1486c9c581dc9b13121083a9c32574003968 Mon Sep 17 00:00:00 2001 From: Sean Kao Date: Mon, 3 Mar 2025 12:15:33 -0800 Subject: [PATCH 18/21] delete unused class Signed-off-by: Sean Kao --- .../executor/ElasticDefaultRestExecutor.java | 127 ------------------ 1 file changed, 127 deletions(-) delete mode 100644 legacy/src/main/java/org/opensearch/sql/legacy/executor/ElasticDefaultRestExecutor.java diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ElasticDefaultRestExecutor.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/ElasticDefaultRestExecutor.java deleted file mode 100644 index 5b18defbdc0..00000000000 --- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/ElasticDefaultRestExecutor.java +++ /dev/null @@ -1,127 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.legacy.executor; - -import com.google.common.collect.Maps; -import java.io.IOException; -import java.util.Map; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; -import org.opensearch.action.ActionRequest; -import org.opensearch.action.admin.indices.get.GetIndexRequest; -import org.opensearch.action.search.SearchRequest; -import org.opensearch.action.search.SearchResponse; -import org.opensearch.action.search.SearchScrollRequest; -import org.opensearch.common.action.ActionFuture; -import org.opensearch.core.rest.RestStatus; -import org.opensearch.index.reindex.BulkIndexByScrollResponseContentListener; -import org.opensearch.index.reindex.DeleteByQueryRequest; -import org.opensearch.rest.BytesRestResponse; -import org.opensearch.rest.RestChannel; -import org.opensearch.rest.action.RestStatusToXContentListener; -import org.opensearch.search.SearchHits; -import org.opensearch.sql.legacy.exception.SqlParseException; -import org.opensearch.sql.legacy.executor.join.ElasticJoinExecutor; -import org.opensearch.sql.legacy.executor.join.ElasticUtils; -import org.opensearch.sql.legacy.executor.join.MetaSearchResult; -import org.opensearch.sql.legacy.executor.multi.MultiRequestExecutorFactory; -import org.opensearch.sql.legacy.query.QueryAction; -import org.opensearch.sql.legacy.query.SqlElasticRequestBuilder; -import org.opensearch.sql.legacy.query.join.JoinRequestBuilder; -import org.opensearch.sql.legacy.query.multi.MultiQueryRequestBuilder; -import org.opensearch.transport.client.Client; - -public class ElasticDefaultRestExecutor implements RestExecutor { - - /** Request builder to generate OpenSearch DSL */ - private final SqlElasticRequestBuilder requestBuilder; - - private static final Logger LOG = LogManager.getLogger(ElasticDefaultRestExecutor.class); - - public ElasticDefaultRestExecutor(QueryAction queryAction) { - // Put explain() here to make it run in NIO thread - try { - this.requestBuilder = queryAction.explain(); - } catch (SqlParseException e) { - throw new IllegalStateException("Failed to explain query action", e); - } - } - - /** Execute the ActionRequest and returns the REST response using the channel. */ - @Override - public void execute( - Client client, Map params, QueryAction queryAction, RestChannel channel) - throws Exception { - ActionRequest request = requestBuilder.request(); - - if (requestBuilder instanceof JoinRequestBuilder) { - ElasticJoinExecutor executor = ElasticJoinExecutor.createJoinExecutor(client, requestBuilder); - executor.run(); - executor.sendResponse(channel); - } else if (requestBuilder instanceof MultiQueryRequestBuilder) { - ElasticHitsExecutor executor = - MultiRequestExecutorFactory.createExecutor( - client, (MultiQueryRequestBuilder) requestBuilder); - executor.run(); - sendDefaultResponse(executor.getHits(), channel); - } else if (request instanceof SearchRequest) { - client.search((SearchRequest) request, new RestStatusToXContentListener<>(channel)); - } else if (request instanceof DeleteByQueryRequest) { - requestBuilder - .getBuilder() - .execute(new BulkIndexByScrollResponseContentListener(channel, Maps.newHashMap())); - } else if (request instanceof GetIndexRequest) { - requestBuilder - .getBuilder() - .execute(new GetIndexRequestRestListener(channel, (GetIndexRequest) request)); - } else if (request instanceof SearchScrollRequest) { - client.searchScroll( - (SearchScrollRequest) request, new RestStatusToXContentListener<>(channel)); - } else { - throw new Exception( - String.format("Unsupported ActionRequest provided: %s", request.getClass().getName())); - } - } - - @Override - public String execute(Client client, Map params, QueryAction queryAction) - throws Exception { - ActionRequest request = requestBuilder.request(); - - if (requestBuilder instanceof JoinRequestBuilder) { - ElasticJoinExecutor executor = ElasticJoinExecutor.createJoinExecutor(client, requestBuilder); - executor.run(); - return ElasticUtils.hitsAsStringResult(executor.getHits(), new MetaSearchResult()); - } else if (requestBuilder instanceof MultiQueryRequestBuilder) { - ElasticHitsExecutor executor = - MultiRequestExecutorFactory.createExecutor( - client, (MultiQueryRequestBuilder) requestBuilder); - executor.run(); - return ElasticUtils.hitsAsStringResult(executor.getHits(), new MetaSearchResult()); - } else if (request instanceof SearchRequest) { - ActionFuture future = client.search((SearchRequest) request); - SearchResponse response = future.actionGet(); - return response.toString(); - } else if (request instanceof DeleteByQueryRequest) { - return requestBuilder.get().toString(); - } else if (request instanceof GetIndexRequest) { - return requestBuilder.getBuilder().execute().actionGet().toString(); - } else { - throw new Exception( - String.format("Unsupported ActionRequest provided: %s", request.getClass().getName())); - } - } - - private void sendDefaultResponse(SearchHits hits, RestChannel channel) { - try { - String json = ElasticUtils.hitsAsStringResult(hits, new MetaSearchResult()); - BytesRestResponse bytesRestResponse = new BytesRestResponse(RestStatus.OK, json); - channel.sendResponse(bytesRestResponse); - } catch (IOException e) { - e.printStackTrace(); - } - } -} From d43a5c0aa181dc63648ae4ef47155412c3850fe4 Mon Sep 17 00:00:00 2001 From: Sean Kao Date: Tue, 4 Mar 2025 11:45:15 -0800 Subject: [PATCH 19/21] revert change in prod code for test purpose; fix test Signed-off-by: Sean Kao --- .../sql/legacy/plugin/SearchDao.java | 2 +- .../legacy/query/OpenSearchActionFactory.java | 16 +++----------- .../sql/legacy/unittest/JSONRequestTest.java | 2 +- .../legacy/unittest/StringOperatorsTest.java | 21 +++++++------------ .../sql/legacy/util/CheckScriptContents.java | 3 +-- 5 files changed, 13 insertions(+), 31 deletions(-) diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/SearchDao.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/SearchDao.java index 10ca9ddae2e..415b98f6b89 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/SearchDao.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/SearchDao.java @@ -45,6 +45,6 @@ public Client getClient() { */ public QueryAction explain(QueryActionRequest queryActionRequest) throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { - return OpenSearchActionFactory.create(client, queryActionRequest, false); + return OpenSearchActionFactory.create(client, queryActionRequest); } } diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java index 52a6d25b294..93e721e52d4 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java @@ -63,26 +63,16 @@ public class OpenSearchActionFactory { public static QueryAction create(Client client, String sql) throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { - return create(client, sql, false); - } - - public static QueryAction create(Client client, String sql, boolean bypassMigrateToQueryPlan) - throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { - return create( - client, - new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JDBC), - bypassMigrateToQueryPlan); + return create(client, new QueryActionRequest(sql, new ColumnTypeProvider(), Format.JDBC)); } /** * Create the compatible Query object based on the SQL query. * * @param request The SQL query. - * @param bypassMigrateToQueryPlan Avoid using QueryPlan for testing. * @return Query object. */ - public static QueryAction create( - Client client, QueryActionRequest request, boolean bypassMigrateToQueryPlan) + public static QueryAction create(Client client, QueryActionRequest request) throws SqlParseException, SQLFeatureNotSupportedException, SQLFeatureDisabledException { String sql = request.getSql(); // Remove line breaker anywhere and semicolon at the end @@ -126,7 +116,7 @@ public static QueryAction create( } else { sqlExpr.accept(new TermFieldRewriter()); // migrate aggregation to query planner framework. - if (!bypassMigrateToQueryPlan && shouldMigrateToQueryPlan(sqlExpr)) { + if (shouldMigrateToQueryPlan(sqlExpr)) { return new QueryPlanQueryAction( new QueryPlanRequestBuilder( new BindingTupleQueryPlanner(client, sqlExpr, request.getTypeProvider()))); diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/JSONRequestTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/JSONRequestTest.java index c0ea8581e13..5cf9c602a26 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/JSONRequestTest.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/JSONRequestTest.java @@ -435,7 +435,7 @@ private String translate(String sql, JSONObject jsonRequest) CheckScriptContents.stubMockClient(mockClient); QueryAction queryAction = OpenSearchActionFactory.create( - mockClient, new QueryActionRequest(sql, columnTypeProvider, Format.JDBC), false); + mockClient, new QueryActionRequest(sql, columnTypeProvider, Format.JDBC)); SqlRequest sqlRequest = new SqlRequest(sql, jsonRequest); queryAction.setSqlRequest(sqlRequest); diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/StringOperatorsTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/StringOperatorsTest.java index 27b8e7f2c6e..8362b711926 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/StringOperatorsTest.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/StringOperatorsTest.java @@ -26,8 +26,7 @@ public static void init() { @Test public void substringTest() { String query = - "SELECT substring(lastname, 2, 1) FROM accounts WHERE substring(lastname, 2, 1) = 'a' " - + "GROUP BY substring(lastname, 2, 1) ORDER BY substring(lastname, 2, 1)"; + "SELECT substring(lastname, 2, 1) FROM accounts WHERE substring(lastname, 2, 1) = 'a'"; ScriptField scriptField = CheckScriptContents.getScriptFieldFromQuery(query); assertTrue( @@ -52,8 +51,7 @@ public void substringIndexOutOfBoundTest() { @Test public void lengthTest() { String query = - "SELECT length(lastname) FROM accounts WHERE length(lastname) = 5 " - + "GROUP BY length(lastname) ORDER BY length(lastname)"; + "SELECT length(lastname) FROM accounts WHERE length(lastname) = 5"; ScriptField scriptField = CheckScriptContents.getScriptFieldFromQuery(query); assertTrue( @@ -67,8 +65,7 @@ public void lengthTest() { @Test public void replaceTest() { String query = - "SELECT replace(lastname, 'a', 'A') FROM accounts WHERE replace(lastname, 'a', 'A') = 'aba'" - + " GROUP BY replace(lastname, 'a', 'A') ORDER BY replace(lastname, 'a', 'A')"; + "SELECT replace(lastname, 'a', 'A') FROM accounts WHERE replace(lastname, 'a', 'A') = 'aba'"; ScriptField scriptField = CheckScriptContents.getScriptFieldFromQuery(query); assertTrue( @@ -84,8 +81,7 @@ public void replaceTest() { @Test public void locateTest() { String query = - "SELECT locate('a', lastname, 1) FROM accounts WHERE locate('a', lastname, 1) = 4 " - + "GROUP BY locate('a', lastname, 1) ORDER BY locate('a', lastname, 1)"; + "SELECT locate('a', lastname, 1) FROM accounts WHERE locate('a', lastname, 1) = 4"; ScriptField scriptField = CheckScriptContents.getScriptFieldFromQuery(query); assertTrue( @@ -101,8 +97,7 @@ public void locateTest() { @Test public void ltrimTest() { String query = - "SELECT ltrim(lastname) FROM accounts WHERE ltrim(lastname) = 'abc' " - + "GROUP BY ltrim(lastname) ORDER BY ltrim(lastname)"; + "SELECT ltrim(lastname) FROM accounts WHERE ltrim(lastname) = 'abc'"; ScriptField scriptField = CheckScriptContents.getScriptFieldFromQuery(query); assertTrue( @@ -118,8 +113,7 @@ public void ltrimTest() { @Test public void rtrimTest() { String query = - "SELECT rtrim(lastname) FROM accounts WHERE rtrim(lastname) = 'cba' " - + "GROUP BY rtrim(lastname) ORDER BY rtrim(lastname)"; + "SELECT rtrim(lastname) FROM accounts WHERE rtrim(lastname) = 'cba'"; ScriptField scriptField = CheckScriptContents.getScriptFieldFromQuery(query); assertTrue( @@ -135,8 +129,7 @@ public void rtrimTest() { @Test public void asciiTest() { String query = - "SELECT ascii(lastname) FROM accounts WHERE ascii(lastname) = 108 " - + "GROUP BY ascii(lastname) ORDER BY ascii(lastname)"; + "SELECT ascii(lastname) FROM accounts WHERE ascii(lastname) = 108"; ScriptField scriptField = CheckScriptContents.getScriptFieldFromQuery(query); assertTrue( diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/util/CheckScriptContents.java b/legacy/src/test/java/org/opensearch/sql/legacy/util/CheckScriptContents.java index c15bbc39f1b..2fe711c3b06 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/util/CheckScriptContents.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/util/CheckScriptContents.java @@ -63,8 +63,7 @@ public static ScriptField getScriptFieldFromQuery(String query) { try { Client mockClient = mock(Client.class); stubMockClient(mockClient); - // Avoid creating QueryPlanQueryAction so that scriptFields is available - QueryAction queryAction = OpenSearchActionFactory.create(mockClient, query, true); + QueryAction queryAction = OpenSearchActionFactory.create(mockClient, query); SqlElasticRequestBuilder requestBuilder = queryAction.explain(); SearchRequestBuilder request = (SearchRequestBuilder) requestBuilder.getBuilder(); From 06bea4fbf9e634dcedfa6e7dedd62637cb0dc2c4 Mon Sep 17 00:00:00 2001 From: Sean Kao Date: Tue, 4 Mar 2025 12:22:08 -0800 Subject: [PATCH 20/21] re-evaluate affected tests Signed-off-by: Sean Kao --- .../java/org/opensearch/sql/legacy/MalformedQueryIT.java | 4 ---- .../src/test/java/org/opensearch/sql/legacy/PluginIT.java | 6 +++--- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/MalformedQueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/MalformedQueryIT.java index 4c2f3c56d7b..84b60fdabd7 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/MalformedQueryIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/MalformedQueryIT.java @@ -11,13 +11,9 @@ import org.apache.hc.core5.http.io.entity.EntityUtils; import org.json.JSONObject; import org.junit.Assert; -import org.junit.Ignore; import org.opensearch.client.ResponseException; /** Tests for clean handling of various types of invalid queries */ -@Ignore( - "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" - + " response for now. Need to decide what to do with these test cases.") public class MalformedQueryIT extends SQLIntegTestCase { @Override protected void init() throws Exception { diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java index 909ffef2c30..49cc61dc054 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/PluginIT.java @@ -20,9 +20,6 @@ import org.opensearch.client.ResponseException; import org.opensearch.sql.plugin.rest.RestQuerySettingsAction; -@Ignore( - "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" - + " response for now. Need to decide what to do with these test cases.") public class PluginIT extends SQLIntegTestCase { @Override @@ -30,6 +27,9 @@ protected void init() throws Exception { wipeAllClusterSettings(); } + @Ignore( + "OpenSearch DSL format is deprecated in 3.0.0. Ignore legacy IT that relies on json format" + + " response for now. Need to decide what to do with these test cases.") @Test public void sqlEnableSettingsTest() throws IOException { loadIndex(Index.ACCOUNT); From 085647f04036317685fc9f2ee2f44c950fbc65f5 Mon Sep 17 00:00:00 2001 From: Sean Kao Date: Tue, 4 Mar 2025 12:24:33 -0800 Subject: [PATCH 21/21] spotlessApply Signed-off-by: Sean Kao --- .../sql/legacy/unittest/StringOperatorsTest.java | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/StringOperatorsTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/StringOperatorsTest.java index 8362b711926..264e74ed136 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/StringOperatorsTest.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/StringOperatorsTest.java @@ -50,8 +50,7 @@ public void substringIndexOutOfBoundTest() { @Test public void lengthTest() { - String query = - "SELECT length(lastname) FROM accounts WHERE length(lastname) = 5"; + String query = "SELECT length(lastname) FROM accounts WHERE length(lastname) = 5"; ScriptField scriptField = CheckScriptContents.getScriptFieldFromQuery(query); assertTrue( @@ -65,7 +64,8 @@ public void lengthTest() { @Test public void replaceTest() { String query = - "SELECT replace(lastname, 'a', 'A') FROM accounts WHERE replace(lastname, 'a', 'A') = 'aba'"; + "SELECT replace(lastname, 'a', 'A') FROM accounts WHERE replace(lastname, 'a', 'A') =" + + " 'aba'"; ScriptField scriptField = CheckScriptContents.getScriptFieldFromQuery(query); assertTrue( @@ -96,8 +96,7 @@ public void locateTest() { @Test public void ltrimTest() { - String query = - "SELECT ltrim(lastname) FROM accounts WHERE ltrim(lastname) = 'abc'"; + String query = "SELECT ltrim(lastname) FROM accounts WHERE ltrim(lastname) = 'abc'"; ScriptField scriptField = CheckScriptContents.getScriptFieldFromQuery(query); assertTrue( @@ -112,8 +111,7 @@ public void ltrimTest() { @Test public void rtrimTest() { - String query = - "SELECT rtrim(lastname) FROM accounts WHERE rtrim(lastname) = 'cba'"; + String query = "SELECT rtrim(lastname) FROM accounts WHERE rtrim(lastname) = 'cba'"; ScriptField scriptField = CheckScriptContents.getScriptFieldFromQuery(query); assertTrue( @@ -128,8 +126,7 @@ public void rtrimTest() { @Test public void asciiTest() { - String query = - "SELECT ascii(lastname) FROM accounts WHERE ascii(lastname) = 108"; + String query = "SELECT ascii(lastname) FROM accounts WHERE ascii(lastname) = 108"; ScriptField scriptField = CheckScriptContents.getScriptFieldFromQuery(query); assertTrue(