From 9f2ad1d889b3faba5195ef5c194d84c19950ae32 Mon Sep 17 00:00:00 2001 From: Margarit Hakobyan Date: Wed, 1 Feb 2023 16:01:55 -0800 Subject: [PATCH 01/38] Implement json support for V2 engine Signed-off-by: Margarit Hakobyan --- .../sql/executor/ExecutionEngine.java | 1 + .../function/BuiltinFunctionName.java | 2 +- .../sql/executor/DefaultExecutionEngine.java | 2 +- .../sql/legacy/plugin/RestSQLQueryAction.java | 9 +- .../executor/OpenSearchExecutionEngine.java | 6 +- .../sql/protocol/response/QueryResult.java | 5 + .../response/format/ErrorFormatter.java | 43 +++- .../sql/protocol/response/format/Format.java | 1 + .../format/JsonJsonResponseFormatter.java | 187 ++++++++++++++++++ .../sql/sql/domain/SQLQueryRequest.java | 7 +- 10 files changed, 255 insertions(+), 8 deletions(-) create mode 100644 protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonJsonResponseFormatter.java diff --git a/core/src/main/java/org/opensearch/sql/executor/ExecutionEngine.java b/core/src/main/java/org/opensearch/sql/executor/ExecutionEngine.java index 1936a0f517..7fc602def2 100644 --- a/core/src/main/java/org/opensearch/sql/executor/ExecutionEngine.java +++ b/core/src/main/java/org/opensearch/sql/executor/ExecutionEngine.java @@ -53,6 +53,7 @@ void execute(PhysicalPlan plan, ExecutionContext context, class QueryResponse { private final Schema schema; private final List results; + private final String indexName; } @Data diff --git a/core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java b/core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java index ec4a7bc140..833b6ee163 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java @@ -263,7 +263,7 @@ public enum BuiltinFunctionName { ALL_NATIVE_FUNCTIONS = builder.build(); } - private static final Map AGGREGATION_FUNC_MAPPING = + public static final Map AGGREGATION_FUNC_MAPPING = new ImmutableMap.Builder() .put("max", BuiltinFunctionName.MAX) .put("min", BuiltinFunctionName.MIN) diff --git a/core/src/testFixtures/java/org/opensearch/sql/executor/DefaultExecutionEngine.java b/core/src/testFixtures/java/org/opensearch/sql/executor/DefaultExecutionEngine.java index e4f9a185a3..ddf528887c 100644 --- a/core/src/testFixtures/java/org/opensearch/sql/executor/DefaultExecutionEngine.java +++ b/core/src/testFixtures/java/org/opensearch/sql/executor/DefaultExecutionEngine.java @@ -32,7 +32,7 @@ public void execute( while (plan.hasNext()) { result.add(plan.next()); } - QueryResponse response = new QueryResponse(new Schema(new ArrayList<>()), new ArrayList<>()); + QueryResponse response = new QueryResponse(new Schema(new ArrayList<>()), new ArrayList<>(), ""); listener.onResponse(response); } catch (Exception e) { listener.onFailure(e); diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java index bc97f71b47..faec6d58e4 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java @@ -32,6 +32,7 @@ import org.opensearch.sql.protocol.response.format.CsvResponseFormatter; import org.opensearch.sql.protocol.response.format.Format; import org.opensearch.sql.protocol.response.format.JdbcResponseFormatter; +import org.opensearch.sql.protocol.response.format.JsonJsonResponseFormatter; import org.opensearch.sql.protocol.response.format.JsonResponseFormatter; import org.opensearch.sql.protocol.response.format.RawResponseFormatter; import org.opensearch.sql.protocol.response.format.ResponseFormatter; @@ -165,14 +166,18 @@ private ResponseListener createQueryResponseListener( formatter = new CsvResponseFormatter(request.sanitize()); } else if (format.equals(Format.RAW)) { formatter = new RawResponseFormatter(); + } else if (format.equals(Format.JSON)) { + formatter = new JsonJsonResponseFormatter(PRETTY); } else { formatter = new JdbcResponseFormatter(PRETTY); } - return new ResponseListener() { + return new ResponseListener<>() { @Override public void onResponse(QueryResponse response) { + QueryResult queryResult = new QueryResult(response.getSchema(), response.getResults()); + queryResult.setIndexName(response.getIndexName()); sendResponse(channel, OK, - formatter.format(new QueryResult(response.getSchema(), response.getResults()))); + formatter.format(queryResult)); } @Override diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java index 9a136a3bec..e12a474c55 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java @@ -17,7 +17,9 @@ import org.opensearch.sql.executor.Explain; import org.opensearch.sql.opensearch.client.OpenSearchClient; import org.opensearch.sql.opensearch.executor.protector.ExecutionProtector; +import org.opensearch.sql.opensearch.storage.OpenSearchIndexScan; import org.opensearch.sql.planner.physical.PhysicalPlan; +import org.opensearch.sql.planner.physical.ProjectOperator; import org.opensearch.sql.storage.TableScanOperator; /** OpenSearch execution engine implementation. */ @@ -48,8 +50,10 @@ public void execute(PhysicalPlan physicalPlan, ExecutionContext context, while (plan.hasNext()) { result.add(plan.next()); } + String indexName = ((OpenSearchIndexScan) ((ProjectOperator) physicalPlan).getInput()) + .getRequestBuilder().getIndexName().toString(); - QueryResponse response = new QueryResponse(physicalPlan.schema(), result); + QueryResponse response = new QueryResponse(physicalPlan.schema(), result, indexName); listener.onResponse(response); } catch (Exception e) { listener.onFailure(e); diff --git a/protocol/src/main/java/org/opensearch/sql/protocol/response/QueryResult.java b/protocol/src/main/java/org/opensearch/sql/protocol/response/QueryResult.java index 915a61f361..413ebba4d7 100644 --- a/protocol/src/main/java/org/opensearch/sql/protocol/response/QueryResult.java +++ b/protocol/src/main/java/org/opensearch/sql/protocol/response/QueryResult.java @@ -12,6 +12,7 @@ import java.util.Map; import lombok.Getter; import lombok.RequiredArgsConstructor; +import lombok.Setter; import org.opensearch.sql.data.model.ExprValue; import org.opensearch.sql.data.model.ExprValueUtils; import org.opensearch.sql.executor.ExecutionEngine; @@ -32,6 +33,10 @@ public class QueryResult implements Iterable { */ private final Collection exprValues; + @Getter + @Setter + private String indexName; + /** * size of results. diff --git a/protocol/src/main/java/org/opensearch/sql/protocol/response/format/ErrorFormatter.java b/protocol/src/main/java/org/opensearch/sql/protocol/response/format/ErrorFormatter.java index 40848e959b..3e470d2165 100644 --- a/protocol/src/main/java/org/opensearch/sql/protocol/response/format/ErrorFormatter.java +++ b/protocol/src/main/java/org/opensearch/sql/protocol/response/format/ErrorFormatter.java @@ -22,9 +22,22 @@ public class ErrorFormatter { .setPrettyPrinting() .disableHtmlEscaping() .create()); + + private static final Gson PRETTY_PRINT_GSON_WITH_NULLS = AccessController.doPrivileged( + (PrivilegedAction) () -> new GsonBuilder() + .setPrettyPrinting() + .disableHtmlEscaping() + .serializeNulls() + .create()); private static final Gson GSON = AccessController.doPrivileged( (PrivilegedAction) () -> new GsonBuilder().disableHtmlEscaping().create()); + private static final Gson GSON_WITH_NULLS = AccessController.doPrivileged( + (PrivilegedAction) () -> new GsonBuilder() + .disableHtmlEscaping() + .serializeNulls() + .create()); + /** * Util method to format {@link Throwable} response to JSON string in compact printing. */ @@ -37,22 +50,50 @@ public static String compactFormat(Throwable t) { /** * Util method to format {@link Throwable} response to JSON string in pretty printing. */ - public static String prettyFormat(Throwable t) { + public static String prettyFormat(Throwable t) { JsonError error = new ErrorFormatter.JsonError(t.getClass().getSimpleName(), t.getMessage()); return prettyJsonify(error); } + /** + * Util method to format {@link Throwable} response to JSON string in compact printing. + */ + public static String compactFormatWithNulls(Throwable t) { + JsonError error = new ErrorFormatter.JsonError(t.getClass().getSimpleName(), + t.getMessage()); + return compactJsonifyWithNullValues(error); + } + + /** + * Util method to format {@link Throwable} response to JSON string in pretty printing. + */ + public static String prettyFormatWithNulls(Throwable t) { + JsonError error = new ErrorFormatter.JsonError(t.getClass().getSimpleName(), + t.getMessage()); + return prettyJsonifyWithNulls(error); + } + public static String compactJsonify(Object jsonObject) { return AccessController.doPrivileged( (PrivilegedAction) () -> GSON.toJson(jsonObject)); } + public static String compactJsonifyWithNullValues(Object jsonObject) { + return AccessController.doPrivileged( + (PrivilegedAction) () -> GSON_WITH_NULLS.toJson(jsonObject)); + } + public static String prettyJsonify(Object jsonObject) { return AccessController.doPrivileged( (PrivilegedAction) () -> PRETTY_PRINT_GSON.toJson(jsonObject)); } + public static String prettyJsonifyWithNulls(Object jsonObject) { + return AccessController.doPrivileged( + (PrivilegedAction) () -> PRETTY_PRINT_GSON_WITH_NULLS.toJson(jsonObject)); + } + @RequiredArgsConstructor @Getter public static class JsonError { diff --git a/protocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.java b/protocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.java index 4291c09df0..4d1a84f47b 100644 --- a/protocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.java +++ b/protocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.java @@ -16,6 +16,7 @@ @RequiredArgsConstructor public enum Format { JDBC("jdbc"), + JSON("json"), CSV("csv"), RAW("raw"), VIZ("viz"); diff --git a/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonJsonResponseFormatter.java b/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonJsonResponseFormatter.java new file mode 100644 index 0000000000..29e539087d --- /dev/null +++ b/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonJsonResponseFormatter.java @@ -0,0 +1,187 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + + +package org.opensearch.sql.protocol.response.format; + +import static org.opensearch.sql.expression.function.BuiltinFunctionName.AGGREGATION_FUNC_MAPPING; +import static org.opensearch.sql.protocol.response.format.ErrorFormatter.compactJsonifyWithNullValues; +import static org.opensearch.sql.protocol.response.format.ErrorFormatter.prettyJsonifyWithNulls; +import static org.opensearch.sql.protocol.response.format.JsonResponseFormatter.Style.PRETTY; + +import com.google.gson.annotations.SerializedName; +import java.security.AccessController; +import java.security.PrivilegedAction; +import java.util.Arrays; +import java.util.HashMap; +import java.util.LinkedHashMap; +import java.util.LinkedList; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; +import lombok.Builder; +import lombok.Getter; +import lombok.RequiredArgsConstructor; +import lombok.Singular; +import org.opensearch.sql.protocol.response.QueryResult; + +public class JsonJsonResponseFormatter extends JsonResponseFormatter { + + private final Style style; + + public JsonJsonResponseFormatter(Style style) { + super(style); + this.style = style; + } + + @Override + public Object buildJsonObject(QueryResult response) { + JsonResponse.JsonResponseBuilder json = JsonResponse.builder(); + Total total = new Total(response.size(), "eq"); + Shards shards = new Shards(1, 1, 0, 0); + List columnNames = new LinkedList<>(response.columnNameTypes().keySet()); + List hitList = new LinkedList<>(); + Map aggregations = new LinkedHashMap<>(); + + int id = 1; + for (Object[] values : getRows(response)) { + List valueList = new LinkedList<>(Arrays.asList(values)); + Map source = combineListsIntoMap(columnNames, valueList); + + Map filteredSource = filterSourceFromFunctions(source); + Map functions = getFunctions(source); + Map aggregationMap = getAggregations(source); + aggregations.putAll(aggregationMap); + + Hit hit = new Hit(response.getIndexName(), Integer.toString(id), 1.0, filteredSource, + functions.isEmpty() ? null : functions); + + hitList.add(hit); + id++; + } + Hits hits = new Hits(total, 1.0, hitList, aggregations.isEmpty() ? null : aggregations); + + json.took(0) + .timedOut(false) + .shards(shards) + .hits(hits); + + return json.build(); + } + + @Override + protected String jsonify(Object jsonObject) { + return AccessController.doPrivileged((PrivilegedAction) () -> + (style == PRETTY) ? prettyJsonifyWithNulls(jsonObject) + : compactJsonifyWithNullValues(jsonObject)); + } + + private Map getAggregations(Map source) { + return source.entrySet() + .stream().filter(entry -> isAggregationFunction(entry.getKey())) + .collect(Collectors.toMap(Map.Entry::getKey, + stringObjectEntry -> new Aggregation(stringObjectEntry.getValue()))); + } + + private Map getFunctions(Map source) { + return source.entrySet() + .stream().filter(entry -> isFunctionName(entry.getKey()) + && !isAggregationFunction(entry.getKey())) + .collect(HashMap::new, (map, values) -> + map.put(values.getKey(), values.getValue()), HashMap::putAll); + } + + private Map filterSourceFromFunctions(Map source) { + return source.entrySet() + .stream().filter(entry -> !isFunctionName(entry.getKey())) + .collect(HashMap::new, (map, values) -> + map.put(values.getKey(), values.getValue()), HashMap::putAll); + } + + private Map combineListsIntoMap(List keys, List values) { + Map map = new LinkedHashMap<>(); + for (int i = 0; i < keys.size(); i++) { + map.put(keys.get(i), values.get(i)); + } + return map; + } + + private List getRows(QueryResult response) { + List rows = new LinkedList<>(); + for (Object[] values : response) { + rows.add(values); + } + return rows; + } + + private boolean isAggregationFunction(String value) { + return isFunctionName(value) + && AGGREGATION_FUNC_MAPPING.containsKey(value.split("[(]")[0].toLowerCase()); + } + + private boolean isFunctionName(String value) { + return value.contains("(") && value.contains(")"); + } + + @Builder + @Getter + public static class JsonResponse { + public final int took; + @SerializedName("timed_out") + public final boolean timedOut; + @SerializedName("_shards") + public final Shards shards; + public final Hits hits; + } + + @RequiredArgsConstructor + @Getter + public static class Hits { + public final Total total; + @SerializedName("max_score") + public final double maxScore; + @Singular("column") + public final List hits; + public final Map aggregations; + } + + @RequiredArgsConstructor + @Getter + public static class Hit { + @SerializedName("_index") + public final String index; + @SerializedName("_id") + public final String id; + @SerializedName("_score") + public final double score; + @SerializedName("_source") + public final Map source; + public final Map fields; + } + + @RequiredArgsConstructor + @Getter + public static class Shards { + public final int total; + public final int successful; + public final int skipped; + public final int failed; + } + + @RequiredArgsConstructor + @Getter + public static class Total { + public final int value; + public final String relation; + } + + @RequiredArgsConstructor + @Getter + public static class Aggregation { + public final Object value; + } + + +} diff --git a/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java b/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java index 508f80cee4..3e64e7a46b 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java +++ b/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java @@ -121,8 +121,11 @@ private boolean isFetchSizeZeroIfPresent() { } private boolean isSupportedFormat() { - return Strings.isNullOrEmpty(format) || "jdbc".equalsIgnoreCase(format) - || "csv".equalsIgnoreCase(format) || "raw".equalsIgnoreCase(format); + return Strings.isNullOrEmpty(format) + || "jdbc".equalsIgnoreCase(format) + || "csv".equalsIgnoreCase(format) + || "raw".equalsIgnoreCase(format) + || "json".equalsIgnoreCase(format); } private String getFormat(Map params) { From 3167ee5e580f7a7830a0c37352e944fae53f4c82 Mon Sep 17 00:00:00 2001 From: Margarit Hakobyan Date: Thu, 2 Feb 2023 13:12:07 -0800 Subject: [PATCH 02/38] Reverted some changes Signed-off-by: Margarit Hakobyan --- .../sql/executor/ExecutionEngine.java | 1 - .../sql/executor/DefaultExecutionEngine.java | 2 +- .../sql/legacy/plugin/RestSQLQueryAction.java | 1 - .../executor/OpenSearchExecutionEngine.java | 6 +-- .../sql/protocol/response/QueryResult.java | 5 --- .../response/format/ErrorFormatter.java | 43 +------------------ .../format/JsonJsonResponseFormatter.java | 14 +----- 7 files changed, 4 insertions(+), 68 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/executor/ExecutionEngine.java b/core/src/main/java/org/opensearch/sql/executor/ExecutionEngine.java index 7fc602def2..1936a0f517 100644 --- a/core/src/main/java/org/opensearch/sql/executor/ExecutionEngine.java +++ b/core/src/main/java/org/opensearch/sql/executor/ExecutionEngine.java @@ -53,7 +53,6 @@ void execute(PhysicalPlan plan, ExecutionContext context, class QueryResponse { private final Schema schema; private final List results; - private final String indexName; } @Data diff --git a/core/src/testFixtures/java/org/opensearch/sql/executor/DefaultExecutionEngine.java b/core/src/testFixtures/java/org/opensearch/sql/executor/DefaultExecutionEngine.java index ddf528887c..e4f9a185a3 100644 --- a/core/src/testFixtures/java/org/opensearch/sql/executor/DefaultExecutionEngine.java +++ b/core/src/testFixtures/java/org/opensearch/sql/executor/DefaultExecutionEngine.java @@ -32,7 +32,7 @@ public void execute( while (plan.hasNext()) { result.add(plan.next()); } - QueryResponse response = new QueryResponse(new Schema(new ArrayList<>()), new ArrayList<>(), ""); + QueryResponse response = new QueryResponse(new Schema(new ArrayList<>()), new ArrayList<>()); listener.onResponse(response); } catch (Exception e) { listener.onFailure(e); diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java index faec6d58e4..58a386d6f6 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java @@ -175,7 +175,6 @@ private ResponseListener createQueryResponseListener( @Override public void onResponse(QueryResponse response) { QueryResult queryResult = new QueryResult(response.getSchema(), response.getResults()); - queryResult.setIndexName(response.getIndexName()); sendResponse(channel, OK, formatter.format(queryResult)); } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java index e12a474c55..9a136a3bec 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java @@ -17,9 +17,7 @@ import org.opensearch.sql.executor.Explain; import org.opensearch.sql.opensearch.client.OpenSearchClient; import org.opensearch.sql.opensearch.executor.protector.ExecutionProtector; -import org.opensearch.sql.opensearch.storage.OpenSearchIndexScan; import org.opensearch.sql.planner.physical.PhysicalPlan; -import org.opensearch.sql.planner.physical.ProjectOperator; import org.opensearch.sql.storage.TableScanOperator; /** OpenSearch execution engine implementation. */ @@ -50,10 +48,8 @@ public void execute(PhysicalPlan physicalPlan, ExecutionContext context, while (plan.hasNext()) { result.add(plan.next()); } - String indexName = ((OpenSearchIndexScan) ((ProjectOperator) physicalPlan).getInput()) - .getRequestBuilder().getIndexName().toString(); - QueryResponse response = new QueryResponse(physicalPlan.schema(), result, indexName); + QueryResponse response = new QueryResponse(physicalPlan.schema(), result); listener.onResponse(response); } catch (Exception e) { listener.onFailure(e); diff --git a/protocol/src/main/java/org/opensearch/sql/protocol/response/QueryResult.java b/protocol/src/main/java/org/opensearch/sql/protocol/response/QueryResult.java index 413ebba4d7..915a61f361 100644 --- a/protocol/src/main/java/org/opensearch/sql/protocol/response/QueryResult.java +++ b/protocol/src/main/java/org/opensearch/sql/protocol/response/QueryResult.java @@ -12,7 +12,6 @@ import java.util.Map; import lombok.Getter; import lombok.RequiredArgsConstructor; -import lombok.Setter; import org.opensearch.sql.data.model.ExprValue; import org.opensearch.sql.data.model.ExprValueUtils; import org.opensearch.sql.executor.ExecutionEngine; @@ -33,10 +32,6 @@ public class QueryResult implements Iterable { */ private final Collection exprValues; - @Getter - @Setter - private String indexName; - /** * size of results. diff --git a/protocol/src/main/java/org/opensearch/sql/protocol/response/format/ErrorFormatter.java b/protocol/src/main/java/org/opensearch/sql/protocol/response/format/ErrorFormatter.java index 3e470d2165..40848e959b 100644 --- a/protocol/src/main/java/org/opensearch/sql/protocol/response/format/ErrorFormatter.java +++ b/protocol/src/main/java/org/opensearch/sql/protocol/response/format/ErrorFormatter.java @@ -22,22 +22,9 @@ public class ErrorFormatter { .setPrettyPrinting() .disableHtmlEscaping() .create()); - - private static final Gson PRETTY_PRINT_GSON_WITH_NULLS = AccessController.doPrivileged( - (PrivilegedAction) () -> new GsonBuilder() - .setPrettyPrinting() - .disableHtmlEscaping() - .serializeNulls() - .create()); private static final Gson GSON = AccessController.doPrivileged( (PrivilegedAction) () -> new GsonBuilder().disableHtmlEscaping().create()); - private static final Gson GSON_WITH_NULLS = AccessController.doPrivileged( - (PrivilegedAction) () -> new GsonBuilder() - .disableHtmlEscaping() - .serializeNulls() - .create()); - /** * Util method to format {@link Throwable} response to JSON string in compact printing. */ @@ -50,50 +37,22 @@ public static String compactFormat(Throwable t) { /** * Util method to format {@link Throwable} response to JSON string in pretty printing. */ - public static String prettyFormat(Throwable t) { + public static String prettyFormat(Throwable t) { JsonError error = new ErrorFormatter.JsonError(t.getClass().getSimpleName(), t.getMessage()); return prettyJsonify(error); } - /** - * Util method to format {@link Throwable} response to JSON string in compact printing. - */ - public static String compactFormatWithNulls(Throwable t) { - JsonError error = new ErrorFormatter.JsonError(t.getClass().getSimpleName(), - t.getMessage()); - return compactJsonifyWithNullValues(error); - } - - /** - * Util method to format {@link Throwable} response to JSON string in pretty printing. - */ - public static String prettyFormatWithNulls(Throwable t) { - JsonError error = new ErrorFormatter.JsonError(t.getClass().getSimpleName(), - t.getMessage()); - return prettyJsonifyWithNulls(error); - } - public static String compactJsonify(Object jsonObject) { return AccessController.doPrivileged( (PrivilegedAction) () -> GSON.toJson(jsonObject)); } - public static String compactJsonifyWithNullValues(Object jsonObject) { - return AccessController.doPrivileged( - (PrivilegedAction) () -> GSON_WITH_NULLS.toJson(jsonObject)); - } - public static String prettyJsonify(Object jsonObject) { return AccessController.doPrivileged( (PrivilegedAction) () -> PRETTY_PRINT_GSON.toJson(jsonObject)); } - public static String prettyJsonifyWithNulls(Object jsonObject) { - return AccessController.doPrivileged( - (PrivilegedAction) () -> PRETTY_PRINT_GSON_WITH_NULLS.toJson(jsonObject)); - } - @RequiredArgsConstructor @Getter public static class JsonError { diff --git a/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonJsonResponseFormatter.java b/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonJsonResponseFormatter.java index 29e539087d..eb74bfc8bd 100644 --- a/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonJsonResponseFormatter.java +++ b/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonJsonResponseFormatter.java @@ -7,13 +7,8 @@ package org.opensearch.sql.protocol.response.format; import static org.opensearch.sql.expression.function.BuiltinFunctionName.AGGREGATION_FUNC_MAPPING; -import static org.opensearch.sql.protocol.response.format.ErrorFormatter.compactJsonifyWithNullValues; -import static org.opensearch.sql.protocol.response.format.ErrorFormatter.prettyJsonifyWithNulls; -import static org.opensearch.sql.protocol.response.format.JsonResponseFormatter.Style.PRETTY; import com.google.gson.annotations.SerializedName; -import java.security.AccessController; -import java.security.PrivilegedAction; import java.util.Arrays; import java.util.HashMap; import java.util.LinkedHashMap; @@ -55,7 +50,7 @@ public Object buildJsonObject(QueryResult response) { Map aggregationMap = getAggregations(source); aggregations.putAll(aggregationMap); - Hit hit = new Hit(response.getIndexName(), Integer.toString(id), 1.0, filteredSource, + Hit hit = new Hit("unknown", Integer.toString(id), 1.0, filteredSource, functions.isEmpty() ? null : functions); hitList.add(hit); @@ -71,13 +66,6 @@ public Object buildJsonObject(QueryResult response) { return json.build(); } - @Override - protected String jsonify(Object jsonObject) { - return AccessController.doPrivileged((PrivilegedAction) () -> - (style == PRETTY) ? prettyJsonifyWithNulls(jsonObject) - : compactJsonifyWithNullValues(jsonObject)); - } - private Map getAggregations(Map source) { return source.entrySet() .stream().filter(entry -> isAggregationFunction(entry.getKey())) From 5e7a28ddab5d28d4b3496caff86d0e43f95c3ca6 Mon Sep 17 00:00:00 2001 From: Margarit Hakobyan Date: Fri, 3 Feb 2023 13:49:24 -0800 Subject: [PATCH 03/38] Removed some fields Signed-off-by: Margarit Hakobyan --- .../sql/legacy/plugin/RestSQLQueryAction.java | 4 +-- ...er.java => JsonTypeResponseFormatter.java} | 25 ++++--------------- 2 files changed, 7 insertions(+), 22 deletions(-) rename protocol/src/main/java/org/opensearch/sql/protocol/response/format/{JsonJsonResponseFormatter.java => JsonTypeResponseFormatter.java} (86%) diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java index 58a386d6f6..1f1524a448 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java @@ -32,7 +32,7 @@ import org.opensearch.sql.protocol.response.format.CsvResponseFormatter; import org.opensearch.sql.protocol.response.format.Format; import org.opensearch.sql.protocol.response.format.JdbcResponseFormatter; -import org.opensearch.sql.protocol.response.format.JsonJsonResponseFormatter; +import org.opensearch.sql.protocol.response.format.JsonTypeResponseFormatter; import org.opensearch.sql.protocol.response.format.JsonResponseFormatter; import org.opensearch.sql.protocol.response.format.RawResponseFormatter; import org.opensearch.sql.protocol.response.format.ResponseFormatter; @@ -167,7 +167,7 @@ private ResponseListener createQueryResponseListener( } else if (format.equals(Format.RAW)) { formatter = new RawResponseFormatter(); } else if (format.equals(Format.JSON)) { - formatter = new JsonJsonResponseFormatter(PRETTY); + formatter = new JsonTypeResponseFormatter(PRETTY); } else { formatter = new JdbcResponseFormatter(PRETTY); } diff --git a/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonJsonResponseFormatter.java b/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonTypeResponseFormatter.java similarity index 86% rename from protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonJsonResponseFormatter.java rename to protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonTypeResponseFormatter.java index eb74bfc8bd..3e804af8f6 100644 --- a/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonJsonResponseFormatter.java +++ b/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonTypeResponseFormatter.java @@ -22,11 +22,11 @@ import lombok.Singular; import org.opensearch.sql.protocol.response.QueryResult; -public class JsonJsonResponseFormatter extends JsonResponseFormatter { +public class JsonTypeResponseFormatter extends JsonResponseFormatter { private final Style style; - public JsonJsonResponseFormatter(Style style) { + public JsonTypeResponseFormatter(Style style) { super(style); this.style = style; } @@ -35,12 +35,11 @@ public JsonJsonResponseFormatter(Style style) { public Object buildJsonObject(QueryResult response) { JsonResponse.JsonResponseBuilder json = JsonResponse.builder(); Total total = new Total(response.size(), "eq"); - Shards shards = new Shards(1, 1, 0, 0); + Shards shards = new Shards(0, 0, 0, 0); List columnNames = new LinkedList<>(response.columnNameTypes().keySet()); List hitList = new LinkedList<>(); Map aggregations = new LinkedHashMap<>(); - int id = 1; for (Object[] values : getRows(response)) { List valueList = new LinkedList<>(Arrays.asList(values)); Map source = combineListsIntoMap(columnNames, valueList); @@ -50,18 +49,13 @@ public Object buildJsonObject(QueryResult response) { Map aggregationMap = getAggregations(source); aggregations.putAll(aggregationMap); - Hit hit = new Hit("unknown", Integer.toString(id), 1.0, filteredSource, - functions.isEmpty() ? null : functions); + Hit hit = new Hit(filteredSource, functions.isEmpty() ? null : functions); hitList.add(hit); - id++; } Hits hits = new Hits(total, 1.0, hitList, aggregations.isEmpty() ? null : aggregations); - json.took(0) - .timedOut(false) - .shards(shards) - .hits(hits); + json.shards(shards).hits(hits); return json.build(); } @@ -116,9 +110,6 @@ private boolean isFunctionName(String value) { @Builder @Getter public static class JsonResponse { - public final int took; - @SerializedName("timed_out") - public final boolean timedOut; @SerializedName("_shards") public final Shards shards; public final Hits hits; @@ -138,12 +129,6 @@ public static class Hits { @RequiredArgsConstructor @Getter public static class Hit { - @SerializedName("_index") - public final String index; - @SerializedName("_id") - public final String id; - @SerializedName("_score") - public final double score; @SerializedName("_source") public final Map source; public final Map fields; From ab98db4f5a23484af76a551270568eab77881df7 Mon Sep 17 00:00:00 2001 From: Margarit Hakobyan Date: Fri, 3 Feb 2023 13:53:54 -0800 Subject: [PATCH 04/38] minor fix Signed-off-by: Margarit Hakobyan --- .../org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java index 1f1524a448..cd951d4c77 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java @@ -174,9 +174,8 @@ private ResponseListener createQueryResponseListener( return new ResponseListener<>() { @Override public void onResponse(QueryResponse response) { - QueryResult queryResult = new QueryResult(response.getSchema(), response.getResults()); sendResponse(channel, OK, - formatter.format(queryResult)); + formatter.format(new QueryResult(response.getSchema(), response.getResults()))); } @Override From 26920d0e597e741179d734c4bf0d705f3f46682b Mon Sep 17 00:00:00 2001 From: Margarit Hakobyan Date: Mon, 6 Feb 2023 15:47:15 -0800 Subject: [PATCH 05/38] Added a unit test, cleaned up Signed-off-by: Margarit Hakobyan --- .../format/JsonTypeResponseFormatter.java | 34 +++- .../format/JsonTypeResponseFormatterTest.java | 186 ++++++++++++++++++ 2 files changed, 216 insertions(+), 4 deletions(-) create mode 100644 protocol/src/test/java/org/opensearch/sql/protocol/response/format/JsonTypeResponseFormatterTest.java diff --git a/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonTypeResponseFormatter.java b/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonTypeResponseFormatter.java index 3e804af8f6..51664b8bb8 100644 --- a/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonTypeResponseFormatter.java +++ b/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonTypeResponseFormatter.java @@ -20,8 +20,15 @@ import lombok.Getter; import lombok.RequiredArgsConstructor; import lombok.Singular; +import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.exception.QueryEngineException; +import org.opensearch.sql.opensearch.response.error.ErrorMessage; +import org.opensearch.sql.opensearch.response.error.ErrorMessageFactory; import org.opensearch.sql.protocol.response.QueryResult; +/** + * Response formatter to format response to json format. + */ public class JsonTypeResponseFormatter extends JsonResponseFormatter { private final Style style; @@ -53,13 +60,29 @@ public Object buildJsonObject(QueryResult response) { hitList.add(hit); } - Hits hits = new Hits(total, 1.0, hitList, aggregations.isEmpty() ? null : aggregations); + Hits hits = new Hits(total, hitList, aggregations.isEmpty() ? null : aggregations); json.shards(shards).hits(hits); return json.build(); } + @Override + public String format(Throwable t) { + int status = getStatus(t); + ErrorMessage message = ErrorMessageFactory.createErrorMessage(t, status); + JdbcResponseFormatter.Error error = new JdbcResponseFormatter.Error( + message.getType(), + message.getReason(), + message.getDetails()); + return jsonify(new JsonTypeErrorResponse(error, status)); + } + + private int getStatus(Throwable t) { + return (t instanceof SyntaxCheckException + || t instanceof QueryEngineException) ? 400 : 503; + } + private Map getAggregations(Map source) { return source.entrySet() .stream().filter(entry -> isAggregationFunction(entry.getKey())) @@ -119,8 +142,6 @@ public static class JsonResponse { @Getter public static class Hits { public final Total total; - @SerializedName("max_score") - public final double maxScore; @Singular("column") public final List hits; public final Map aggregations; @@ -156,5 +177,10 @@ public static class Aggregation { public final Object value; } - + @RequiredArgsConstructor + @Getter + public static class JsonTypeErrorResponse { + private final JdbcResponseFormatter.Error error; + private final int status; + } } diff --git a/protocol/src/test/java/org/opensearch/sql/protocol/response/format/JsonTypeResponseFormatterTest.java b/protocol/src/test/java/org/opensearch/sql/protocol/response/format/JsonTypeResponseFormatterTest.java new file mode 100644 index 0000000000..009867cbd1 --- /dev/null +++ b/protocol/src/test/java/org/opensearch/sql/protocol/response/format/JsonTypeResponseFormatterTest.java @@ -0,0 +1,186 @@ +package org.opensearch.sql.protocol.response.format; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.opensearch.sql.data.model.ExprValueUtils.LITERAL_MISSING; +import static org.opensearch.sql.data.model.ExprValueUtils.LITERAL_NULL; +import static org.opensearch.sql.data.model.ExprValueUtils.stringValue; +import static org.opensearch.sql.data.model.ExprValueUtils.tupleValue; +import static org.opensearch.sql.data.type.ExprCoreType.ARRAY; +import static org.opensearch.sql.data.type.ExprCoreType.INTEGER; +import static org.opensearch.sql.data.type.ExprCoreType.STRING; +import static org.opensearch.sql.data.type.ExprCoreType.STRUCT; +import static org.opensearch.sql.opensearch.data.type.OpenSearchDataType.OPENSEARCH_TEXT; +import static org.opensearch.sql.opensearch.data.type.OpenSearchDataType.OPENSEARCH_TEXT_KEYWORD; +import static org.opensearch.sql.protocol.response.format.JsonResponseFormatter.Style.COMPACT; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.gson.JsonParser; +import java.util.Arrays; +import org.junit.jupiter.api.DisplayNameGeneration; +import org.junit.jupiter.api.DisplayNameGenerator; +import org.junit.jupiter.api.Test; +import org.opensearch.OpenSearchException; +import org.opensearch.sql.common.antlr.SyntaxCheckException; +import org.opensearch.sql.data.model.ExprTupleValue; +import org.opensearch.sql.exception.SemanticCheckException; +import org.opensearch.sql.executor.ExecutionEngine; +import org.opensearch.sql.protocol.response.QueryResult; + +@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) +public class JsonTypeResponseFormatterTest { + + private final JsonTypeResponseFormatter formatter = new JsonTypeResponseFormatter(COMPACT); + + @Test + void format_response() { + QueryResult response = new QueryResult( + new ExecutionEngine.Schema(ImmutableList.of( + new ExecutionEngine.Schema.Column("name", "name", STRING), + new ExecutionEngine.Schema.Column("address1", "address1", OPENSEARCH_TEXT), + new ExecutionEngine.Schema.Column("address2", "address2", OPENSEARCH_TEXT_KEYWORD), + new ExecutionEngine.Schema.Column("location", "location", STRUCT), + new ExecutionEngine.Schema.Column("employer", "employer", ARRAY), + new ExecutionEngine.Schema.Column("age", "age", INTEGER))), + ImmutableList.of( + tupleValue(ImmutableMap.builder() + .put("name", "John") + .put("address1", "Seattle") + .put("address2", "WA") + .put("location", ImmutableMap.of("x", "1", "y", "2")) + .put("employments", ImmutableList.of( + ImmutableMap.of("name", "Amazon"), + ImmutableMap.of("name", "AWS"))) + .put("age", 20) + .build()))); + + assertJsonEquals( + "{\n" + + " \"_shards\":{\n" + + " \"total\":0,\n" + + " \"successful\":0,\n" + + " \"skipped\":0,\n" + + " \"failed\":0\n" + " },\n" + + " \"hits\":{\n" + + " \"total\":{\n" + + " \"value\":1,\n" + + " \"relation\":\"eq\"\n" + + " },\n" + + " \"hits\":[\n" + + " {\n" + + " \"_source\":{\n" + + " \"address2\":\"WA\",\n" + + " \"address1\":\"Seattle\",\n" + + " \"name\":\"John\",\n" + + " \"employer\":[\n" + + " {\n" + + " \"name\":\"Amazon\"\n" + + " },\n" + + " {\n" + + " \"name\":\"AWS\"\n" + + " }\n" + + " ],\n" + + " \"location\":{\n" + + " \"x\":\"1\",\n" + + " \"y\":\"2\"\n" + + " },\n" + + " \"age\":20\n" + + " }\n" + + " }\n" + + " ]\n" + + " }\n" + + "}", + formatter.format(response)); + } + + @Test + void format_response_with_missing_and_null_value() { + QueryResult response = + new QueryResult( + new ExecutionEngine.Schema(ImmutableList.of( + new ExecutionEngine.Schema.Column("name", null, STRING), + new ExecutionEngine.Schema.Column("age", null, INTEGER))), + Arrays.asList( + ExprTupleValue.fromExprValueMap( + ImmutableMap.of("name", stringValue("John"), "age", LITERAL_MISSING)), + ExprTupleValue.fromExprValueMap( + ImmutableMap.of("name", stringValue("Allen"), "age", LITERAL_NULL)), + tupleValue(ImmutableMap.of("name", "Smith", "age", 30)))); + + assertEquals( + "{\"_shards\":{\"total\":0,\"successful\":0,\"skipped\":0,\"failed\":0}," + + "\"hits\":{\"total\":{\"value\":3,\"relation\":\"eq\"}," + + "\"hits\":[{\"_source\":{\"name\":\"John\"}},{\"_source\":{\"name\":\"Allen\"}}," + + "{\"_source\":{\"name\":\"Smith\",\"age\":30}}]}}", + formatter.format(response)); + } + + @Test + void format_client_error_response_due_to_syntax_exception() { + assertJsonEquals( + "{\"error\":" + + "{\"" + + "type\":\"SyntaxCheckException\"," + + "\"reason\":\"Invalid Query\"," + + "\"details\":\"Invalid query syntax\"" + + "}," + + "\"status\":400}", + formatter.format(new SyntaxCheckException("Invalid query syntax")) + ); + } + + @Test + void format_client_error_response_due_to_semantic_exception() { + assertJsonEquals( + "{\"error\":" + + "{\"" + + "type\":\"SemanticCheckException\"," + + "\"reason\":\"Invalid Query\"," + + "\"details\":\"Invalid query semantics\"" + + "}," + + "\"status\":400}", + formatter.format(new SemanticCheckException("Invalid query semantics")) + ); + } + + @Test + void format_server_error_response() { + assertJsonEquals( + "{\"error\":" + + "{\"" + + "type\":\"IllegalStateException\"," + + "\"reason\":\"There was internal problem at backend\"," + + "\"details\":\"Execution error\"" + + "}," + + "\"status\":503}", + formatter.format(new IllegalStateException("Execution error")) + ); + } + + @Test + void format_server_error_response_due_to_opensearch() { + assertJsonEquals( + "{\"error\":" + + "{\"" + + "type\":\"OpenSearchException\"," + + "\"reason\":\"Error occurred in OpenSearch engine: all shards failed\"," + + "\"details\":\"OpenSearchException[all shards failed]; " + + "nested: IllegalStateException[Execution error];; " + + "java.lang.IllegalStateException: Execution error\\n" + + "For more details, please send request for Json format to see the raw response " + + "from OpenSearch engine.\"" + + "}," + + "\"status\":503}", + formatter.format(new OpenSearchException("all shards failed", + new IllegalStateException("Execution error"))) + ); + } + + private static void assertJsonEquals(String expected, String actual) { + assertEquals( + JsonParser.parseString(expected), + JsonParser.parseString(actual)); + } + +} + From 543773ece252043ac4dcaac7839b74a8a261b5f6 Mon Sep 17 00:00:00 2001 From: Margarit Hakobyan Date: Thu, 9 Feb 2023 11:16:43 -0800 Subject: [PATCH 06/38] Returning raw OpenSearch response when type is json Signed-off-by: Margarit Hakobyan --- .../sql/executor/ExecutionEngine.java | 1 + .../function/BuiltinFunctionName.java | 2 +- .../sql/executor/DefaultExecutionEngine.java | 2 +- .../sql/legacy/plugin/RestSQLQueryAction.java | 13 +- .../executor/OpenSearchExecutionEngine.java | 11 +- .../response/OpenSearchResponse.java | 5 + .../storage/OpenSearchIndexScan.java | 8 +- .../format/JsonTypeResponseFormatter.java | 186 ------------------ .../format/JsonTypeResponseFormatterTest.java | 186 ------------------ 9 files changed, 35 insertions(+), 379 deletions(-) delete mode 100644 protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonTypeResponseFormatter.java delete mode 100644 protocol/src/test/java/org/opensearch/sql/protocol/response/format/JsonTypeResponseFormatterTest.java diff --git a/core/src/main/java/org/opensearch/sql/executor/ExecutionEngine.java b/core/src/main/java/org/opensearch/sql/executor/ExecutionEngine.java index 1936a0f517..3a767304e8 100644 --- a/core/src/main/java/org/opensearch/sql/executor/ExecutionEngine.java +++ b/core/src/main/java/org/opensearch/sql/executor/ExecutionEngine.java @@ -53,6 +53,7 @@ void execute(PhysicalPlan plan, ExecutionContext context, class QueryResponse { private final Schema schema; private final List results; + private final String rawResponse; } @Data diff --git a/core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java b/core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java index 833b6ee163..ec4a7bc140 100644 --- a/core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java +++ b/core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java @@ -263,7 +263,7 @@ public enum BuiltinFunctionName { ALL_NATIVE_FUNCTIONS = builder.build(); } - public static final Map AGGREGATION_FUNC_MAPPING = + private static final Map AGGREGATION_FUNC_MAPPING = new ImmutableMap.Builder() .put("max", BuiltinFunctionName.MAX) .put("min", BuiltinFunctionName.MIN) diff --git a/core/src/testFixtures/java/org/opensearch/sql/executor/DefaultExecutionEngine.java b/core/src/testFixtures/java/org/opensearch/sql/executor/DefaultExecutionEngine.java index e4f9a185a3..ddf528887c 100644 --- a/core/src/testFixtures/java/org/opensearch/sql/executor/DefaultExecutionEngine.java +++ b/core/src/testFixtures/java/org/opensearch/sql/executor/DefaultExecutionEngine.java @@ -32,7 +32,7 @@ public void execute( while (plan.hasNext()) { result.add(plan.next()); } - QueryResponse response = new QueryResponse(new Schema(new ArrayList<>()), new ArrayList<>()); + QueryResponse response = new QueryResponse(new Schema(new ArrayList<>()), new ArrayList<>(), ""); listener.onResponse(response); } catch (Exception e) { listener.onFailure(e); diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java index cd951d4c77..caddc02ca3 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java @@ -32,7 +32,6 @@ import org.opensearch.sql.protocol.response.format.CsvResponseFormatter; import org.opensearch.sql.protocol.response.format.Format; import org.opensearch.sql.protocol.response.format.JdbcResponseFormatter; -import org.opensearch.sql.protocol.response.format.JsonTypeResponseFormatter; import org.opensearch.sql.protocol.response.format.JsonResponseFormatter; import org.opensearch.sql.protocol.response.format.RawResponseFormatter; import org.opensearch.sql.protocol.response.format.ResponseFormatter; @@ -167,7 +166,17 @@ private ResponseListener createQueryResponseListener( } else if (format.equals(Format.RAW)) { formatter = new RawResponseFormatter(); } else if (format.equals(Format.JSON)) { - formatter = new JsonTypeResponseFormatter(PRETTY); + return new ResponseListener<>() { + @Override + public void onResponse(QueryResponse response) { + sendResponse(channel, OK, response.getRawResponse()); + } + + @Override + public void onFailure(Exception e) { + errorHandler.accept(channel, e); + } + }; } else { formatter = new JdbcResponseFormatter(PRETTY); } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java index 9a136a3bec..c9d5b8fcaf 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java @@ -17,7 +17,9 @@ import org.opensearch.sql.executor.Explain; import org.opensearch.sql.opensearch.client.OpenSearchClient; import org.opensearch.sql.opensearch.executor.protector.ExecutionProtector; +import org.opensearch.sql.opensearch.storage.OpenSearchIndexScan; import org.opensearch.sql.planner.physical.PhysicalPlan; +import org.opensearch.sql.planner.physical.ProjectOperator; import org.opensearch.sql.storage.TableScanOperator; /** OpenSearch execution engine implementation. */ @@ -49,7 +51,14 @@ public void execute(PhysicalPlan physicalPlan, ExecutionContext context, result.add(plan.next()); } - QueryResponse response = new QueryResponse(physicalPlan.schema(), result); + String rawResponse; + try { + rawResponse = ((OpenSearchIndexScan) ((ProjectOperator) physicalPlan).getInput()) + .getRawResponse(); + } catch (ClassCastException e) { + rawResponse = ""; + } + QueryResponse response = new QueryResponse(physicalPlan.schema(), result, rawResponse); listener.onResponse(response); } catch (Exception e) { listener.onFailure(e); diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java index aadd73efdd..0c7e707d22 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java @@ -12,6 +12,7 @@ import java.util.Map; import java.util.stream.Collectors; import lombok.EqualsAndHashCode; +import lombok.Getter; import lombok.ToString; import org.opensearch.action.search.SearchResponse; import org.opensearch.search.SearchHits; @@ -43,6 +44,9 @@ public class OpenSearchResponse implements Iterable { */ @EqualsAndHashCode.Exclude private final OpenSearchExprValueFactory exprValueFactory; + @EqualsAndHashCode.Exclude + @Getter + private String rawResponse; /** * Constructor of ElasticsearchResponse. @@ -52,6 +56,7 @@ public OpenSearchResponse(SearchResponse searchResponse, this.hits = searchResponse.getHits(); this.aggregations = searchResponse.getAggregations(); this.exprValueFactory = exprValueFactory; + this.rawResponse = searchResponse.toString(); } /** diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java index e9746e1fae..20e05f20ba 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/OpenSearchIndexScan.java @@ -52,6 +52,9 @@ public class OpenSearchIndexScan extends TableScanOperator { /** Search response for current batch. */ private Iterator iterator; + @Getter + private String rawResponse; + /** * Constructor. */ @@ -80,7 +83,7 @@ public void open() { request = requestBuilder.build(); iterator = Collections.emptyIterator(); queryCount = 0; - fetchNextBatch(); + rawResponse = fetchNextBatch().getRawResponse(); } @Override @@ -99,11 +102,12 @@ public ExprValue next() { return iterator.next(); } - private void fetchNextBatch() { + private OpenSearchResponse fetchNextBatch() { OpenSearchResponse response = client.search(request); if (!response.isEmpty()) { iterator = response.iterator(); } + return response; } @Override diff --git a/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonTypeResponseFormatter.java b/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonTypeResponseFormatter.java deleted file mode 100644 index 51664b8bb8..0000000000 --- a/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JsonTypeResponseFormatter.java +++ /dev/null @@ -1,186 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - - -package org.opensearch.sql.protocol.response.format; - -import static org.opensearch.sql.expression.function.BuiltinFunctionName.AGGREGATION_FUNC_MAPPING; - -import com.google.gson.annotations.SerializedName; -import java.util.Arrays; -import java.util.HashMap; -import java.util.LinkedHashMap; -import java.util.LinkedList; -import java.util.List; -import java.util.Map; -import java.util.stream.Collectors; -import lombok.Builder; -import lombok.Getter; -import lombok.RequiredArgsConstructor; -import lombok.Singular; -import org.opensearch.sql.common.antlr.SyntaxCheckException; -import org.opensearch.sql.exception.QueryEngineException; -import org.opensearch.sql.opensearch.response.error.ErrorMessage; -import org.opensearch.sql.opensearch.response.error.ErrorMessageFactory; -import org.opensearch.sql.protocol.response.QueryResult; - -/** - * Response formatter to format response to json format. - */ -public class JsonTypeResponseFormatter extends JsonResponseFormatter { - - private final Style style; - - public JsonTypeResponseFormatter(Style style) { - super(style); - this.style = style; - } - - @Override - public Object buildJsonObject(QueryResult response) { - JsonResponse.JsonResponseBuilder json = JsonResponse.builder(); - Total total = new Total(response.size(), "eq"); - Shards shards = new Shards(0, 0, 0, 0); - List columnNames = new LinkedList<>(response.columnNameTypes().keySet()); - List hitList = new LinkedList<>(); - Map aggregations = new LinkedHashMap<>(); - - for (Object[] values : getRows(response)) { - List valueList = new LinkedList<>(Arrays.asList(values)); - Map source = combineListsIntoMap(columnNames, valueList); - - Map filteredSource = filterSourceFromFunctions(source); - Map functions = getFunctions(source); - Map aggregationMap = getAggregations(source); - aggregations.putAll(aggregationMap); - - Hit hit = new Hit(filteredSource, functions.isEmpty() ? null : functions); - - hitList.add(hit); - } - Hits hits = new Hits(total, hitList, aggregations.isEmpty() ? null : aggregations); - - json.shards(shards).hits(hits); - - return json.build(); - } - - @Override - public String format(Throwable t) { - int status = getStatus(t); - ErrorMessage message = ErrorMessageFactory.createErrorMessage(t, status); - JdbcResponseFormatter.Error error = new JdbcResponseFormatter.Error( - message.getType(), - message.getReason(), - message.getDetails()); - return jsonify(new JsonTypeErrorResponse(error, status)); - } - - private int getStatus(Throwable t) { - return (t instanceof SyntaxCheckException - || t instanceof QueryEngineException) ? 400 : 503; - } - - private Map getAggregations(Map source) { - return source.entrySet() - .stream().filter(entry -> isAggregationFunction(entry.getKey())) - .collect(Collectors.toMap(Map.Entry::getKey, - stringObjectEntry -> new Aggregation(stringObjectEntry.getValue()))); - } - - private Map getFunctions(Map source) { - return source.entrySet() - .stream().filter(entry -> isFunctionName(entry.getKey()) - && !isAggregationFunction(entry.getKey())) - .collect(HashMap::new, (map, values) -> - map.put(values.getKey(), values.getValue()), HashMap::putAll); - } - - private Map filterSourceFromFunctions(Map source) { - return source.entrySet() - .stream().filter(entry -> !isFunctionName(entry.getKey())) - .collect(HashMap::new, (map, values) -> - map.put(values.getKey(), values.getValue()), HashMap::putAll); - } - - private Map combineListsIntoMap(List keys, List values) { - Map map = new LinkedHashMap<>(); - for (int i = 0; i < keys.size(); i++) { - map.put(keys.get(i), values.get(i)); - } - return map; - } - - private List getRows(QueryResult response) { - List rows = new LinkedList<>(); - for (Object[] values : response) { - rows.add(values); - } - return rows; - } - - private boolean isAggregationFunction(String value) { - return isFunctionName(value) - && AGGREGATION_FUNC_MAPPING.containsKey(value.split("[(]")[0].toLowerCase()); - } - - private boolean isFunctionName(String value) { - return value.contains("(") && value.contains(")"); - } - - @Builder - @Getter - public static class JsonResponse { - @SerializedName("_shards") - public final Shards shards; - public final Hits hits; - } - - @RequiredArgsConstructor - @Getter - public static class Hits { - public final Total total; - @Singular("column") - public final List hits; - public final Map aggregations; - } - - @RequiredArgsConstructor - @Getter - public static class Hit { - @SerializedName("_source") - public final Map source; - public final Map fields; - } - - @RequiredArgsConstructor - @Getter - public static class Shards { - public final int total; - public final int successful; - public final int skipped; - public final int failed; - } - - @RequiredArgsConstructor - @Getter - public static class Total { - public final int value; - public final String relation; - } - - @RequiredArgsConstructor - @Getter - public static class Aggregation { - public final Object value; - } - - @RequiredArgsConstructor - @Getter - public static class JsonTypeErrorResponse { - private final JdbcResponseFormatter.Error error; - private final int status; - } -} diff --git a/protocol/src/test/java/org/opensearch/sql/protocol/response/format/JsonTypeResponseFormatterTest.java b/protocol/src/test/java/org/opensearch/sql/protocol/response/format/JsonTypeResponseFormatterTest.java deleted file mode 100644 index 009867cbd1..0000000000 --- a/protocol/src/test/java/org/opensearch/sql/protocol/response/format/JsonTypeResponseFormatterTest.java +++ /dev/null @@ -1,186 +0,0 @@ -package org.opensearch.sql.protocol.response.format; - -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.opensearch.sql.data.model.ExprValueUtils.LITERAL_MISSING; -import static org.opensearch.sql.data.model.ExprValueUtils.LITERAL_NULL; -import static org.opensearch.sql.data.model.ExprValueUtils.stringValue; -import static org.opensearch.sql.data.model.ExprValueUtils.tupleValue; -import static org.opensearch.sql.data.type.ExprCoreType.ARRAY; -import static org.opensearch.sql.data.type.ExprCoreType.INTEGER; -import static org.opensearch.sql.data.type.ExprCoreType.STRING; -import static org.opensearch.sql.data.type.ExprCoreType.STRUCT; -import static org.opensearch.sql.opensearch.data.type.OpenSearchDataType.OPENSEARCH_TEXT; -import static org.opensearch.sql.opensearch.data.type.OpenSearchDataType.OPENSEARCH_TEXT_KEYWORD; -import static org.opensearch.sql.protocol.response.format.JsonResponseFormatter.Style.COMPACT; - -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import com.google.gson.JsonParser; -import java.util.Arrays; -import org.junit.jupiter.api.DisplayNameGeneration; -import org.junit.jupiter.api.DisplayNameGenerator; -import org.junit.jupiter.api.Test; -import org.opensearch.OpenSearchException; -import org.opensearch.sql.common.antlr.SyntaxCheckException; -import org.opensearch.sql.data.model.ExprTupleValue; -import org.opensearch.sql.exception.SemanticCheckException; -import org.opensearch.sql.executor.ExecutionEngine; -import org.opensearch.sql.protocol.response.QueryResult; - -@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) -public class JsonTypeResponseFormatterTest { - - private final JsonTypeResponseFormatter formatter = new JsonTypeResponseFormatter(COMPACT); - - @Test - void format_response() { - QueryResult response = new QueryResult( - new ExecutionEngine.Schema(ImmutableList.of( - new ExecutionEngine.Schema.Column("name", "name", STRING), - new ExecutionEngine.Schema.Column("address1", "address1", OPENSEARCH_TEXT), - new ExecutionEngine.Schema.Column("address2", "address2", OPENSEARCH_TEXT_KEYWORD), - new ExecutionEngine.Schema.Column("location", "location", STRUCT), - new ExecutionEngine.Schema.Column("employer", "employer", ARRAY), - new ExecutionEngine.Schema.Column("age", "age", INTEGER))), - ImmutableList.of( - tupleValue(ImmutableMap.builder() - .put("name", "John") - .put("address1", "Seattle") - .put("address2", "WA") - .put("location", ImmutableMap.of("x", "1", "y", "2")) - .put("employments", ImmutableList.of( - ImmutableMap.of("name", "Amazon"), - ImmutableMap.of("name", "AWS"))) - .put("age", 20) - .build()))); - - assertJsonEquals( - "{\n" - + " \"_shards\":{\n" - + " \"total\":0,\n" - + " \"successful\":0,\n" - + " \"skipped\":0,\n" - + " \"failed\":0\n" + " },\n" - + " \"hits\":{\n" - + " \"total\":{\n" - + " \"value\":1,\n" - + " \"relation\":\"eq\"\n" - + " },\n" - + " \"hits\":[\n" - + " {\n" - + " \"_source\":{\n" - + " \"address2\":\"WA\",\n" - + " \"address1\":\"Seattle\",\n" - + " \"name\":\"John\",\n" - + " \"employer\":[\n" - + " {\n" - + " \"name\":\"Amazon\"\n" - + " },\n" - + " {\n" - + " \"name\":\"AWS\"\n" - + " }\n" - + " ],\n" - + " \"location\":{\n" - + " \"x\":\"1\",\n" - + " \"y\":\"2\"\n" - + " },\n" - + " \"age\":20\n" - + " }\n" - + " }\n" - + " ]\n" - + " }\n" - + "}", - formatter.format(response)); - } - - @Test - void format_response_with_missing_and_null_value() { - QueryResult response = - new QueryResult( - new ExecutionEngine.Schema(ImmutableList.of( - new ExecutionEngine.Schema.Column("name", null, STRING), - new ExecutionEngine.Schema.Column("age", null, INTEGER))), - Arrays.asList( - ExprTupleValue.fromExprValueMap( - ImmutableMap.of("name", stringValue("John"), "age", LITERAL_MISSING)), - ExprTupleValue.fromExprValueMap( - ImmutableMap.of("name", stringValue("Allen"), "age", LITERAL_NULL)), - tupleValue(ImmutableMap.of("name", "Smith", "age", 30)))); - - assertEquals( - "{\"_shards\":{\"total\":0,\"successful\":0,\"skipped\":0,\"failed\":0}," - + "\"hits\":{\"total\":{\"value\":3,\"relation\":\"eq\"}," - + "\"hits\":[{\"_source\":{\"name\":\"John\"}},{\"_source\":{\"name\":\"Allen\"}}," - + "{\"_source\":{\"name\":\"Smith\",\"age\":30}}]}}", - formatter.format(response)); - } - - @Test - void format_client_error_response_due_to_syntax_exception() { - assertJsonEquals( - "{\"error\":" - + "{\"" - + "type\":\"SyntaxCheckException\"," - + "\"reason\":\"Invalid Query\"," - + "\"details\":\"Invalid query syntax\"" - + "}," - + "\"status\":400}", - formatter.format(new SyntaxCheckException("Invalid query syntax")) - ); - } - - @Test - void format_client_error_response_due_to_semantic_exception() { - assertJsonEquals( - "{\"error\":" - + "{\"" - + "type\":\"SemanticCheckException\"," - + "\"reason\":\"Invalid Query\"," - + "\"details\":\"Invalid query semantics\"" - + "}," - + "\"status\":400}", - formatter.format(new SemanticCheckException("Invalid query semantics")) - ); - } - - @Test - void format_server_error_response() { - assertJsonEquals( - "{\"error\":" - + "{\"" - + "type\":\"IllegalStateException\"," - + "\"reason\":\"There was internal problem at backend\"," - + "\"details\":\"Execution error\"" - + "}," - + "\"status\":503}", - formatter.format(new IllegalStateException("Execution error")) - ); - } - - @Test - void format_server_error_response_due_to_opensearch() { - assertJsonEquals( - "{\"error\":" - + "{\"" - + "type\":\"OpenSearchException\"," - + "\"reason\":\"Error occurred in OpenSearch engine: all shards failed\"," - + "\"details\":\"OpenSearchException[all shards failed]; " - + "nested: IllegalStateException[Execution error];; " - + "java.lang.IllegalStateException: Execution error\\n" - + "For more details, please send request for Json format to see the raw response " - + "from OpenSearch engine.\"" - + "}," - + "\"status\":503}", - formatter.format(new OpenSearchException("all shards failed", - new IllegalStateException("Execution error"))) - ); - } - - private static void assertJsonEquals(String expected, String actual) { - assertEquals( - JsonParser.parseString(expected), - JsonParser.parseString(actual)); - } - -} - From 3f23122dfb47115ffe5cbc2dfe837cb64489f12e Mon Sep 17 00:00:00 2001 From: Margarit Hakobyan Date: Fri, 10 Feb 2023 10:40:06 -0800 Subject: [PATCH 07/38] Add an integration test, fix checkstyle errors Signed-off-by: Margarit Hakobyan --- .../sql/executor/QueryServiceTest.java | 2 +- .../MicroBatchStreamingExecutionTest.java | 2 +- .../sql/sql/JsonResponseTypeIT.java | 94 +++++++++++++++++++ .../opensearch/sql/ppl/PPLServiceTest.java | 6 +- .../opensearch/sql/sql/SQLServiceTest.java | 4 +- 5 files changed, 101 insertions(+), 7 deletions(-) create mode 100644 integ-test/src/test/java/org/opensearch/sql/sql/JsonResponseTypeIT.java diff --git a/core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.java b/core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.java index 4df38027f4..b1d882cf7f 100644 --- a/core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.java +++ b/core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.java @@ -134,7 +134,7 @@ Helper executeSuccess(Split split) { invocation -> { ResponseListener listener = invocation.getArgument(2); listener.onResponse( - new ExecutionEngine.QueryResponse(schema, Collections.emptyList())); + new ExecutionEngine.QueryResponse(schema, Collections.emptyList(), "")); return null; }) .when(executionEngine) diff --git a/core/src/test/java/org/opensearch/sql/executor/streaming/MicroBatchStreamingExecutionTest.java b/core/src/test/java/org/opensearch/sql/executor/streaming/MicroBatchStreamingExecutionTest.java index 1a2b6e3f2a..c0b611fb44 100644 --- a/core/src/test/java/org/opensearch/sql/executor/streaming/MicroBatchStreamingExecutionTest.java +++ b/core/src/test/java/org/opensearch/sql/executor/streaming/MicroBatchStreamingExecutionTest.java @@ -169,7 +169,7 @@ Helper executeSuccess(Long... offsets) { ResponseListener listener = invocation.getArgument(2); listener.onResponse( - new ExecutionEngine.QueryResponse(null, Collections.emptyList())); + new ExecutionEngine.QueryResponse(null, Collections.emptyList(), "")); PlanContext planContext = invocation.getArgument(1); assertTrue(planContext.getSplit().isPresent()); diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/JsonResponseTypeIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/JsonResponseTypeIT.java new file mode 100644 index 0000000000..f1b647191f --- /dev/null +++ b/integ-test/src/test/java/org/opensearch/sql/sql/JsonResponseTypeIT.java @@ -0,0 +1,94 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.sql; + +import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_CALCS; +import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_DOG; +import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_PEOPLE2; + +import org.json.JSONArray; +import org.json.JSONObject; +import org.junit.Assert; +import org.junit.Test; +import org.opensearch.sql.legacy.SQLIntegTestCase; + +public class JsonResponseTypeIT extends SQLIntegTestCase { + + @Override + protected void init() throws Exception { + loadIndex(Index.PEOPLE2); + loadIndex(Index.DOG); + loadIndex(Index.CALCS); + } + + // TODO: Verify JSON type response when using String and Date Functions and add test cases + // For example: 'select concat(str1, '!!!!!!!!') from calcs' might not be handled as expected. + // Also, handling of the date/time functions (for example: 'select now() from calcs) will need attention as well. + + @Test + public void selectTest() { + String responseAsString = + executeQuery(String.format("SELECT * FROM %s", TEST_INDEX_PEOPLE2), "json"); + JSONObject response = new JSONObject(responseAsString); + Assert.assertTrue(response.has("hits")); + Assert.assertTrue(response.has("_shards")); + Assert.assertTrue(response.has("took")); + Assert.assertTrue(response.has("timed_out")); + Assert.assertEquals(12, getTotalHits(response)); + } + + @Test + public void aggregationTest() { + final String aggregationsObjName = "aggregations"; + String responseAsString = + executeQuery(String.format("SELECT count(*), sum(age) FROM %s", TEST_INDEX_PEOPLE2), "json"); + JSONObject response = new JSONObject(responseAsString); + Assert.assertTrue(response.has(aggregationsObjName)); + Assert.assertEquals(12, + response.getJSONObject(aggregationsObjName).getJSONObject("count(*)").getInt("value")); + Assert.assertEquals(400, + response.getJSONObject(aggregationsObjName).getJSONObject("sum(age)").getInt("value")); + } + + @Test + public void groupByTest() { + String responseAsString = + executeQuery(String.format("SELECT count(*) FROM %s group by age", TEST_INDEX_DOG), "json"); + Assert.assertTrue(responseAsString.contains("\"aggregations\":{\"composite_buckets\":{\"after_key\":{\"age\":4}," + + "\"buckets\":[{\"key\":{\"age\":2},\"doc_count\":1,\"count(*)\":{\"value\":1}},{\"key\":{\"age\":4}," + + "\"doc_count\":1,\"count(*)\":{\"value\":1}}]}}")); + } + + @Test + public void selectWithoutWhereTest() { + String responseAsString = + executeQuery(String.format("SELECT count(*) FROM %s group by age", TEST_INDEX_DOG), "json"); + + Assert.assertTrue(responseAsString.contains("\"aggregations\":{\"composite_buckets\":{\"after_key\":{\"age\":4}," + + "\"buckets\":[{\"key\":{\"age\":2},\"doc_count\":1,\"count(*)\":{\"value\":1}},{\"key\":{\"age\":4}," + + "\"doc_count\":1,\"count(*)\":{\"value\":1}}]}}")); + } + + @Test + public void selectFromTwoDocumentsTest() { + // This query will fall back to V1, as V2 doesn't support Joins yet. + String responseAsString = + executeQuery(String.format("SELECT d.age, c.int0 FROM %s c JOIN %s d WHERE c.int1 > 2", + TEST_INDEX_CALCS, TEST_INDEX_DOG), "json"); + + JSONObject response = new JSONObject(responseAsString); + Assert.assertTrue(response.has("hits")); + + JSONArray hits = response.getJSONObject("hits").getJSONArray("hits"); + Assert.assertTrue(hits.length() > 0); + for (int i = 0; i < hits.length(); i++) { + JSONObject jsonObject = hits.getJSONObject(i); + Assert.assertTrue(jsonObject.getJSONObject("_source").keySet().contains("d.age")); + Assert.assertTrue(jsonObject.getJSONObject("_source").keySet().contains("c.int0")); + } + } + } + diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/PPLServiceTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/PPLServiceTest.java index 178335a126..b975ed88bb 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/PPLServiceTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/PPLServiceTest.java @@ -65,7 +65,7 @@ public void cleanup() throws InterruptedException { public void testExecuteShouldPass() { doAnswer(invocation -> { ResponseListener listener = invocation.getArgument(1); - listener.onResponse(new QueryResponse(schema, Collections.emptyList())); + listener.onResponse(new QueryResponse(schema, Collections.emptyList(), "")); return null; }).when(queryService).execute(any(), any()); @@ -87,7 +87,7 @@ public void onFailure(Exception e) { public void testExecuteCsvFormatShouldPass() { doAnswer(invocation -> { ResponseListener listener = invocation.getArgument(1); - listener.onResponse(new QueryResponse(schema, Collections.emptyList())); + listener.onResponse(new QueryResponse(schema, Collections.emptyList(), "")); return null; }).when(queryService).execute(any(), any()); @@ -161,7 +161,7 @@ public void onFailure(Exception e) { public void testPrometheusQuery() { doAnswer(invocation -> { ResponseListener listener = invocation.getArgument(1); - listener.onResponse(new QueryResponse(schema, Collections.emptyList())); + listener.onResponse(new QueryResponse(schema, Collections.emptyList(), "")); return null; }).when(queryService).execute(any(), any()); diff --git a/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java b/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java index a351c30609..ccb8363d0b 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java @@ -64,7 +64,7 @@ public void cleanup() throws InterruptedException { public void canExecuteSqlQuery() { doAnswer(invocation -> { ResponseListener listener = invocation.getArgument(1); - listener.onResponse(new QueryResponse(schema, Collections.emptyList())); + listener.onResponse(new QueryResponse(schema, Collections.emptyList(), "")); return null; }).when(queryService).execute(any(), any()); @@ -87,7 +87,7 @@ public void onFailure(Exception e) { public void canExecuteCsvFormatRequest() { doAnswer(invocation -> { ResponseListener listener = invocation.getArgument(1); - listener.onResponse(new QueryResponse(schema, Collections.emptyList())); + listener.onResponse(new QueryResponse(schema, Collections.emptyList(), "")); return null; }).when(queryService).execute(any(), any()); From 1365ae0e708ba859bedab71d2881b61e52ea8a94 Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Wed, 22 Feb 2023 09:20:58 -0800 Subject: [PATCH 08/38] Made new engine fallback to legacy for in memory operations for json format Signed-off-by: Guian Gumpac --- .../sql/legacy/plugin/RestSQLQueryAction.java | 2 +- .../java/org/opensearch/sql/sql/SQLService.java | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java index caddc02ca3..818c1b5e70 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java @@ -126,7 +126,7 @@ public void onResponse(T response) { @Override public void onFailure(Exception e) { - if (e instanceof SyntaxCheckException) { + if (e instanceof SyntaxCheckException || e instanceof UnsupportedOperationException) { fallBackHandler.accept(channel, e); } else { next.onFailure(e); diff --git a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java index 082a3e9581..6da2169af1 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java +++ b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java @@ -6,10 +6,15 @@ package org.opensearch.sql.sql; +import java.util.List; import java.util.Optional; import lombok.RequiredArgsConstructor; import org.antlr.v4.runtime.tree.ParseTree; +import org.opensearch.sql.ast.expression.Alias; +import org.opensearch.sql.ast.expression.Function; +import org.opensearch.sql.ast.statement.Query; import org.opensearch.sql.ast.statement.Statement; +import org.opensearch.sql.ast.tree.Project; import org.opensearch.sql.common.response.ResponseListener; import org.opensearch.sql.executor.ExecutionEngine.ExplainResponse; import org.opensearch.sql.executor.ExecutionEngine.QueryResponse; @@ -75,6 +80,17 @@ private AbstractPlan plan( .isExplain(request.isExplainRequest()) .build())); + // There is no full support for JSON format yet for in memory operations + if (request.format().getFormatName().equals("json")) { + List projectList = ((Project) ((Query) statement).getPlan()).getProjectList(); + + for (var project : projectList) { + if (((Alias) project).getDelegated() instanceof Function) + throw new UnsupportedOperationException("[%s] is not yet supported with json " + + "format because it is not pushed down to the OpenSearch instance"); + } + } + return queryExecutionFactory.create(statement, queryListener, explainListener); } } From 8e43ba67ee568becdeb92031db29978a6448ca4b Mon Sep 17 00:00:00 2001 From: MaxKsyunz Date: Wed, 22 Feb 2023 00:48:31 -0800 Subject: [PATCH 09/38] Address build failures Signed-off-by: MaxKsyunz --- .../org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java index 6249cfba6c..27134d6652 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java @@ -20,9 +20,9 @@ import org.opensearch.common.Strings; import org.opensearch.common.settings.Settings; import org.opensearch.common.xcontent.XContentFactory; -import org.opensearch.common.xcontent.XContentType; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.common.xcontent.XContentType; import org.opensearch.rest.BaseRestHandler; import org.opensearch.rest.BytesRestResponse; import org.opensearch.rest.RestController; From 799a4ce35e2c64bfb6193e289d059e499ac1a059 Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Mon, 27 Feb 2023 14:17:21 -0800 Subject: [PATCH 10/38] Added legacy fall back Signed-off-by: Guian Gumpac --- .../org/opensearch/sql/sql/SQLService.java | 46 ++++- .../sql/sql/antlr/SQLSyntaxParser.java | 5 + .../opensearch/sql/sql/SQLServiceTest.java | 160 ++++++++++++++++++ 3 files changed, 207 insertions(+), 4 deletions(-) diff --git a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java index 6da2169af1..697776663a 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java +++ b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java @@ -11,9 +11,14 @@ import lombok.RequiredArgsConstructor; import org.antlr.v4.runtime.tree.ParseTree; import org.opensearch.sql.ast.expression.Alias; +import org.opensearch.sql.ast.expression.Cast; import org.opensearch.sql.ast.expression.Function; +import org.opensearch.sql.ast.expression.Literal; import org.opensearch.sql.ast.statement.Query; import org.opensearch.sql.ast.statement.Statement; +import org.opensearch.sql.ast.tree.Aggregation; +import org.opensearch.sql.ast.tree.Filter; +import org.opensearch.sql.ast.tree.Limit; import org.opensearch.sql.ast.tree.Project; import org.opensearch.sql.common.response.ResponseListener; import org.opensearch.sql.executor.ExecutionEngine.ExplainResponse; @@ -70,6 +75,11 @@ private AbstractPlan plan( SQLQueryRequest request, Optional> queryListener, Optional> explainListener) { + // Hints are currently unsupported for V2 + ParseTree cstHints = parser.parseHints(request.getQuery()); + if (cstHints.getChildCount() > 1) + throw new UnsupportedOperationException("Hints are not yet supported in the new engine."); + // 1.Parse query and convert parse tree (CST) to abstract syntax tree (AST) ParseTree cst = parser.parse(request.getQuery()); Statement statement = @@ -80,14 +90,42 @@ private AbstractPlan plan( .isExplain(request.isExplainRequest()) .build())); - // There is no full support for JSON format yet for in memory operations + // There is no full support for JSON format yet for in memory operations, aliases, literals, and casts + // Aggregation has differences with legacy results if (request.format().getFormatName().equals("json")) { List projectList = ((Project) ((Query) statement).getPlan()).getProjectList(); + List planChildren = ((Project) ((Query) statement).getPlan()).getChild(); + UnsupportedOperationException aggException = new UnsupportedOperationException( + "Queries with aggregation are not yet supported with json format in the new engine"); + + for (var child : planChildren) { + if (child instanceof Aggregation && ((Aggregation) child).getGroupExprList().size() > 0) + throw aggException; + + if (child instanceof Filter) { + for (var filterChild : ((Filter) child).getChild()) + if (filterChild instanceof Aggregation) + throw aggException; + } + + if (child instanceof Limit) { + for (var filterChild : ((Limit) child).getChild()) + if(filterChild instanceof Aggregation) + throw aggException; + } + } for (var project : projectList) { - if (((Alias) project).getDelegated() instanceof Function) - throw new UnsupportedOperationException("[%s] is not yet supported with json " - + "format because it is not pushed down to the OpenSearch instance"); + if (project instanceof Alias) { + Alias projectAlias = ((Alias) project); + + if (projectAlias.getDelegated() instanceof Function || + projectAlias.getDelegated() instanceof Literal || + projectAlias.getDelegated() instanceof Cast || + (projectAlias.getAlias() != null && !projectAlias.getAlias().isEmpty())) + throw new UnsupportedOperationException("The query is not yet supported with json " + + "format because there is an expression in the query that is not pushed down to the OpenSearch instance"); + } } } diff --git a/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java b/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java index ee1e991bd4..73b2864152 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java +++ b/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java @@ -32,4 +32,9 @@ public ParseTree parse(String query) { return parser.root(); } + public ParseTree parseHints(String query) { + OpenSearchSQLLexer lexer = new OpenSearchSQLLexer(new CaseInsensitiveCharStream(query)); + OpenSearchSQLParser hintsParser = new OpenSearchSQLParser(new CommonTokenStream(lexer, OpenSearchSQLLexer.SQLCOMMENT)); + return hintsParser.root(); + } } diff --git a/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java b/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java index ccb8363d0b..711ef8a73d 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java @@ -7,6 +7,7 @@ package org.opensearch.sql.sql; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; @@ -83,6 +84,148 @@ public void onFailure(Exception e) { }); } + @Test + public void canExecuteJsonFormatRequest() { + doAnswer(invocation -> { + ResponseListener listener = invocation.getArgument(1); + listener.onResponse(new QueryResponse(schema, Collections.emptyList(), "")); + return null; + }).when(queryService).execute(any(), any()); + + sqlService.execute( + new SQLQueryRequest(new JSONObject(), "SELECT FIELD FROM TABLE", QUERY, "json"), + new ResponseListener() { + @Override + public void onResponse(QueryResponse response) { + assertNotNull(response); + } + + @Override + public void onFailure(Exception e) { + fail(e); + } + }); + } + + @Test + public void canThrowUnsupportedExceptionForAggregationJsonQuery() { + sqlService.execute( + new SQLQueryRequest(new JSONObject(), "SELECT FIELD FROM TABLE GROUP BY FIELD", QUERY, "json"), + new ResponseListener() { + @Override + public void onResponse(QueryResponse response) { + assertNotNull(response); + } + + @Override + public void onFailure(Exception e) { + assert(e instanceof UnsupportedOperationException); + } + }); + } + + @Test + public void canThrowUnsupportedExceptionForFilterAggregationJsonQuery() { + sqlService.execute( + new SQLQueryRequest(new JSONObject(), "SELECT * FROM TABLE GROUP BY FIELD HAVING COUNT(*) > 0", QUERY, "json"), + new ResponseListener() { + @Override + public void onResponse(QueryResponse response) { + assertNotNull(response); + } + + @Override + public void onFailure(Exception e) { + assert(e instanceof UnsupportedOperationException); + } + }); + } + + @Test + public void canThrowUnsupportedExceptionForLimitAggregationJsonQuery() { + sqlService.execute( + new SQLQueryRequest(new JSONObject(), "SELECT * FROM TABLE GROUP BY FIELD LIMIT 10", QUERY, "json"), + new ResponseListener() { + @Override + public void onResponse(QueryResponse response) { + assertNotNull(response); + } + + @Override + public void onFailure(Exception e) { + assert(e instanceof UnsupportedOperationException); + } + }); + } + + @Test + public void canThrowUnsupportedExceptionForAliasJsonQuery() { + sqlService.execute( + new SQLQueryRequest(new JSONObject(), "SELECT FIELD as FIELD FROM TABLE", QUERY, "json"), + new ResponseListener() { + @Override + public void onResponse(QueryResponse response) { + assertNotNull(response); + } + + @Override + public void onFailure(Exception e) { + assert(e instanceof UnsupportedOperationException); + } + }); + } + + @Test + public void canThrowUnsupportedExceptionForFunctionJsonQuery() { + sqlService.execute( + new SQLQueryRequest(new JSONObject(), "SELECT 1 + 1", QUERY, "json"), + new ResponseListener() { + @Override + public void onResponse(QueryResponse response) { + assertNotNull(response); + } + + @Override + public void onFailure(Exception e) { + assert(e instanceof UnsupportedOperationException); + } + }); + } + + @Test + public void canThrowUnsupportedExceptionForLiteralJsonQuery() { + sqlService.execute( + new SQLQueryRequest(new JSONObject(), "SELECT 123", QUERY, "json"), + new ResponseListener() { + @Override + public void onResponse(QueryResponse response) { + assertNotNull(response); + } + + @Override + public void onFailure(Exception e) { + assert(e instanceof UnsupportedOperationException); + } + }); + } + + @Test + public void canThrowUnsupportedExceptionForCastJsonQuery() { + sqlService.execute( + new SQLQueryRequest(new JSONObject(), "SELECT CAST(FIELD AS DOUBLE)", QUERY, "json"), + new ResponseListener() { + @Override + public void onResponse(QueryResponse response) { + assertNotNull(response); + } + + @Override + public void onFailure(Exception e) { + assert(e instanceof UnsupportedOperationException); + } + }); + } + @Test public void canExecuteCsvFormatRequest() { doAnswer(invocation -> { @@ -106,6 +249,23 @@ public void onFailure(Exception e) { }); } + @Test + public void canThrowUnsupportedExceptionForHintsQuery() { + sqlService.execute( + new SQLQueryRequest(new JSONObject(), "SELECT /*! HINTS */ 123", QUERY, "jdbc"), + new ResponseListener() { + @Override + public void onResponse(QueryResponse response) { + assertNotNull(response); + } + + @Override + public void onFailure(Exception e) { + assert(e instanceof UnsupportedOperationException); + } + }); + } + @Test public void canExplainSqlQuery() { doAnswer(invocation -> { From a82de603632b48183ab5c845cf2ad8cd83151ba2 Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Wed, 1 Mar 2023 14:03:01 -0800 Subject: [PATCH 11/38] Refactored fall back logic to use visitor design pattern Signed-off-by: Guian Gumpac --- .../org/opensearch/sql/sql/SQLService.java | 116 +++++++++++++----- .../sql/sql/antlr/SQLSyntaxParser.java | 4 +- 2 files changed, 85 insertions(+), 35 deletions(-) diff --git a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java index 697776663a..dde2fa416f 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java +++ b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java @@ -6,10 +6,15 @@ package org.opensearch.sql.sql; -import java.util.List; import java.util.Optional; + +import lombok.Getter; import lombok.RequiredArgsConstructor; +import lombok.Setter; import org.antlr.v4.runtime.tree.ParseTree; +import org.apache.commons.lang3.StringUtils; +import org.opensearch.sql.ast.AbstractNodeVisitor; +import org.opensearch.sql.ast.Node; import org.opensearch.sql.ast.expression.Alias; import org.opensearch.sql.ast.expression.Cast; import org.opensearch.sql.ast.expression.Function; @@ -17,8 +22,6 @@ import org.opensearch.sql.ast.statement.Query; import org.opensearch.sql.ast.statement.Statement; import org.opensearch.sql.ast.tree.Aggregation; -import org.opensearch.sql.ast.tree.Filter; -import org.opensearch.sql.ast.tree.Limit; import org.opensearch.sql.ast.tree.Project; import org.opensearch.sql.common.response.ResponseListener; import org.opensearch.sql.executor.ExecutionEngine.ExplainResponse; @@ -75,9 +78,7 @@ private AbstractPlan plan( SQLQueryRequest request, Optional> queryListener, Optional> explainListener) { - // Hints are currently unsupported for V2 - ParseTree cstHints = parser.parseHints(request.getQuery()); - if (cstHints.getChildCount() > 1) + if (parser.containsHints(request.getQuery())) throw new UnsupportedOperationException("Hints are not yet supported in the new engine."); // 1.Parse query and convert parse tree (CST) to abstract syntax tree (AST) @@ -93,39 +94,88 @@ private AbstractPlan plan( // There is no full support for JSON format yet for in memory operations, aliases, literals, and casts // Aggregation has differences with legacy results if (request.format().getFormatName().equals("json")) { - List projectList = ((Project) ((Query) statement).getPlan()).getProjectList(); - List planChildren = ((Project) ((Query) statement).getPlan()).getChild(); - UnsupportedOperationException aggException = new UnsupportedOperationException( - "Queries with aggregation are not yet supported with json format in the new engine"); - - for (var child : planChildren) { - if (child instanceof Aggregation && ((Aggregation) child).getGroupExprList().size() > 0) - throw aggException; - - if (child instanceof Filter) { - for (var filterChild : ((Filter) child).getChild()) - if (filterChild instanceof Aggregation) - throw aggException; + class VisitorContext { + @Getter + @Setter + boolean isJSONSupported = true; + } + + class JsonSupportedVisitor extends AbstractNodeVisitor { + @Override + public Boolean visit(Node node, VisitorContext context) { + // A node is supported if it's a leaf node or if all of its children are supported. + return node.getChild().isEmpty() + || node.getChild().stream().filter(c -> c.accept(this, context) != null) + .allMatch(c -> c.accept(this, context)); } - if (child instanceof Limit) { - for (var filterChild : ((Limit) child).getChild()) - if(filterChild instanceof Aggregation) - throw aggException; + @Override + public Boolean visitAlias(Alias node, VisitorContext context) { + // Alias node is accepted if it does not have a user-defined alias + // and if the delegated expression is accepted. + if (!StringUtils.isEmpty(node.getAlias())) + return false; + else + return node.getDelegated().accept(this, context); } - } - for (var project : projectList) { - if (project instanceof Alias) { - Alias projectAlias = ((Alias) project); + @Override + public Boolean visitProject(Project node, VisitorContext context) { + // Overridden visit functions are done in memory and are not supported with json format + class UnsupportedExpressionsVisitor + extends AbstractNodeVisitor { + + @Override + public Boolean visitFunction(Function node, Object context) { + // queries with function calls are not supported. + return false; + } + + @Override + public Boolean visitLiteral(Literal node, Object context) { + // queries with literal values are not supported + return false; + } + + @Override + public Boolean visitCast(Cast node, Object context) { + // Queries with cast are not supported + return false; + } + + @Override + public Boolean visitAlias(Alias node, Object context) { + // Alias node is accepted if it does not have a user-defined alias + // and if the delegated expression is accepted. + if (!StringUtils.isEmpty(node.getAlias()))// && node.getDelegated().accept(this, context)) + return false; + else { + return node.getDelegated().accept(this, context); + } + } + } + + // Project node is supported if all of its children are supported and + // all expressions in project list are supported. + UnsupportedExpressionsVisitor unsupportedExpressionsVisitor = new UnsupportedExpressionsVisitor(); + return visit(node, context) + && node.getProjectList().stream().filter(c -> c.accept(unsupportedExpressionsVisitor, context) != null) + .allMatch(e -> e.accept(unsupportedExpressionsVisitor, context)); - if (projectAlias.getDelegated() instanceof Function || - projectAlias.getDelegated() instanceof Literal || - projectAlias.getDelegated() instanceof Cast || - (projectAlias.getAlias() != null && !projectAlias.getAlias().isEmpty())) - throw new UnsupportedOperationException("The query is not yet supported with json " - + "format because there is an expression in the query that is not pushed down to the OpenSearch instance"); } + + @Override + public Boolean visitAggregation(Aggregation node, VisitorContext context) { + return node.getGroupExprList().isEmpty(); + } + } + + Boolean isJsonSupported = ((Query) statement).getPlan().accept(new JsonSupportedVisitor(), new VisitorContext()); + + if (!isJsonSupported) { + throw new UnsupportedOperationException( + "Queries with aggregation and in memory operations (cast, literals, alias, math, etc.)" + + " are not yet supported with json format in the new engine"); } } diff --git a/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java b/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java index 73b2864152..8ffba7d097 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java +++ b/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java @@ -32,9 +32,9 @@ public ParseTree parse(String query) { return parser.root(); } - public ParseTree parseHints(String query) { + public boolean containsHints(String query) { OpenSearchSQLLexer lexer = new OpenSearchSQLLexer(new CaseInsensitiveCharStream(query)); OpenSearchSQLParser hintsParser = new OpenSearchSQLParser(new CommonTokenStream(lexer, OpenSearchSQLLexer.SQLCOMMENT)); - return hintsParser.root(); + return hintsParser.root().getChildCount() > 1; } } From f11274f981cced3e7045edc9de4bb26476a84ca0 Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Wed, 1 Mar 2023 15:33:08 -0800 Subject: [PATCH 12/38] Added unit tests Signed-off-by: Guian Gumpac --- .../org/opensearch/sql/sql/SQLService.java | 15 ++------ .../opensearch/sql/sql/SQLServiceTest.java | 35 ------------------- .../sql/sql/antlr/SQLSyntaxParserTest.java | 10 +++++- .../sql/sql/domain/SQLQueryRequestTest.java | 9 +++++ 4 files changed, 20 insertions(+), 49 deletions(-) diff --git a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java index dde2fa416f..64b6bd4fd9 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java +++ b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java @@ -103,22 +103,11 @@ class VisitorContext { class JsonSupportedVisitor extends AbstractNodeVisitor { @Override public Boolean visit(Node node, VisitorContext context) { - // A node is supported if it's a leaf node or if all of its children are supported. - return node.getChild().isEmpty() - || node.getChild().stream().filter(c -> c.accept(this, context) != null) + // A node is supported if all of its children are supported. + return node.getChild().stream().filter(c -> c.accept(this, context) != null) .allMatch(c -> c.accept(this, context)); } - @Override - public Boolean visitAlias(Alias node, VisitorContext context) { - // Alias node is accepted if it does not have a user-defined alias - // and if the delegated expression is accepted. - if (!StringUtils.isEmpty(node.getAlias())) - return false; - else - return node.getDelegated().accept(this, context); - } - @Override public Boolean visitProject(Project node, VisitorContext context) { // Overridden visit functions are done in memory and are not supported with json format diff --git a/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java b/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java index 711ef8a73d..1dbedcea83 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java @@ -7,7 +7,6 @@ package org.opensearch.sql.sql; import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; @@ -124,40 +123,6 @@ public void onFailure(Exception e) { }); } - @Test - public void canThrowUnsupportedExceptionForFilterAggregationJsonQuery() { - sqlService.execute( - new SQLQueryRequest(new JSONObject(), "SELECT * FROM TABLE GROUP BY FIELD HAVING COUNT(*) > 0", QUERY, "json"), - new ResponseListener() { - @Override - public void onResponse(QueryResponse response) { - assertNotNull(response); - } - - @Override - public void onFailure(Exception e) { - assert(e instanceof UnsupportedOperationException); - } - }); - } - - @Test - public void canThrowUnsupportedExceptionForLimitAggregationJsonQuery() { - sqlService.execute( - new SQLQueryRequest(new JSONObject(), "SELECT * FROM TABLE GROUP BY FIELD LIMIT 10", QUERY, "json"), - new ResponseListener() { - @Override - public void onResponse(QueryResponse response) { - assertNotNull(response); - } - - @Override - public void onFailure(Exception e) { - assert(e instanceof UnsupportedOperationException); - } - }); - } - @Test public void canThrowUnsupportedExceptionForAliasJsonQuery() { sqlService.execute( diff --git a/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java b/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java index 0b8e64d0bb..007f8ca317 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java @@ -7,8 +7,10 @@ package org.opensearch.sql.sql.antlr; import static org.junit.jupiter.api.Assertions.assertAll; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Streams; @@ -238,7 +240,7 @@ public void can_parse_dayofmonth_functions() { assertNotNull(parser.parse("SELECT dayofmonth('2022-11-18')")); assertNotNull(parser.parse("SELECT day_of_month('2022-11-18')")); } - + @Test public void can_parse_day_of_week_functions() { assertNotNull(parser.parse("SELECT dayofweek('2022-11-18')")); @@ -633,6 +635,12 @@ public void canParseMultiMatchAlternateSyntax() { assertNotNull(parser.parse("SELECT * FROM test WHERE Field = multimatch(\"query\")")); } + @Test + public void containHintsTest() { + assertTrue(parser.containsHints("SELECT /*! HINTS */ field FROM test")); + assertFalse(parser.containsHints("SELECT field FROM test")); + } + private static Stream matchPhraseQueryComplexQueries() { return Stream.of( "SELECT * FROM t WHERE matchphrasequery(c, 3)", diff --git a/sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java b/sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java index 52a1f534e9..c206433280 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java @@ -142,6 +142,15 @@ public void shouldSupportRawFormat() { assertTrue(csvRequest.isSupported()); } + @Test + public void shouldSupportJsonFormat() { + SQLQueryRequest jsonRequest = + SQLQueryRequestBuilder.request("SELECT 1") + .format("json") + .build(); + assertTrue(jsonRequest.isSupported()); + } + /** * SQL query request build helper to improve test data setup readability. */ From 16af6eaf8156bf21cce1d6bdd24ea60b79a2db0b Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Wed, 1 Mar 2023 15:42:55 -0800 Subject: [PATCH 13/38] Revert "Address build failures" This reverts commit 17684b3f945a70017865c4a7db8f26e866c3b26a. --- .../test/java/org/opensearch/sql/legacy/JSONRequestIT.java | 4 ++-- .../test/java/org/opensearch/sql/legacy/SourceFieldIT.java | 4 ++-- .../main/java/org/opensearch/sql/legacy/domain/Paramer.java | 2 +- .../org/opensearch/sql/legacy/domain/hints/HintFactory.java | 2 +- .../sql/legacy/executor/GetIndexRequestRestListener.java | 4 ++-- .../sql/legacy/executor/join/ElasticJoinExecutor.java | 2 +- .../opensearch/sql/legacy/executor/join/ElasticUtils.java | 6 +++--- .../java/org/opensearch/sql/legacy/query/QueryAction.java | 2 +- .../sql/legacy/query/join/JoinRequestBuilder.java | 4 ++-- .../legacy/query/join/NestedLoopsElasticRequestBuilder.java | 4 ++-- .../org/opensearch/sql/legacy/query/maker/AggMaker.java | 4 ++-- .../java/org/opensearch/sql/legacy/query/maker/Maker.java | 6 +++--- .../sql/legacy/query/multi/MultiQueryRequestBuilder.java | 4 ++-- .../legacy/query/planner/physical/node/scroll/Scroll.java | 1 + .../java/org/opensearch/sql/legacy/request/SqlRequest.java | 4 ++-- .../opensearch/sql/legacy/utils/JsonPrettyFormatter.java | 6 +++--- .../src/main/java/org/opensearch/sql/plugin/SQLPlugin.java | 2 +- .../opensearch/sql/plugin/rest/RestQuerySettingsAction.java | 4 ++-- 18 files changed, 33 insertions(+), 32 deletions(-) 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 ec5972be78..5b7f9202bf 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 @@ -17,9 +17,9 @@ import org.junit.Test; import org.opensearch.action.search.SearchResponse; import org.opensearch.common.xcontent.LoggingDeprecationHandler; +import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.common.xcontent.XContentFactory; -import org.opensearch.core.xcontent.NamedXContentRegistry; -import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.XContentType; import org.opensearch.search.SearchHit; import org.opensearch.search.SearchHits; 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 756cbcd064..91c8321c95 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 @@ -15,9 +15,9 @@ import org.junit.Test; import org.opensearch.action.search.SearchResponse; import org.opensearch.common.xcontent.LoggingDeprecationHandler; +import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.common.xcontent.XContentFactory; -import org.opensearch.core.xcontent.NamedXContentRegistry; -import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.XContentType; import org.opensearch.search.SearchHit; import org.opensearch.search.SearchHits; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/domain/Paramer.java b/legacy/src/main/java/org/opensearch/sql/legacy/domain/Paramer.java index 5c2c84f9b8..b35ddcb0ed 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/domain/Paramer.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/domain/Paramer.java @@ -15,7 +15,7 @@ import java.util.List; import java.util.Map; import org.opensearch.common.Strings; -import org.opensearch.core.xcontent.ToXContent; +import org.opensearch.common.xcontent.ToXContent; import org.opensearch.index.query.MatchPhraseQueryBuilder; import org.opensearch.index.query.MatchQueryBuilder; import org.opensearch.index.query.MultiMatchQueryBuilder; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/domain/hints/HintFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/domain/hints/HintFactory.java index 18c68d57ab..59ecf2eba8 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/domain/hints/HintFactory.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/domain/hints/HintFactory.java @@ -14,8 +14,8 @@ import java.util.List; import java.util.Map; import org.opensearch.common.xcontent.LoggingDeprecationHandler; +import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.common.xcontent.yaml.YamlXContentParser; -import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.sql.legacy.exception.SqlParseException; /** diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/GetIndexRequestRestListener.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/GetIndexRequestRestListener.java index ebd9648d5e..b930044345 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/GetIndexRequestRestListener.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/GetIndexRequestRestListener.java @@ -17,8 +17,8 @@ import org.opensearch.cluster.metadata.MappingMetadata; import org.opensearch.common.collect.ImmutableOpenMap; import org.opensearch.common.settings.Settings; -import org.opensearch.core.xcontent.ToXContent; -import org.opensearch.core.xcontent.XContentBuilder; +import org.opensearch.common.xcontent.ToXContent; +import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.rest.BytesRestResponse; import org.opensearch.rest.RestChannel; import org.opensearch.rest.RestResponse; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticJoinExecutor.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticJoinExecutor.java index 3bbfcff1dd..811d444a7f 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticJoinExecutor.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticJoinExecutor.java @@ -23,7 +23,7 @@ import org.opensearch.common.document.DocumentField; import org.opensearch.common.text.Text; import org.opensearch.common.unit.TimeValue; -import org.opensearch.core.xcontent.XContentBuilder; +import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.index.mapper.MapperService; import org.opensearch.rest.BytesRestResponse; import org.opensearch.rest.RestChannel; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticUtils.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticUtils.java index 5a91184a09..c3b03c653a 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticUtils.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticUtils.java @@ -6,7 +6,7 @@ package org.opensearch.sql.legacy.executor.join; -import static org.opensearch.core.xcontent.ToXContent.EMPTY_PARAMS; +import static org.opensearch.common.xcontent.ToXContent.EMPTY_PARAMS; import com.google.common.collect.ImmutableMap; import java.io.IOException; @@ -20,10 +20,10 @@ import org.opensearch.common.bytes.BytesReference; import org.opensearch.common.io.stream.BytesStreamOutput; import org.opensearch.common.unit.TimeValue; +import org.opensearch.common.xcontent.ToXContent.Params; +import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.common.xcontent.XContentType; -import org.opensearch.core.xcontent.ToXContent.Params; -import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.search.SearchHit; import org.opensearch.search.SearchHits; import org.opensearch.search.sort.FieldSortBuilder; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/QueryAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/QueryAction.java index 7646639be4..46a9dda1d7 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/QueryAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/QueryAction.java @@ -16,8 +16,8 @@ import org.opensearch.action.support.IndicesOptions; import org.opensearch.client.Client; import org.opensearch.common.xcontent.LoggingDeprecationHandler; +import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.common.xcontent.json.JsonXContentParser; -import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.index.query.QueryBuilders; import org.opensearch.search.collapse.CollapseBuilder; import org.opensearch.search.fetch.subphase.highlight.HighlightBuilder; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/join/JoinRequestBuilder.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/join/JoinRequestBuilder.java index de6cc0cddd..b4ebbb2ad7 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/join/JoinRequestBuilder.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/join/JoinRequestBuilder.java @@ -13,10 +13,10 @@ import org.opensearch.action.ActionResponse; import org.opensearch.action.search.MultiSearchRequest; import org.opensearch.common.bytes.BytesReference; +import org.opensearch.common.xcontent.ToXContent; +import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.common.xcontent.XContentType; -import org.opensearch.core.xcontent.ToXContent; -import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.sql.legacy.query.SqlElasticRequestBuilder; /** diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/join/NestedLoopsElasticRequestBuilder.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/join/NestedLoopsElasticRequestBuilder.java index 6783441931..2e80c05ff2 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/join/NestedLoopsElasticRequestBuilder.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/join/NestedLoopsElasticRequestBuilder.java @@ -11,10 +11,10 @@ import org.json.JSONObject; import org.json.JSONStringer; import org.opensearch.common.bytes.BytesReference; +import org.opensearch.common.xcontent.ToXContent; +import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.common.xcontent.XContentType; -import org.opensearch.core.xcontent.ToXContent; -import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.sql.legacy.domain.Condition; import org.opensearch.sql.legacy.domain.Where; import org.opensearch.sql.legacy.exception.SqlParseException; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/AggMaker.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/AggMaker.java index c7ab574a38..87125721c0 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/AggMaker.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/AggMaker.java @@ -21,10 +21,10 @@ import org.opensearch.common.ParsingException; import org.opensearch.common.Strings; import org.opensearch.common.xcontent.LoggingDeprecationHandler; +import org.opensearch.common.xcontent.NamedXContentRegistry; +import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.json.JsonXContent; import org.opensearch.common.xcontent.json.JsonXContentParser; -import org.opensearch.core.xcontent.NamedXContentRegistry; -import org.opensearch.core.xcontent.XContentParser; import org.opensearch.geo.search.aggregations.bucket.geogrid.GeoHashGridAggregationBuilder; import org.opensearch.join.aggregations.JoinAggregationBuilders; import org.opensearch.script.Script; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/Maker.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/Maker.java index 409288503a..951d87a2c8 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/Maker.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/Maker.java @@ -31,10 +31,10 @@ import org.opensearch.common.geo.builders.ShapeBuilder; import org.opensearch.common.geo.parsers.ShapeParser; import org.opensearch.common.xcontent.LoggingDeprecationHandler; +import org.opensearch.common.xcontent.NamedXContentRegistry; +import org.opensearch.common.xcontent.ToXContent; +import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.json.JsonXContent; -import org.opensearch.core.xcontent.NamedXContentRegistry; -import org.opensearch.core.xcontent.ToXContent; -import org.opensearch.core.xcontent.XContentParser; import org.opensearch.index.query.BoolQueryBuilder; import org.opensearch.index.query.GeoPolygonQueryBuilder; import org.opensearch.index.query.MatchNoneQueryBuilder; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/multi/MultiQueryRequestBuilder.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/multi/MultiQueryRequestBuilder.java index 1b20fbe3fb..4b5448ac30 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/multi/MultiQueryRequestBuilder.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/multi/MultiQueryRequestBuilder.java @@ -16,10 +16,10 @@ import org.opensearch.action.ActionResponse; import org.opensearch.action.search.SearchRequestBuilder; import org.opensearch.common.bytes.BytesReference; +import org.opensearch.common.xcontent.ToXContent; +import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.common.xcontent.XContentType; -import org.opensearch.core.xcontent.ToXContent; -import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.sql.legacy.domain.Field; import org.opensearch.sql.legacy.domain.Select; import org.opensearch.sql.legacy.query.SqlElasticRequestBuilder; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/scroll/Scroll.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/scroll/Scroll.java index 6dd8ff6e89..b623e4c1f7 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/scroll/Scroll.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/scroll/Scroll.java @@ -15,6 +15,7 @@ import org.opensearch.client.Client; import org.opensearch.common.Strings; import org.opensearch.common.unit.TimeValue; +import org.opensearch.common.xcontent.MediaType; import org.opensearch.common.xcontent.XContentType; import org.opensearch.index.query.BoolQueryBuilder; import org.opensearch.index.query.QueryBuilder; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/request/SqlRequest.java b/legacy/src/main/java/org/opensearch/sql/legacy/request/SqlRequest.java index 605ef3c958..14acb26b3c 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/request/SqlRequest.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/request/SqlRequest.java @@ -12,10 +12,10 @@ import org.json.JSONObject; import org.opensearch.common.settings.Settings; import org.opensearch.common.xcontent.LoggingDeprecationHandler; +import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.common.xcontent.XContentFactory; +import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.XContentType; -import org.opensearch.core.xcontent.NamedXContentRegistry; -import org.opensearch.core.xcontent.XContentParser; import org.opensearch.index.query.BoolQueryBuilder; import org.opensearch.search.SearchModule; import org.opensearch.sql.legacy.exception.SqlParseException; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/utils/JsonPrettyFormatter.java b/legacy/src/main/java/org/opensearch/sql/legacy/utils/JsonPrettyFormatter.java index 24cc47b0c0..b94ef06c25 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/utils/JsonPrettyFormatter.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/utils/JsonPrettyFormatter.java @@ -9,11 +9,11 @@ import java.io.IOException; import org.opensearch.common.Strings; import org.opensearch.common.xcontent.LoggingDeprecationHandler; +import org.opensearch.common.xcontent.NamedXContentRegistry; +import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentFactory; +import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.XContentType; -import org.opensearch.core.xcontent.NamedXContentRegistry; -import org.opensearch.core.xcontent.XContentBuilder; -import org.opensearch.core.xcontent.XContentParser; /** * Utility Class for formatting Json string pretty. diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java b/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java index 2c401af352..98826c6f62 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java @@ -40,7 +40,7 @@ import org.opensearch.common.settings.Settings; import org.opensearch.common.settings.SettingsFilter; import org.opensearch.common.util.concurrent.OpenSearchExecutors; -import org.opensearch.core.xcontent.NamedXContentRegistry; +import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.env.Environment; import org.opensearch.env.NodeEnvironment; import org.opensearch.plugins.ActionPlugin; diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java index 27134d6652..59518e7a9d 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java @@ -19,9 +19,9 @@ import org.opensearch.client.node.NodeClient; import org.opensearch.common.Strings; import org.opensearch.common.settings.Settings; +import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentFactory; -import org.opensearch.core.xcontent.XContentBuilder; -import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.XContentType; import org.opensearch.rest.BaseRestHandler; import org.opensearch.rest.BytesRestResponse; From 7a782c7bcd511bdfc5293a93180e07ba963d8ae6 Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Wed, 1 Mar 2023 16:22:12 -0800 Subject: [PATCH 14/38] Removed unnecessary IT Signed-off-by: Guian Gumpac --- .../sql/sql/JsonResponseTypeIT.java | 94 ------------------- 1 file changed, 94 deletions(-) delete mode 100644 integ-test/src/test/java/org/opensearch/sql/sql/JsonResponseTypeIT.java diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/JsonResponseTypeIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/JsonResponseTypeIT.java deleted file mode 100644 index f1b647191f..0000000000 --- a/integ-test/src/test/java/org/opensearch/sql/sql/JsonResponseTypeIT.java +++ /dev/null @@ -1,94 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.sql; - -import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_CALCS; -import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_DOG; -import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_PEOPLE2; - -import org.json.JSONArray; -import org.json.JSONObject; -import org.junit.Assert; -import org.junit.Test; -import org.opensearch.sql.legacy.SQLIntegTestCase; - -public class JsonResponseTypeIT extends SQLIntegTestCase { - - @Override - protected void init() throws Exception { - loadIndex(Index.PEOPLE2); - loadIndex(Index.DOG); - loadIndex(Index.CALCS); - } - - // TODO: Verify JSON type response when using String and Date Functions and add test cases - // For example: 'select concat(str1, '!!!!!!!!') from calcs' might not be handled as expected. - // Also, handling of the date/time functions (for example: 'select now() from calcs) will need attention as well. - - @Test - public void selectTest() { - String responseAsString = - executeQuery(String.format("SELECT * FROM %s", TEST_INDEX_PEOPLE2), "json"); - JSONObject response = new JSONObject(responseAsString); - Assert.assertTrue(response.has("hits")); - Assert.assertTrue(response.has("_shards")); - Assert.assertTrue(response.has("took")); - Assert.assertTrue(response.has("timed_out")); - Assert.assertEquals(12, getTotalHits(response)); - } - - @Test - public void aggregationTest() { - final String aggregationsObjName = "aggregations"; - String responseAsString = - executeQuery(String.format("SELECT count(*), sum(age) FROM %s", TEST_INDEX_PEOPLE2), "json"); - JSONObject response = new JSONObject(responseAsString); - Assert.assertTrue(response.has(aggregationsObjName)); - Assert.assertEquals(12, - response.getJSONObject(aggregationsObjName).getJSONObject("count(*)").getInt("value")); - Assert.assertEquals(400, - response.getJSONObject(aggregationsObjName).getJSONObject("sum(age)").getInt("value")); - } - - @Test - public void groupByTest() { - String responseAsString = - executeQuery(String.format("SELECT count(*) FROM %s group by age", TEST_INDEX_DOG), "json"); - Assert.assertTrue(responseAsString.contains("\"aggregations\":{\"composite_buckets\":{\"after_key\":{\"age\":4}," - + "\"buckets\":[{\"key\":{\"age\":2},\"doc_count\":1,\"count(*)\":{\"value\":1}},{\"key\":{\"age\":4}," - + "\"doc_count\":1,\"count(*)\":{\"value\":1}}]}}")); - } - - @Test - public void selectWithoutWhereTest() { - String responseAsString = - executeQuery(String.format("SELECT count(*) FROM %s group by age", TEST_INDEX_DOG), "json"); - - Assert.assertTrue(responseAsString.contains("\"aggregations\":{\"composite_buckets\":{\"after_key\":{\"age\":4}," - + "\"buckets\":[{\"key\":{\"age\":2},\"doc_count\":1,\"count(*)\":{\"value\":1}},{\"key\":{\"age\":4}," - + "\"doc_count\":1,\"count(*)\":{\"value\":1}}]}}")); - } - - @Test - public void selectFromTwoDocumentsTest() { - // This query will fall back to V1, as V2 doesn't support Joins yet. - String responseAsString = - executeQuery(String.format("SELECT d.age, c.int0 FROM %s c JOIN %s d WHERE c.int1 > 2", - TEST_INDEX_CALCS, TEST_INDEX_DOG), "json"); - - JSONObject response = new JSONObject(responseAsString); - Assert.assertTrue(response.has("hits")); - - JSONArray hits = response.getJSONObject("hits").getJSONArray("hits"); - Assert.assertTrue(hits.length() > 0); - for (int i = 0; i < hits.length(); i++) { - JSONObject jsonObject = hits.getJSONObject(i); - Assert.assertTrue(jsonObject.getJSONObject("_source").keySet().contains("d.age")); - Assert.assertTrue(jsonObject.getJSONObject("_source").keySet().contains("c.int0")); - } - } - } - From 52a3c99a3e376f3e4f8c31948ce138e296440a35 Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Thu, 2 Mar 2023 15:09:58 -0800 Subject: [PATCH 15/38] Addressed PR feedback Signed-off-by: Guian Gumpac --- .../analysis/JsonSupportAnalysisContext.java | 15 ++++ .../sql/analysis/JsonSupportAnalyzer.java | 75 +++++++++++++++++++ .../org/opensearch/sql/sql/SQLService.java | 70 +---------------- .../opensearch/sql/sql/SQLServiceTest.java | 7 +- 4 files changed, 98 insertions(+), 69 deletions(-) create mode 100644 core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalysisContext.java create mode 100644 core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java diff --git a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalysisContext.java b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalysisContext.java new file mode 100644 index 0000000000..d266ffa9c9 --- /dev/null +++ b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalysisContext.java @@ -0,0 +1,15 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.analysis; + +import lombok.Getter; +import lombok.Setter; + +public class JsonSupportAnalysisContext { + @Getter + @Setter + boolean isJSONSupported = true; +} diff --git a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java new file mode 100644 index 0000000000..bdec41467a --- /dev/null +++ b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java @@ -0,0 +1,75 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.analysis; + +import org.apache.commons.lang3.StringUtils; +import org.opensearch.sql.ast.AbstractNodeVisitor; +import org.opensearch.sql.ast.Node; +import org.opensearch.sql.ast.expression.Alias; +import org.opensearch.sql.ast.expression.Cast; +import org.opensearch.sql.ast.expression.Function; +import org.opensearch.sql.ast.expression.Literal; +import org.opensearch.sql.ast.tree.Aggregation; +import org.opensearch.sql.ast.tree.Project; + +public class JsonSupportAnalyzer extends AbstractNodeVisitor { + @Override + public Boolean visit(Node node, JsonSupportAnalysisContext context) { + // A node is supported if all of its children are supported. + return node.getChild().stream().filter(c -> c.accept(this, context) != null) + .allMatch(c -> c.accept(this, context)); + } + + @Override + public Boolean visitChildren(Node node, JsonSupportAnalysisContext context) { + for (Node child : node.getChild()) { + if (!child.accept(this, context)) + return false; + } + return true; + } + + @Override + public Boolean visitFunction(Function node, JsonSupportAnalysisContext context) { + // queries with function calls are not supported. + return false; + } + + @Override + public Boolean visitLiteral(Literal node, JsonSupportAnalysisContext context) { + // queries with literal values are not supported + return false; + } + + @Override + public Boolean visitCast(Cast node, JsonSupportAnalysisContext context) { + // Queries with cast are not supported + return false; + } + + @Override + public Boolean visitAlias(Alias node, JsonSupportAnalysisContext context) { + // Alias node is accepted if it does not have a user-defined alias + // and if the delegated expression is accepted. + if (!StringUtils.isEmpty(node.getAlias())) + return false; + else { + return node.getDelegated().accept(this, context); + } + } + + @Override + public Boolean visitProject(Project node, JsonSupportAnalysisContext context) { + return visit(node, context) + && node.getProjectList().stream().filter(c -> c.accept(this, context) != null) + .allMatch(e -> e.accept(this, context)); + } + + @Override + public Boolean visitAggregation(Aggregation node, JsonSupportAnalysisContext context) { + return node.getGroupExprList().isEmpty(); + } +} diff --git a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java index 64b6bd4fd9..b4c59748b6 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java +++ b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java @@ -13,6 +13,8 @@ import lombok.Setter; import org.antlr.v4.runtime.tree.ParseTree; import org.apache.commons.lang3.StringUtils; +import org.opensearch.sql.analysis.JsonSupportAnalysisContext; +import org.opensearch.sql.analysis.JsonSupportAnalyzer; import org.opensearch.sql.ast.AbstractNodeVisitor; import org.opensearch.sql.ast.Node; import org.opensearch.sql.ast.expression.Alias; @@ -94,72 +96,8 @@ private AbstractPlan plan( // There is no full support for JSON format yet for in memory operations, aliases, literals, and casts // Aggregation has differences with legacy results if (request.format().getFormatName().equals("json")) { - class VisitorContext { - @Getter - @Setter - boolean isJSONSupported = true; - } - - class JsonSupportedVisitor extends AbstractNodeVisitor { - @Override - public Boolean visit(Node node, VisitorContext context) { - // A node is supported if all of its children are supported. - return node.getChild().stream().filter(c -> c.accept(this, context) != null) - .allMatch(c -> c.accept(this, context)); - } - - @Override - public Boolean visitProject(Project node, VisitorContext context) { - // Overridden visit functions are done in memory and are not supported with json format - class UnsupportedExpressionsVisitor - extends AbstractNodeVisitor { - - @Override - public Boolean visitFunction(Function node, Object context) { - // queries with function calls are not supported. - return false; - } - - @Override - public Boolean visitLiteral(Literal node, Object context) { - // queries with literal values are not supported - return false; - } - - @Override - public Boolean visitCast(Cast node, Object context) { - // Queries with cast are not supported - return false; - } - - @Override - public Boolean visitAlias(Alias node, Object context) { - // Alias node is accepted if it does not have a user-defined alias - // and if the delegated expression is accepted. - if (!StringUtils.isEmpty(node.getAlias()))// && node.getDelegated().accept(this, context)) - return false; - else { - return node.getDelegated().accept(this, context); - } - } - } - - // Project node is supported if all of its children are supported and - // all expressions in project list are supported. - UnsupportedExpressionsVisitor unsupportedExpressionsVisitor = new UnsupportedExpressionsVisitor(); - return visit(node, context) - && node.getProjectList().stream().filter(c -> c.accept(unsupportedExpressionsVisitor, context) != null) - .allMatch(e -> e.accept(unsupportedExpressionsVisitor, context)); - - } - - @Override - public Boolean visitAggregation(Aggregation node, VisitorContext context) { - return node.getGroupExprList().isEmpty(); - } - } - - Boolean isJsonSupported = ((Query) statement).getPlan().accept(new JsonSupportedVisitor(), new VisitorContext()); + Boolean isJsonSupported = ((Query) statement).getPlan() + .accept(new JsonSupportAnalyzer(), new JsonSupportAnalysisContext()); if (!isJsonSupported) { throw new UnsupportedOperationException( diff --git a/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java b/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java index 1dbedcea83..1758d58526 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java @@ -7,6 +7,7 @@ package org.opensearch.sql.sql; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; @@ -118,7 +119,7 @@ public void onResponse(QueryResponse response) { @Override public void onFailure(Exception e) { - assert(e instanceof UnsupportedOperationException); + assertTrue(e instanceof UnsupportedOperationException); } }); } @@ -135,7 +136,7 @@ public void onResponse(QueryResponse response) { @Override public void onFailure(Exception e) { - assert(e instanceof UnsupportedOperationException); + assertTrue(e instanceof UnsupportedOperationException); } }); } @@ -152,7 +153,7 @@ public void onResponse(QueryResponse response) { @Override public void onFailure(Exception e) { - assert(e instanceof UnsupportedOperationException); + assertTrue(e instanceof UnsupportedOperationException); } }); } From b676acdc9eec066217e37ad9efb9a0c242774714 Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Thu, 2 Mar 2023 16:15:38 -0800 Subject: [PATCH 16/38] Removed unnecessary context Signed-off-by: Guian Gumpac --- .../analysis/JsonSupportAnalysisContext.java | 15 --------- .../sql/analysis/JsonSupportAnalyzer.java | 31 +++++++++---------- .../org/opensearch/sql/sql/SQLService.java | 15 +-------- 3 files changed, 16 insertions(+), 45 deletions(-) delete mode 100644 core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalysisContext.java diff --git a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalysisContext.java b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalysisContext.java deleted file mode 100644 index d266ffa9c9..0000000000 --- a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalysisContext.java +++ /dev/null @@ -1,15 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.analysis; - -import lombok.Getter; -import lombok.Setter; - -public class JsonSupportAnalysisContext { - @Getter - @Setter - boolean isJSONSupported = true; -} diff --git a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java index bdec41467a..28af1f7e98 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java @@ -15,61 +15,60 @@ import org.opensearch.sql.ast.tree.Aggregation; import org.opensearch.sql.ast.tree.Project; -public class JsonSupportAnalyzer extends AbstractNodeVisitor { +public class JsonSupportAnalyzer extends AbstractNodeVisitor { @Override - public Boolean visit(Node node, JsonSupportAnalysisContext context) { + public Boolean visit(Node node, Object context) { // A node is supported if all of its children are supported. - return node.getChild().stream().filter(c -> c.accept(this, context) != null) - .allMatch(c -> c.accept(this, context)); + return node.getChild().stream().filter(c -> c.accept(this, null) != null) + .allMatch(c -> c.accept(this, null)); } @Override - public Boolean visitChildren(Node node, JsonSupportAnalysisContext context) { + public Boolean visitChildren(Node node, Object context) { for (Node child : node.getChild()) { - if (!child.accept(this, context)) + if (!child.accept(this, null)) return false; } return true; } @Override - public Boolean visitFunction(Function node, JsonSupportAnalysisContext context) { + public Boolean visitFunction(Function node, Object context) { // queries with function calls are not supported. return false; } @Override - public Boolean visitLiteral(Literal node, JsonSupportAnalysisContext context) { + public Boolean visitLiteral(Literal node, Object context) { // queries with literal values are not supported return false; } @Override - public Boolean visitCast(Cast node, JsonSupportAnalysisContext context) { + public Boolean visitCast(Cast node, Object context) { // Queries with cast are not supported return false; } @Override - public Boolean visitAlias(Alias node, JsonSupportAnalysisContext context) { + public Boolean visitAlias(Alias node, Object context) { // Alias node is accepted if it does not have a user-defined alias // and if the delegated expression is accepted. if (!StringUtils.isEmpty(node.getAlias())) return false; else { - return node.getDelegated().accept(this, context); + return node.getDelegated().accept(this, null); } } @Override - public Boolean visitProject(Project node, JsonSupportAnalysisContext context) { - return visit(node, context) - && node.getProjectList().stream().filter(c -> c.accept(this, context) != null) - .allMatch(e -> e.accept(this, context)); + public Boolean visitProject(Project node, Object context) { + return visit(node, null) + && node.getProjectList().stream().allMatch(e -> e.accept(this, null)); } @Override - public Boolean visitAggregation(Aggregation node, JsonSupportAnalysisContext context) { + public Boolean visitAggregation(Aggregation node, Object context) { return node.getGroupExprList().isEmpty(); } } diff --git a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java index b4c59748b6..bdd6f14477 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java +++ b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java @@ -8,23 +8,11 @@ import java.util.Optional; -import lombok.Getter; import lombok.RequiredArgsConstructor; -import lombok.Setter; import org.antlr.v4.runtime.tree.ParseTree; -import org.apache.commons.lang3.StringUtils; -import org.opensearch.sql.analysis.JsonSupportAnalysisContext; import org.opensearch.sql.analysis.JsonSupportAnalyzer; -import org.opensearch.sql.ast.AbstractNodeVisitor; -import org.opensearch.sql.ast.Node; -import org.opensearch.sql.ast.expression.Alias; -import org.opensearch.sql.ast.expression.Cast; -import org.opensearch.sql.ast.expression.Function; -import org.opensearch.sql.ast.expression.Literal; import org.opensearch.sql.ast.statement.Query; import org.opensearch.sql.ast.statement.Statement; -import org.opensearch.sql.ast.tree.Aggregation; -import org.opensearch.sql.ast.tree.Project; import org.opensearch.sql.common.response.ResponseListener; import org.opensearch.sql.executor.ExecutionEngine.ExplainResponse; import org.opensearch.sql.executor.ExecutionEngine.QueryResponse; @@ -96,8 +84,7 @@ private AbstractPlan plan( // There is no full support for JSON format yet for in memory operations, aliases, literals, and casts // Aggregation has differences with legacy results if (request.format().getFormatName().equals("json")) { - Boolean isJsonSupported = ((Query) statement).getPlan() - .accept(new JsonSupportAnalyzer(), new JsonSupportAnalysisContext()); + Boolean isJsonSupported = ((Query) statement).getPlan().accept(new JsonSupportAnalyzer(), null); if (!isJsonSupported) { throw new UnsupportedOperationException( From f7c5ee64a04e58676aa7b46f386b0cb10ad22851 Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Fri, 3 Mar 2023 13:54:00 -0800 Subject: [PATCH 17/38] Added fall back for Filter functions Signed-off-by: Guian Gumpac --- .../opensearch/sql/analysis/JsonSupportAnalyzer.java | 10 ++++++++-- .../main/java/org/opensearch/sql/sql/SQLService.java | 2 +- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java index 28af1f7e98..603dd63d96 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java @@ -13,14 +13,14 @@ import org.opensearch.sql.ast.expression.Function; import org.opensearch.sql.ast.expression.Literal; import org.opensearch.sql.ast.tree.Aggregation; +import org.opensearch.sql.ast.tree.Filter; import org.opensearch.sql.ast.tree.Project; public class JsonSupportAnalyzer extends AbstractNodeVisitor { @Override public Boolean visit(Node node, Object context) { // A node is supported if all of its children are supported. - return node.getChild().stream().filter(c -> c.accept(this, null) != null) - .allMatch(c -> c.accept(this, null)); + return node.getChild().stream().allMatch(c -> c.accept(this, null)); } @Override @@ -71,4 +71,10 @@ public Boolean visitProject(Project node, Object context) { public Boolean visitAggregation(Aggregation node, Object context) { return node.getGroupExprList().isEmpty(); } + + @Override + public Boolean visitFilter(Filter node, Object context) { + return visit(node, null) + && node.getCondition().accept(this, null); + } } diff --git a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java index bdd6f14477..178ef34d08 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java +++ b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java @@ -83,7 +83,7 @@ private AbstractPlan plan( // There is no full support for JSON format yet for in memory operations, aliases, literals, and casts // Aggregation has differences with legacy results - if (request.format().getFormatName().equals("json")) { + if (request.format().getFormatName().equals("json") && statement instanceof Query) { Boolean isJsonSupported = ((Query) statement).getPlan().accept(new JsonSupportAnalyzer(), null); if (!isJsonSupported) { From bdb0ff6b1c3ab7a55a58501ae61dd9d01bf972ce Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Wed, 22 Feb 2023 09:20:58 -0800 Subject: [PATCH 18/38] Made new engine fallback to legacy for in memory operations for json format Signed-off-by: Guian Gumpac --- .../sql/legacy/plugin/RestSQLQueryAction.java | 2 +- .../java/org/opensearch/sql/sql/SQLService.java | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java index caddc02ca3..818c1b5e70 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java @@ -126,7 +126,7 @@ public void onResponse(T response) { @Override public void onFailure(Exception e) { - if (e instanceof SyntaxCheckException) { + if (e instanceof SyntaxCheckException || e instanceof UnsupportedOperationException) { fallBackHandler.accept(channel, e); } else { next.onFailure(e); diff --git a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java index 082a3e9581..6da2169af1 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java +++ b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java @@ -6,10 +6,15 @@ package org.opensearch.sql.sql; +import java.util.List; import java.util.Optional; import lombok.RequiredArgsConstructor; import org.antlr.v4.runtime.tree.ParseTree; +import org.opensearch.sql.ast.expression.Alias; +import org.opensearch.sql.ast.expression.Function; +import org.opensearch.sql.ast.statement.Query; import org.opensearch.sql.ast.statement.Statement; +import org.opensearch.sql.ast.tree.Project; import org.opensearch.sql.common.response.ResponseListener; import org.opensearch.sql.executor.ExecutionEngine.ExplainResponse; import org.opensearch.sql.executor.ExecutionEngine.QueryResponse; @@ -75,6 +80,17 @@ private AbstractPlan plan( .isExplain(request.isExplainRequest()) .build())); + // There is no full support for JSON format yet for in memory operations + if (request.format().getFormatName().equals("json")) { + List projectList = ((Project) ((Query) statement).getPlan()).getProjectList(); + + for (var project : projectList) { + if (((Alias) project).getDelegated() instanceof Function) + throw new UnsupportedOperationException("[%s] is not yet supported with json " + + "format because it is not pushed down to the OpenSearch instance"); + } + } + return queryExecutionFactory.create(statement, queryListener, explainListener); } } From 7b4fea107bb10e30b10784747f233464428a87ca Mon Sep 17 00:00:00 2001 From: MaxKsyunz Date: Wed, 22 Feb 2023 00:48:31 -0800 Subject: [PATCH 19/38] Address build failures Signed-off-by: MaxKsyunz --- .../org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java index 6249cfba6c..27134d6652 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java @@ -20,9 +20,9 @@ import org.opensearch.common.Strings; import org.opensearch.common.settings.Settings; import org.opensearch.common.xcontent.XContentFactory; -import org.opensearch.common.xcontent.XContentType; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.common.xcontent.XContentType; import org.opensearch.rest.BaseRestHandler; import org.opensearch.rest.BytesRestResponse; import org.opensearch.rest.RestController; From 581e7fe52d14e52f99d7c3a4fab3965bbe0bf2aa Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Mon, 27 Feb 2023 14:17:21 -0800 Subject: [PATCH 20/38] Added legacy fall back Signed-off-by: Guian Gumpac --- .../org/opensearch/sql/sql/SQLService.java | 46 ++++- .../sql/sql/antlr/SQLSyntaxParser.java | 5 + .../opensearch/sql/sql/SQLServiceTest.java | 160 ++++++++++++++++++ 3 files changed, 207 insertions(+), 4 deletions(-) diff --git a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java index 6da2169af1..697776663a 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java +++ b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java @@ -11,9 +11,14 @@ import lombok.RequiredArgsConstructor; import org.antlr.v4.runtime.tree.ParseTree; import org.opensearch.sql.ast.expression.Alias; +import org.opensearch.sql.ast.expression.Cast; import org.opensearch.sql.ast.expression.Function; +import org.opensearch.sql.ast.expression.Literal; import org.opensearch.sql.ast.statement.Query; import org.opensearch.sql.ast.statement.Statement; +import org.opensearch.sql.ast.tree.Aggregation; +import org.opensearch.sql.ast.tree.Filter; +import org.opensearch.sql.ast.tree.Limit; import org.opensearch.sql.ast.tree.Project; import org.opensearch.sql.common.response.ResponseListener; import org.opensearch.sql.executor.ExecutionEngine.ExplainResponse; @@ -70,6 +75,11 @@ private AbstractPlan plan( SQLQueryRequest request, Optional> queryListener, Optional> explainListener) { + // Hints are currently unsupported for V2 + ParseTree cstHints = parser.parseHints(request.getQuery()); + if (cstHints.getChildCount() > 1) + throw new UnsupportedOperationException("Hints are not yet supported in the new engine."); + // 1.Parse query and convert parse tree (CST) to abstract syntax tree (AST) ParseTree cst = parser.parse(request.getQuery()); Statement statement = @@ -80,14 +90,42 @@ private AbstractPlan plan( .isExplain(request.isExplainRequest()) .build())); - // There is no full support for JSON format yet for in memory operations + // There is no full support for JSON format yet for in memory operations, aliases, literals, and casts + // Aggregation has differences with legacy results if (request.format().getFormatName().equals("json")) { List projectList = ((Project) ((Query) statement).getPlan()).getProjectList(); + List planChildren = ((Project) ((Query) statement).getPlan()).getChild(); + UnsupportedOperationException aggException = new UnsupportedOperationException( + "Queries with aggregation are not yet supported with json format in the new engine"); + + for (var child : planChildren) { + if (child instanceof Aggregation && ((Aggregation) child).getGroupExprList().size() > 0) + throw aggException; + + if (child instanceof Filter) { + for (var filterChild : ((Filter) child).getChild()) + if (filterChild instanceof Aggregation) + throw aggException; + } + + if (child instanceof Limit) { + for (var filterChild : ((Limit) child).getChild()) + if(filterChild instanceof Aggregation) + throw aggException; + } + } for (var project : projectList) { - if (((Alias) project).getDelegated() instanceof Function) - throw new UnsupportedOperationException("[%s] is not yet supported with json " - + "format because it is not pushed down to the OpenSearch instance"); + if (project instanceof Alias) { + Alias projectAlias = ((Alias) project); + + if (projectAlias.getDelegated() instanceof Function || + projectAlias.getDelegated() instanceof Literal || + projectAlias.getDelegated() instanceof Cast || + (projectAlias.getAlias() != null && !projectAlias.getAlias().isEmpty())) + throw new UnsupportedOperationException("The query is not yet supported with json " + + "format because there is an expression in the query that is not pushed down to the OpenSearch instance"); + } } } diff --git a/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java b/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java index ee1e991bd4..73b2864152 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java +++ b/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java @@ -32,4 +32,9 @@ public ParseTree parse(String query) { return parser.root(); } + public ParseTree parseHints(String query) { + OpenSearchSQLLexer lexer = new OpenSearchSQLLexer(new CaseInsensitiveCharStream(query)); + OpenSearchSQLParser hintsParser = new OpenSearchSQLParser(new CommonTokenStream(lexer, OpenSearchSQLLexer.SQLCOMMENT)); + return hintsParser.root(); + } } diff --git a/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java b/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java index ccb8363d0b..711ef8a73d 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java @@ -7,6 +7,7 @@ package org.opensearch.sql.sql; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; @@ -83,6 +84,148 @@ public void onFailure(Exception e) { }); } + @Test + public void canExecuteJsonFormatRequest() { + doAnswer(invocation -> { + ResponseListener listener = invocation.getArgument(1); + listener.onResponse(new QueryResponse(schema, Collections.emptyList(), "")); + return null; + }).when(queryService).execute(any(), any()); + + sqlService.execute( + new SQLQueryRequest(new JSONObject(), "SELECT FIELD FROM TABLE", QUERY, "json"), + new ResponseListener() { + @Override + public void onResponse(QueryResponse response) { + assertNotNull(response); + } + + @Override + public void onFailure(Exception e) { + fail(e); + } + }); + } + + @Test + public void canThrowUnsupportedExceptionForAggregationJsonQuery() { + sqlService.execute( + new SQLQueryRequest(new JSONObject(), "SELECT FIELD FROM TABLE GROUP BY FIELD", QUERY, "json"), + new ResponseListener() { + @Override + public void onResponse(QueryResponse response) { + assertNotNull(response); + } + + @Override + public void onFailure(Exception e) { + assert(e instanceof UnsupportedOperationException); + } + }); + } + + @Test + public void canThrowUnsupportedExceptionForFilterAggregationJsonQuery() { + sqlService.execute( + new SQLQueryRequest(new JSONObject(), "SELECT * FROM TABLE GROUP BY FIELD HAVING COUNT(*) > 0", QUERY, "json"), + new ResponseListener() { + @Override + public void onResponse(QueryResponse response) { + assertNotNull(response); + } + + @Override + public void onFailure(Exception e) { + assert(e instanceof UnsupportedOperationException); + } + }); + } + + @Test + public void canThrowUnsupportedExceptionForLimitAggregationJsonQuery() { + sqlService.execute( + new SQLQueryRequest(new JSONObject(), "SELECT * FROM TABLE GROUP BY FIELD LIMIT 10", QUERY, "json"), + new ResponseListener() { + @Override + public void onResponse(QueryResponse response) { + assertNotNull(response); + } + + @Override + public void onFailure(Exception e) { + assert(e instanceof UnsupportedOperationException); + } + }); + } + + @Test + public void canThrowUnsupportedExceptionForAliasJsonQuery() { + sqlService.execute( + new SQLQueryRequest(new JSONObject(), "SELECT FIELD as FIELD FROM TABLE", QUERY, "json"), + new ResponseListener() { + @Override + public void onResponse(QueryResponse response) { + assertNotNull(response); + } + + @Override + public void onFailure(Exception e) { + assert(e instanceof UnsupportedOperationException); + } + }); + } + + @Test + public void canThrowUnsupportedExceptionForFunctionJsonQuery() { + sqlService.execute( + new SQLQueryRequest(new JSONObject(), "SELECT 1 + 1", QUERY, "json"), + new ResponseListener() { + @Override + public void onResponse(QueryResponse response) { + assertNotNull(response); + } + + @Override + public void onFailure(Exception e) { + assert(e instanceof UnsupportedOperationException); + } + }); + } + + @Test + public void canThrowUnsupportedExceptionForLiteralJsonQuery() { + sqlService.execute( + new SQLQueryRequest(new JSONObject(), "SELECT 123", QUERY, "json"), + new ResponseListener() { + @Override + public void onResponse(QueryResponse response) { + assertNotNull(response); + } + + @Override + public void onFailure(Exception e) { + assert(e instanceof UnsupportedOperationException); + } + }); + } + + @Test + public void canThrowUnsupportedExceptionForCastJsonQuery() { + sqlService.execute( + new SQLQueryRequest(new JSONObject(), "SELECT CAST(FIELD AS DOUBLE)", QUERY, "json"), + new ResponseListener() { + @Override + public void onResponse(QueryResponse response) { + assertNotNull(response); + } + + @Override + public void onFailure(Exception e) { + assert(e instanceof UnsupportedOperationException); + } + }); + } + @Test public void canExecuteCsvFormatRequest() { doAnswer(invocation -> { @@ -106,6 +249,23 @@ public void onFailure(Exception e) { }); } + @Test + public void canThrowUnsupportedExceptionForHintsQuery() { + sqlService.execute( + new SQLQueryRequest(new JSONObject(), "SELECT /*! HINTS */ 123", QUERY, "jdbc"), + new ResponseListener() { + @Override + public void onResponse(QueryResponse response) { + assertNotNull(response); + } + + @Override + public void onFailure(Exception e) { + assert(e instanceof UnsupportedOperationException); + } + }); + } + @Test public void canExplainSqlQuery() { doAnswer(invocation -> { From d8828301bcf0741888dc7b9159061770fbd7cb49 Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Wed, 1 Mar 2023 14:03:01 -0800 Subject: [PATCH 21/38] Refactored fall back logic to use visitor design pattern Signed-off-by: Guian Gumpac --- .../org/opensearch/sql/sql/SQLService.java | 116 +++++++++++++----- .../sql/sql/antlr/SQLSyntaxParser.java | 4 +- 2 files changed, 85 insertions(+), 35 deletions(-) diff --git a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java index 697776663a..dde2fa416f 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java +++ b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java @@ -6,10 +6,15 @@ package org.opensearch.sql.sql; -import java.util.List; import java.util.Optional; + +import lombok.Getter; import lombok.RequiredArgsConstructor; +import lombok.Setter; import org.antlr.v4.runtime.tree.ParseTree; +import org.apache.commons.lang3.StringUtils; +import org.opensearch.sql.ast.AbstractNodeVisitor; +import org.opensearch.sql.ast.Node; import org.opensearch.sql.ast.expression.Alias; import org.opensearch.sql.ast.expression.Cast; import org.opensearch.sql.ast.expression.Function; @@ -17,8 +22,6 @@ import org.opensearch.sql.ast.statement.Query; import org.opensearch.sql.ast.statement.Statement; import org.opensearch.sql.ast.tree.Aggregation; -import org.opensearch.sql.ast.tree.Filter; -import org.opensearch.sql.ast.tree.Limit; import org.opensearch.sql.ast.tree.Project; import org.opensearch.sql.common.response.ResponseListener; import org.opensearch.sql.executor.ExecutionEngine.ExplainResponse; @@ -75,9 +78,7 @@ private AbstractPlan plan( SQLQueryRequest request, Optional> queryListener, Optional> explainListener) { - // Hints are currently unsupported for V2 - ParseTree cstHints = parser.parseHints(request.getQuery()); - if (cstHints.getChildCount() > 1) + if (parser.containsHints(request.getQuery())) throw new UnsupportedOperationException("Hints are not yet supported in the new engine."); // 1.Parse query and convert parse tree (CST) to abstract syntax tree (AST) @@ -93,39 +94,88 @@ private AbstractPlan plan( // There is no full support for JSON format yet for in memory operations, aliases, literals, and casts // Aggregation has differences with legacy results if (request.format().getFormatName().equals("json")) { - List projectList = ((Project) ((Query) statement).getPlan()).getProjectList(); - List planChildren = ((Project) ((Query) statement).getPlan()).getChild(); - UnsupportedOperationException aggException = new UnsupportedOperationException( - "Queries with aggregation are not yet supported with json format in the new engine"); - - for (var child : planChildren) { - if (child instanceof Aggregation && ((Aggregation) child).getGroupExprList().size() > 0) - throw aggException; - - if (child instanceof Filter) { - for (var filterChild : ((Filter) child).getChild()) - if (filterChild instanceof Aggregation) - throw aggException; + class VisitorContext { + @Getter + @Setter + boolean isJSONSupported = true; + } + + class JsonSupportedVisitor extends AbstractNodeVisitor { + @Override + public Boolean visit(Node node, VisitorContext context) { + // A node is supported if it's a leaf node or if all of its children are supported. + return node.getChild().isEmpty() + || node.getChild().stream().filter(c -> c.accept(this, context) != null) + .allMatch(c -> c.accept(this, context)); } - if (child instanceof Limit) { - for (var filterChild : ((Limit) child).getChild()) - if(filterChild instanceof Aggregation) - throw aggException; + @Override + public Boolean visitAlias(Alias node, VisitorContext context) { + // Alias node is accepted if it does not have a user-defined alias + // and if the delegated expression is accepted. + if (!StringUtils.isEmpty(node.getAlias())) + return false; + else + return node.getDelegated().accept(this, context); } - } - for (var project : projectList) { - if (project instanceof Alias) { - Alias projectAlias = ((Alias) project); + @Override + public Boolean visitProject(Project node, VisitorContext context) { + // Overridden visit functions are done in memory and are not supported with json format + class UnsupportedExpressionsVisitor + extends AbstractNodeVisitor { + + @Override + public Boolean visitFunction(Function node, Object context) { + // queries with function calls are not supported. + return false; + } + + @Override + public Boolean visitLiteral(Literal node, Object context) { + // queries with literal values are not supported + return false; + } + + @Override + public Boolean visitCast(Cast node, Object context) { + // Queries with cast are not supported + return false; + } + + @Override + public Boolean visitAlias(Alias node, Object context) { + // Alias node is accepted if it does not have a user-defined alias + // and if the delegated expression is accepted. + if (!StringUtils.isEmpty(node.getAlias()))// && node.getDelegated().accept(this, context)) + return false; + else { + return node.getDelegated().accept(this, context); + } + } + } + + // Project node is supported if all of its children are supported and + // all expressions in project list are supported. + UnsupportedExpressionsVisitor unsupportedExpressionsVisitor = new UnsupportedExpressionsVisitor(); + return visit(node, context) + && node.getProjectList().stream().filter(c -> c.accept(unsupportedExpressionsVisitor, context) != null) + .allMatch(e -> e.accept(unsupportedExpressionsVisitor, context)); - if (projectAlias.getDelegated() instanceof Function || - projectAlias.getDelegated() instanceof Literal || - projectAlias.getDelegated() instanceof Cast || - (projectAlias.getAlias() != null && !projectAlias.getAlias().isEmpty())) - throw new UnsupportedOperationException("The query is not yet supported with json " - + "format because there is an expression in the query that is not pushed down to the OpenSearch instance"); } + + @Override + public Boolean visitAggregation(Aggregation node, VisitorContext context) { + return node.getGroupExprList().isEmpty(); + } + } + + Boolean isJsonSupported = ((Query) statement).getPlan().accept(new JsonSupportedVisitor(), new VisitorContext()); + + if (!isJsonSupported) { + throw new UnsupportedOperationException( + "Queries with aggregation and in memory operations (cast, literals, alias, math, etc.)" + + " are not yet supported with json format in the new engine"); } } diff --git a/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java b/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java index 73b2864152..8ffba7d097 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java +++ b/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java @@ -32,9 +32,9 @@ public ParseTree parse(String query) { return parser.root(); } - public ParseTree parseHints(String query) { + public boolean containsHints(String query) { OpenSearchSQLLexer lexer = new OpenSearchSQLLexer(new CaseInsensitiveCharStream(query)); OpenSearchSQLParser hintsParser = new OpenSearchSQLParser(new CommonTokenStream(lexer, OpenSearchSQLLexer.SQLCOMMENT)); - return hintsParser.root(); + return hintsParser.root().getChildCount() > 1; } } From 7ecc76ebd3d0830e815db0c4e57957f1490614c8 Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Wed, 1 Mar 2023 15:33:08 -0800 Subject: [PATCH 22/38] Added unit tests Signed-off-by: Guian Gumpac --- .../org/opensearch/sql/sql/SQLService.java | 15 ++------ .../opensearch/sql/sql/SQLServiceTest.java | 35 ------------------- .../sql/sql/antlr/SQLSyntaxParserTest.java | 10 +++++- .../sql/sql/domain/SQLQueryRequestTest.java | 9 +++++ 4 files changed, 20 insertions(+), 49 deletions(-) diff --git a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java index dde2fa416f..64b6bd4fd9 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java +++ b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java @@ -103,22 +103,11 @@ class VisitorContext { class JsonSupportedVisitor extends AbstractNodeVisitor { @Override public Boolean visit(Node node, VisitorContext context) { - // A node is supported if it's a leaf node or if all of its children are supported. - return node.getChild().isEmpty() - || node.getChild().stream().filter(c -> c.accept(this, context) != null) + // A node is supported if all of its children are supported. + return node.getChild().stream().filter(c -> c.accept(this, context) != null) .allMatch(c -> c.accept(this, context)); } - @Override - public Boolean visitAlias(Alias node, VisitorContext context) { - // Alias node is accepted if it does not have a user-defined alias - // and if the delegated expression is accepted. - if (!StringUtils.isEmpty(node.getAlias())) - return false; - else - return node.getDelegated().accept(this, context); - } - @Override public Boolean visitProject(Project node, VisitorContext context) { // Overridden visit functions are done in memory and are not supported with json format diff --git a/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java b/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java index 711ef8a73d..1dbedcea83 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java @@ -7,7 +7,6 @@ package org.opensearch.sql.sql; import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; @@ -124,40 +123,6 @@ public void onFailure(Exception e) { }); } - @Test - public void canThrowUnsupportedExceptionForFilterAggregationJsonQuery() { - sqlService.execute( - new SQLQueryRequest(new JSONObject(), "SELECT * FROM TABLE GROUP BY FIELD HAVING COUNT(*) > 0", QUERY, "json"), - new ResponseListener() { - @Override - public void onResponse(QueryResponse response) { - assertNotNull(response); - } - - @Override - public void onFailure(Exception e) { - assert(e instanceof UnsupportedOperationException); - } - }); - } - - @Test - public void canThrowUnsupportedExceptionForLimitAggregationJsonQuery() { - sqlService.execute( - new SQLQueryRequest(new JSONObject(), "SELECT * FROM TABLE GROUP BY FIELD LIMIT 10", QUERY, "json"), - new ResponseListener() { - @Override - public void onResponse(QueryResponse response) { - assertNotNull(response); - } - - @Override - public void onFailure(Exception e) { - assert(e instanceof UnsupportedOperationException); - } - }); - } - @Test public void canThrowUnsupportedExceptionForAliasJsonQuery() { sqlService.execute( diff --git a/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java b/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java index 0b8e64d0bb..007f8ca317 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java @@ -7,8 +7,10 @@ package org.opensearch.sql.sql.antlr; import static org.junit.jupiter.api.Assertions.assertAll; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Streams; @@ -238,7 +240,7 @@ public void can_parse_dayofmonth_functions() { assertNotNull(parser.parse("SELECT dayofmonth('2022-11-18')")); assertNotNull(parser.parse("SELECT day_of_month('2022-11-18')")); } - + @Test public void can_parse_day_of_week_functions() { assertNotNull(parser.parse("SELECT dayofweek('2022-11-18')")); @@ -633,6 +635,12 @@ public void canParseMultiMatchAlternateSyntax() { assertNotNull(parser.parse("SELECT * FROM test WHERE Field = multimatch(\"query\")")); } + @Test + public void containHintsTest() { + assertTrue(parser.containsHints("SELECT /*! HINTS */ field FROM test")); + assertFalse(parser.containsHints("SELECT field FROM test")); + } + private static Stream matchPhraseQueryComplexQueries() { return Stream.of( "SELECT * FROM t WHERE matchphrasequery(c, 3)", diff --git a/sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java b/sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java index 52a1f534e9..c206433280 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java @@ -142,6 +142,15 @@ public void shouldSupportRawFormat() { assertTrue(csvRequest.isSupported()); } + @Test + public void shouldSupportJsonFormat() { + SQLQueryRequest jsonRequest = + SQLQueryRequestBuilder.request("SELECT 1") + .format("json") + .build(); + assertTrue(jsonRequest.isSupported()); + } + /** * SQL query request build helper to improve test data setup readability. */ From 1a85ff09e02452b4462f12e194b0021de85db982 Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Wed, 1 Mar 2023 15:42:55 -0800 Subject: [PATCH 23/38] Revert "Address build failures" This reverts commit 17684b3f945a70017865c4a7db8f26e866c3b26a. --- .../test/java/org/opensearch/sql/legacy/JSONRequestIT.java | 4 ++-- .../test/java/org/opensearch/sql/legacy/SourceFieldIT.java | 4 ++-- .../main/java/org/opensearch/sql/legacy/domain/Paramer.java | 2 +- .../org/opensearch/sql/legacy/domain/hints/HintFactory.java | 2 +- .../sql/legacy/executor/GetIndexRequestRestListener.java | 4 ++-- .../sql/legacy/executor/join/ElasticJoinExecutor.java | 2 +- .../opensearch/sql/legacy/executor/join/ElasticUtils.java | 6 +++--- .../java/org/opensearch/sql/legacy/query/QueryAction.java | 2 +- .../sql/legacy/query/join/JoinRequestBuilder.java | 4 ++-- .../legacy/query/join/NestedLoopsElasticRequestBuilder.java | 4 ++-- .../org/opensearch/sql/legacy/query/maker/AggMaker.java | 4 ++-- .../java/org/opensearch/sql/legacy/query/maker/Maker.java | 6 +++--- .../sql/legacy/query/multi/MultiQueryRequestBuilder.java | 4 ++-- .../legacy/query/planner/physical/node/scroll/Scroll.java | 1 + .../java/org/opensearch/sql/legacy/request/SqlRequest.java | 4 ++-- .../opensearch/sql/legacy/utils/JsonPrettyFormatter.java | 6 +++--- .../src/main/java/org/opensearch/sql/plugin/SQLPlugin.java | 2 +- .../opensearch/sql/plugin/rest/RestQuerySettingsAction.java | 4 ++-- 18 files changed, 33 insertions(+), 32 deletions(-) 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 ec5972be78..5b7f9202bf 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 @@ -17,9 +17,9 @@ import org.junit.Test; import org.opensearch.action.search.SearchResponse; import org.opensearch.common.xcontent.LoggingDeprecationHandler; +import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.common.xcontent.XContentFactory; -import org.opensearch.core.xcontent.NamedXContentRegistry; -import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.XContentType; import org.opensearch.search.SearchHit; import org.opensearch.search.SearchHits; 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 756cbcd064..91c8321c95 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 @@ -15,9 +15,9 @@ import org.junit.Test; import org.opensearch.action.search.SearchResponse; import org.opensearch.common.xcontent.LoggingDeprecationHandler; +import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.common.xcontent.XContentFactory; -import org.opensearch.core.xcontent.NamedXContentRegistry; -import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.XContentType; import org.opensearch.search.SearchHit; import org.opensearch.search.SearchHits; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/domain/Paramer.java b/legacy/src/main/java/org/opensearch/sql/legacy/domain/Paramer.java index 5c2c84f9b8..b35ddcb0ed 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/domain/Paramer.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/domain/Paramer.java @@ -15,7 +15,7 @@ import java.util.List; import java.util.Map; import org.opensearch.common.Strings; -import org.opensearch.core.xcontent.ToXContent; +import org.opensearch.common.xcontent.ToXContent; import org.opensearch.index.query.MatchPhraseQueryBuilder; import org.opensearch.index.query.MatchQueryBuilder; import org.opensearch.index.query.MultiMatchQueryBuilder; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/domain/hints/HintFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/domain/hints/HintFactory.java index 18c68d57ab..59ecf2eba8 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/domain/hints/HintFactory.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/domain/hints/HintFactory.java @@ -14,8 +14,8 @@ import java.util.List; import java.util.Map; import org.opensearch.common.xcontent.LoggingDeprecationHandler; +import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.common.xcontent.yaml.YamlXContentParser; -import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.sql.legacy.exception.SqlParseException; /** diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/GetIndexRequestRestListener.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/GetIndexRequestRestListener.java index ebd9648d5e..b930044345 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/GetIndexRequestRestListener.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/GetIndexRequestRestListener.java @@ -17,8 +17,8 @@ import org.opensearch.cluster.metadata.MappingMetadata; import org.opensearch.common.collect.ImmutableOpenMap; import org.opensearch.common.settings.Settings; -import org.opensearch.core.xcontent.ToXContent; -import org.opensearch.core.xcontent.XContentBuilder; +import org.opensearch.common.xcontent.ToXContent; +import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.rest.BytesRestResponse; import org.opensearch.rest.RestChannel; import org.opensearch.rest.RestResponse; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticJoinExecutor.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticJoinExecutor.java index 3bbfcff1dd..811d444a7f 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticJoinExecutor.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticJoinExecutor.java @@ -23,7 +23,7 @@ import org.opensearch.common.document.DocumentField; import org.opensearch.common.text.Text; import org.opensearch.common.unit.TimeValue; -import org.opensearch.core.xcontent.XContentBuilder; +import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.index.mapper.MapperService; import org.opensearch.rest.BytesRestResponse; import org.opensearch.rest.RestChannel; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticUtils.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticUtils.java index 5a91184a09..c3b03c653a 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticUtils.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticUtils.java @@ -6,7 +6,7 @@ package org.opensearch.sql.legacy.executor.join; -import static org.opensearch.core.xcontent.ToXContent.EMPTY_PARAMS; +import static org.opensearch.common.xcontent.ToXContent.EMPTY_PARAMS; import com.google.common.collect.ImmutableMap; import java.io.IOException; @@ -20,10 +20,10 @@ import org.opensearch.common.bytes.BytesReference; import org.opensearch.common.io.stream.BytesStreamOutput; import org.opensearch.common.unit.TimeValue; +import org.opensearch.common.xcontent.ToXContent.Params; +import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.common.xcontent.XContentType; -import org.opensearch.core.xcontent.ToXContent.Params; -import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.search.SearchHit; import org.opensearch.search.SearchHits; import org.opensearch.search.sort.FieldSortBuilder; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/QueryAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/QueryAction.java index 7646639be4..46a9dda1d7 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/QueryAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/QueryAction.java @@ -16,8 +16,8 @@ import org.opensearch.action.support.IndicesOptions; import org.opensearch.client.Client; import org.opensearch.common.xcontent.LoggingDeprecationHandler; +import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.common.xcontent.json.JsonXContentParser; -import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.index.query.QueryBuilders; import org.opensearch.search.collapse.CollapseBuilder; import org.opensearch.search.fetch.subphase.highlight.HighlightBuilder; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/join/JoinRequestBuilder.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/join/JoinRequestBuilder.java index de6cc0cddd..b4ebbb2ad7 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/join/JoinRequestBuilder.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/join/JoinRequestBuilder.java @@ -13,10 +13,10 @@ import org.opensearch.action.ActionResponse; import org.opensearch.action.search.MultiSearchRequest; import org.opensearch.common.bytes.BytesReference; +import org.opensearch.common.xcontent.ToXContent; +import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.common.xcontent.XContentType; -import org.opensearch.core.xcontent.ToXContent; -import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.sql.legacy.query.SqlElasticRequestBuilder; /** diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/join/NestedLoopsElasticRequestBuilder.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/join/NestedLoopsElasticRequestBuilder.java index 6783441931..2e80c05ff2 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/join/NestedLoopsElasticRequestBuilder.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/join/NestedLoopsElasticRequestBuilder.java @@ -11,10 +11,10 @@ import org.json.JSONObject; import org.json.JSONStringer; import org.opensearch.common.bytes.BytesReference; +import org.opensearch.common.xcontent.ToXContent; +import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.common.xcontent.XContentType; -import org.opensearch.core.xcontent.ToXContent; -import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.sql.legacy.domain.Condition; import org.opensearch.sql.legacy.domain.Where; import org.opensearch.sql.legacy.exception.SqlParseException; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/AggMaker.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/AggMaker.java index c7ab574a38..87125721c0 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/AggMaker.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/AggMaker.java @@ -21,10 +21,10 @@ import org.opensearch.common.ParsingException; import org.opensearch.common.Strings; import org.opensearch.common.xcontent.LoggingDeprecationHandler; +import org.opensearch.common.xcontent.NamedXContentRegistry; +import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.json.JsonXContent; import org.opensearch.common.xcontent.json.JsonXContentParser; -import org.opensearch.core.xcontent.NamedXContentRegistry; -import org.opensearch.core.xcontent.XContentParser; import org.opensearch.geo.search.aggregations.bucket.geogrid.GeoHashGridAggregationBuilder; import org.opensearch.join.aggregations.JoinAggregationBuilders; import org.opensearch.script.Script; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/Maker.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/Maker.java index 409288503a..951d87a2c8 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/Maker.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/Maker.java @@ -31,10 +31,10 @@ import org.opensearch.common.geo.builders.ShapeBuilder; import org.opensearch.common.geo.parsers.ShapeParser; import org.opensearch.common.xcontent.LoggingDeprecationHandler; +import org.opensearch.common.xcontent.NamedXContentRegistry; +import org.opensearch.common.xcontent.ToXContent; +import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.json.JsonXContent; -import org.opensearch.core.xcontent.NamedXContentRegistry; -import org.opensearch.core.xcontent.ToXContent; -import org.opensearch.core.xcontent.XContentParser; import org.opensearch.index.query.BoolQueryBuilder; import org.opensearch.index.query.GeoPolygonQueryBuilder; import org.opensearch.index.query.MatchNoneQueryBuilder; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/multi/MultiQueryRequestBuilder.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/multi/MultiQueryRequestBuilder.java index 1b20fbe3fb..4b5448ac30 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/multi/MultiQueryRequestBuilder.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/multi/MultiQueryRequestBuilder.java @@ -16,10 +16,10 @@ import org.opensearch.action.ActionResponse; import org.opensearch.action.search.SearchRequestBuilder; import org.opensearch.common.bytes.BytesReference; +import org.opensearch.common.xcontent.ToXContent; +import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.common.xcontent.XContentType; -import org.opensearch.core.xcontent.ToXContent; -import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.sql.legacy.domain.Field; import org.opensearch.sql.legacy.domain.Select; import org.opensearch.sql.legacy.query.SqlElasticRequestBuilder; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/scroll/Scroll.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/scroll/Scroll.java index 6dd8ff6e89..b623e4c1f7 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/scroll/Scroll.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/scroll/Scroll.java @@ -15,6 +15,7 @@ import org.opensearch.client.Client; import org.opensearch.common.Strings; import org.opensearch.common.unit.TimeValue; +import org.opensearch.common.xcontent.MediaType; import org.opensearch.common.xcontent.XContentType; import org.opensearch.index.query.BoolQueryBuilder; import org.opensearch.index.query.QueryBuilder; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/request/SqlRequest.java b/legacy/src/main/java/org/opensearch/sql/legacy/request/SqlRequest.java index 605ef3c958..14acb26b3c 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/request/SqlRequest.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/request/SqlRequest.java @@ -12,10 +12,10 @@ import org.json.JSONObject; import org.opensearch.common.settings.Settings; import org.opensearch.common.xcontent.LoggingDeprecationHandler; +import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.common.xcontent.XContentFactory; +import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.XContentType; -import org.opensearch.core.xcontent.NamedXContentRegistry; -import org.opensearch.core.xcontent.XContentParser; import org.opensearch.index.query.BoolQueryBuilder; import org.opensearch.search.SearchModule; import org.opensearch.sql.legacy.exception.SqlParseException; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/utils/JsonPrettyFormatter.java b/legacy/src/main/java/org/opensearch/sql/legacy/utils/JsonPrettyFormatter.java index 24cc47b0c0..b94ef06c25 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/utils/JsonPrettyFormatter.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/utils/JsonPrettyFormatter.java @@ -9,11 +9,11 @@ import java.io.IOException; import org.opensearch.common.Strings; import org.opensearch.common.xcontent.LoggingDeprecationHandler; +import org.opensearch.common.xcontent.NamedXContentRegistry; +import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentFactory; +import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.XContentType; -import org.opensearch.core.xcontent.NamedXContentRegistry; -import org.opensearch.core.xcontent.XContentBuilder; -import org.opensearch.core.xcontent.XContentParser; /** * Utility Class for formatting Json string pretty. diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java b/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java index 2c401af352..98826c6f62 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java @@ -40,7 +40,7 @@ import org.opensearch.common.settings.Settings; import org.opensearch.common.settings.SettingsFilter; import org.opensearch.common.util.concurrent.OpenSearchExecutors; -import org.opensearch.core.xcontent.NamedXContentRegistry; +import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.env.Environment; import org.opensearch.env.NodeEnvironment; import org.opensearch.plugins.ActionPlugin; diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java index 27134d6652..59518e7a9d 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java @@ -19,9 +19,9 @@ import org.opensearch.client.node.NodeClient; import org.opensearch.common.Strings; import org.opensearch.common.settings.Settings; +import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentFactory; -import org.opensearch.core.xcontent.XContentBuilder; -import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.XContentType; import org.opensearch.rest.BaseRestHandler; import org.opensearch.rest.BytesRestResponse; From bb2d8e49cfa9a6514c23f7499c1afbe4c7d5a8d8 Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Wed, 1 Mar 2023 16:22:12 -0800 Subject: [PATCH 24/38] Removed unnecessary IT Signed-off-by: Guian Gumpac --- .../sql/sql/JsonResponseTypeIT.java | 94 ------------------- 1 file changed, 94 deletions(-) delete mode 100644 integ-test/src/test/java/org/opensearch/sql/sql/JsonResponseTypeIT.java diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/JsonResponseTypeIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/JsonResponseTypeIT.java deleted file mode 100644 index f1b647191f..0000000000 --- a/integ-test/src/test/java/org/opensearch/sql/sql/JsonResponseTypeIT.java +++ /dev/null @@ -1,94 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.sql; - -import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_CALCS; -import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_DOG; -import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_PEOPLE2; - -import org.json.JSONArray; -import org.json.JSONObject; -import org.junit.Assert; -import org.junit.Test; -import org.opensearch.sql.legacy.SQLIntegTestCase; - -public class JsonResponseTypeIT extends SQLIntegTestCase { - - @Override - protected void init() throws Exception { - loadIndex(Index.PEOPLE2); - loadIndex(Index.DOG); - loadIndex(Index.CALCS); - } - - // TODO: Verify JSON type response when using String and Date Functions and add test cases - // For example: 'select concat(str1, '!!!!!!!!') from calcs' might not be handled as expected. - // Also, handling of the date/time functions (for example: 'select now() from calcs) will need attention as well. - - @Test - public void selectTest() { - String responseAsString = - executeQuery(String.format("SELECT * FROM %s", TEST_INDEX_PEOPLE2), "json"); - JSONObject response = new JSONObject(responseAsString); - Assert.assertTrue(response.has("hits")); - Assert.assertTrue(response.has("_shards")); - Assert.assertTrue(response.has("took")); - Assert.assertTrue(response.has("timed_out")); - Assert.assertEquals(12, getTotalHits(response)); - } - - @Test - public void aggregationTest() { - final String aggregationsObjName = "aggregations"; - String responseAsString = - executeQuery(String.format("SELECT count(*), sum(age) FROM %s", TEST_INDEX_PEOPLE2), "json"); - JSONObject response = new JSONObject(responseAsString); - Assert.assertTrue(response.has(aggregationsObjName)); - Assert.assertEquals(12, - response.getJSONObject(aggregationsObjName).getJSONObject("count(*)").getInt("value")); - Assert.assertEquals(400, - response.getJSONObject(aggregationsObjName).getJSONObject("sum(age)").getInt("value")); - } - - @Test - public void groupByTest() { - String responseAsString = - executeQuery(String.format("SELECT count(*) FROM %s group by age", TEST_INDEX_DOG), "json"); - Assert.assertTrue(responseAsString.contains("\"aggregations\":{\"composite_buckets\":{\"after_key\":{\"age\":4}," - + "\"buckets\":[{\"key\":{\"age\":2},\"doc_count\":1,\"count(*)\":{\"value\":1}},{\"key\":{\"age\":4}," - + "\"doc_count\":1,\"count(*)\":{\"value\":1}}]}}")); - } - - @Test - public void selectWithoutWhereTest() { - String responseAsString = - executeQuery(String.format("SELECT count(*) FROM %s group by age", TEST_INDEX_DOG), "json"); - - Assert.assertTrue(responseAsString.contains("\"aggregations\":{\"composite_buckets\":{\"after_key\":{\"age\":4}," - + "\"buckets\":[{\"key\":{\"age\":2},\"doc_count\":1,\"count(*)\":{\"value\":1}},{\"key\":{\"age\":4}," - + "\"doc_count\":1,\"count(*)\":{\"value\":1}}]}}")); - } - - @Test - public void selectFromTwoDocumentsTest() { - // This query will fall back to V1, as V2 doesn't support Joins yet. - String responseAsString = - executeQuery(String.format("SELECT d.age, c.int0 FROM %s c JOIN %s d WHERE c.int1 > 2", - TEST_INDEX_CALCS, TEST_INDEX_DOG), "json"); - - JSONObject response = new JSONObject(responseAsString); - Assert.assertTrue(response.has("hits")); - - JSONArray hits = response.getJSONObject("hits").getJSONArray("hits"); - Assert.assertTrue(hits.length() > 0); - for (int i = 0; i < hits.length(); i++) { - JSONObject jsonObject = hits.getJSONObject(i); - Assert.assertTrue(jsonObject.getJSONObject("_source").keySet().contains("d.age")); - Assert.assertTrue(jsonObject.getJSONObject("_source").keySet().contains("c.int0")); - } - } - } - From ce02e9dc4e26b8069c5121a21e42b331a581d1fa Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Thu, 2 Mar 2023 15:09:58 -0800 Subject: [PATCH 25/38] Addressed PR feedback Signed-off-by: Guian Gumpac --- .../analysis/JsonSupportAnalysisContext.java | 15 ++++ .../sql/analysis/JsonSupportAnalyzer.java | 75 +++++++++++++++++++ .../org/opensearch/sql/sql/SQLService.java | 70 +---------------- .../opensearch/sql/sql/SQLServiceTest.java | 7 +- 4 files changed, 98 insertions(+), 69 deletions(-) create mode 100644 core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalysisContext.java create mode 100644 core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java diff --git a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalysisContext.java b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalysisContext.java new file mode 100644 index 0000000000..d266ffa9c9 --- /dev/null +++ b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalysisContext.java @@ -0,0 +1,15 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.analysis; + +import lombok.Getter; +import lombok.Setter; + +public class JsonSupportAnalysisContext { + @Getter + @Setter + boolean isJSONSupported = true; +} diff --git a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java new file mode 100644 index 0000000000..bdec41467a --- /dev/null +++ b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java @@ -0,0 +1,75 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.analysis; + +import org.apache.commons.lang3.StringUtils; +import org.opensearch.sql.ast.AbstractNodeVisitor; +import org.opensearch.sql.ast.Node; +import org.opensearch.sql.ast.expression.Alias; +import org.opensearch.sql.ast.expression.Cast; +import org.opensearch.sql.ast.expression.Function; +import org.opensearch.sql.ast.expression.Literal; +import org.opensearch.sql.ast.tree.Aggregation; +import org.opensearch.sql.ast.tree.Project; + +public class JsonSupportAnalyzer extends AbstractNodeVisitor { + @Override + public Boolean visit(Node node, JsonSupportAnalysisContext context) { + // A node is supported if all of its children are supported. + return node.getChild().stream().filter(c -> c.accept(this, context) != null) + .allMatch(c -> c.accept(this, context)); + } + + @Override + public Boolean visitChildren(Node node, JsonSupportAnalysisContext context) { + for (Node child : node.getChild()) { + if (!child.accept(this, context)) + return false; + } + return true; + } + + @Override + public Boolean visitFunction(Function node, JsonSupportAnalysisContext context) { + // queries with function calls are not supported. + return false; + } + + @Override + public Boolean visitLiteral(Literal node, JsonSupportAnalysisContext context) { + // queries with literal values are not supported + return false; + } + + @Override + public Boolean visitCast(Cast node, JsonSupportAnalysisContext context) { + // Queries with cast are not supported + return false; + } + + @Override + public Boolean visitAlias(Alias node, JsonSupportAnalysisContext context) { + // Alias node is accepted if it does not have a user-defined alias + // and if the delegated expression is accepted. + if (!StringUtils.isEmpty(node.getAlias())) + return false; + else { + return node.getDelegated().accept(this, context); + } + } + + @Override + public Boolean visitProject(Project node, JsonSupportAnalysisContext context) { + return visit(node, context) + && node.getProjectList().stream().filter(c -> c.accept(this, context) != null) + .allMatch(e -> e.accept(this, context)); + } + + @Override + public Boolean visitAggregation(Aggregation node, JsonSupportAnalysisContext context) { + return node.getGroupExprList().isEmpty(); + } +} diff --git a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java index 64b6bd4fd9..b4c59748b6 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java +++ b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java @@ -13,6 +13,8 @@ import lombok.Setter; import org.antlr.v4.runtime.tree.ParseTree; import org.apache.commons.lang3.StringUtils; +import org.opensearch.sql.analysis.JsonSupportAnalysisContext; +import org.opensearch.sql.analysis.JsonSupportAnalyzer; import org.opensearch.sql.ast.AbstractNodeVisitor; import org.opensearch.sql.ast.Node; import org.opensearch.sql.ast.expression.Alias; @@ -94,72 +96,8 @@ private AbstractPlan plan( // There is no full support for JSON format yet for in memory operations, aliases, literals, and casts // Aggregation has differences with legacy results if (request.format().getFormatName().equals("json")) { - class VisitorContext { - @Getter - @Setter - boolean isJSONSupported = true; - } - - class JsonSupportedVisitor extends AbstractNodeVisitor { - @Override - public Boolean visit(Node node, VisitorContext context) { - // A node is supported if all of its children are supported. - return node.getChild().stream().filter(c -> c.accept(this, context) != null) - .allMatch(c -> c.accept(this, context)); - } - - @Override - public Boolean visitProject(Project node, VisitorContext context) { - // Overridden visit functions are done in memory and are not supported with json format - class UnsupportedExpressionsVisitor - extends AbstractNodeVisitor { - - @Override - public Boolean visitFunction(Function node, Object context) { - // queries with function calls are not supported. - return false; - } - - @Override - public Boolean visitLiteral(Literal node, Object context) { - // queries with literal values are not supported - return false; - } - - @Override - public Boolean visitCast(Cast node, Object context) { - // Queries with cast are not supported - return false; - } - - @Override - public Boolean visitAlias(Alias node, Object context) { - // Alias node is accepted if it does not have a user-defined alias - // and if the delegated expression is accepted. - if (!StringUtils.isEmpty(node.getAlias()))// && node.getDelegated().accept(this, context)) - return false; - else { - return node.getDelegated().accept(this, context); - } - } - } - - // Project node is supported if all of its children are supported and - // all expressions in project list are supported. - UnsupportedExpressionsVisitor unsupportedExpressionsVisitor = new UnsupportedExpressionsVisitor(); - return visit(node, context) - && node.getProjectList().stream().filter(c -> c.accept(unsupportedExpressionsVisitor, context) != null) - .allMatch(e -> e.accept(unsupportedExpressionsVisitor, context)); - - } - - @Override - public Boolean visitAggregation(Aggregation node, VisitorContext context) { - return node.getGroupExprList().isEmpty(); - } - } - - Boolean isJsonSupported = ((Query) statement).getPlan().accept(new JsonSupportedVisitor(), new VisitorContext()); + Boolean isJsonSupported = ((Query) statement).getPlan() + .accept(new JsonSupportAnalyzer(), new JsonSupportAnalysisContext()); if (!isJsonSupported) { throw new UnsupportedOperationException( diff --git a/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java b/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java index 1dbedcea83..1758d58526 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java @@ -7,6 +7,7 @@ package org.opensearch.sql.sql; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; @@ -118,7 +119,7 @@ public void onResponse(QueryResponse response) { @Override public void onFailure(Exception e) { - assert(e instanceof UnsupportedOperationException); + assertTrue(e instanceof UnsupportedOperationException); } }); } @@ -135,7 +136,7 @@ public void onResponse(QueryResponse response) { @Override public void onFailure(Exception e) { - assert(e instanceof UnsupportedOperationException); + assertTrue(e instanceof UnsupportedOperationException); } }); } @@ -152,7 +153,7 @@ public void onResponse(QueryResponse response) { @Override public void onFailure(Exception e) { - assert(e instanceof UnsupportedOperationException); + assertTrue(e instanceof UnsupportedOperationException); } }); } From dbe5067da74c93464b246aacd57a9b7f671c23ff Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Thu, 2 Mar 2023 16:15:38 -0800 Subject: [PATCH 26/38] Removed unnecessary context Signed-off-by: Guian Gumpac --- .../analysis/JsonSupportAnalysisContext.java | 15 --------- .../sql/analysis/JsonSupportAnalyzer.java | 31 +++++++++---------- .../org/opensearch/sql/sql/SQLService.java | 15 +-------- 3 files changed, 16 insertions(+), 45 deletions(-) delete mode 100644 core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalysisContext.java diff --git a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalysisContext.java b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalysisContext.java deleted file mode 100644 index d266ffa9c9..0000000000 --- a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalysisContext.java +++ /dev/null @@ -1,15 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.analysis; - -import lombok.Getter; -import lombok.Setter; - -public class JsonSupportAnalysisContext { - @Getter - @Setter - boolean isJSONSupported = true; -} diff --git a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java index bdec41467a..28af1f7e98 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java @@ -15,61 +15,60 @@ import org.opensearch.sql.ast.tree.Aggregation; import org.opensearch.sql.ast.tree.Project; -public class JsonSupportAnalyzer extends AbstractNodeVisitor { +public class JsonSupportAnalyzer extends AbstractNodeVisitor { @Override - public Boolean visit(Node node, JsonSupportAnalysisContext context) { + public Boolean visit(Node node, Object context) { // A node is supported if all of its children are supported. - return node.getChild().stream().filter(c -> c.accept(this, context) != null) - .allMatch(c -> c.accept(this, context)); + return node.getChild().stream().filter(c -> c.accept(this, null) != null) + .allMatch(c -> c.accept(this, null)); } @Override - public Boolean visitChildren(Node node, JsonSupportAnalysisContext context) { + public Boolean visitChildren(Node node, Object context) { for (Node child : node.getChild()) { - if (!child.accept(this, context)) + if (!child.accept(this, null)) return false; } return true; } @Override - public Boolean visitFunction(Function node, JsonSupportAnalysisContext context) { + public Boolean visitFunction(Function node, Object context) { // queries with function calls are not supported. return false; } @Override - public Boolean visitLiteral(Literal node, JsonSupportAnalysisContext context) { + public Boolean visitLiteral(Literal node, Object context) { // queries with literal values are not supported return false; } @Override - public Boolean visitCast(Cast node, JsonSupportAnalysisContext context) { + public Boolean visitCast(Cast node, Object context) { // Queries with cast are not supported return false; } @Override - public Boolean visitAlias(Alias node, JsonSupportAnalysisContext context) { + public Boolean visitAlias(Alias node, Object context) { // Alias node is accepted if it does not have a user-defined alias // and if the delegated expression is accepted. if (!StringUtils.isEmpty(node.getAlias())) return false; else { - return node.getDelegated().accept(this, context); + return node.getDelegated().accept(this, null); } } @Override - public Boolean visitProject(Project node, JsonSupportAnalysisContext context) { - return visit(node, context) - && node.getProjectList().stream().filter(c -> c.accept(this, context) != null) - .allMatch(e -> e.accept(this, context)); + public Boolean visitProject(Project node, Object context) { + return visit(node, null) + && node.getProjectList().stream().allMatch(e -> e.accept(this, null)); } @Override - public Boolean visitAggregation(Aggregation node, JsonSupportAnalysisContext context) { + public Boolean visitAggregation(Aggregation node, Object context) { return node.getGroupExprList().isEmpty(); } } diff --git a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java index b4c59748b6..bdd6f14477 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java +++ b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java @@ -8,23 +8,11 @@ import java.util.Optional; -import lombok.Getter; import lombok.RequiredArgsConstructor; -import lombok.Setter; import org.antlr.v4.runtime.tree.ParseTree; -import org.apache.commons.lang3.StringUtils; -import org.opensearch.sql.analysis.JsonSupportAnalysisContext; import org.opensearch.sql.analysis.JsonSupportAnalyzer; -import org.opensearch.sql.ast.AbstractNodeVisitor; -import org.opensearch.sql.ast.Node; -import org.opensearch.sql.ast.expression.Alias; -import org.opensearch.sql.ast.expression.Cast; -import org.opensearch.sql.ast.expression.Function; -import org.opensearch.sql.ast.expression.Literal; import org.opensearch.sql.ast.statement.Query; import org.opensearch.sql.ast.statement.Statement; -import org.opensearch.sql.ast.tree.Aggregation; -import org.opensearch.sql.ast.tree.Project; import org.opensearch.sql.common.response.ResponseListener; import org.opensearch.sql.executor.ExecutionEngine.ExplainResponse; import org.opensearch.sql.executor.ExecutionEngine.QueryResponse; @@ -96,8 +84,7 @@ private AbstractPlan plan( // There is no full support for JSON format yet for in memory operations, aliases, literals, and casts // Aggregation has differences with legacy results if (request.format().getFormatName().equals("json")) { - Boolean isJsonSupported = ((Query) statement).getPlan() - .accept(new JsonSupportAnalyzer(), new JsonSupportAnalysisContext()); + Boolean isJsonSupported = ((Query) statement).getPlan().accept(new JsonSupportAnalyzer(), null); if (!isJsonSupported) { throw new UnsupportedOperationException( From 4ff206b12c81f5cb913bdffcecf707904755ebf8 Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Fri, 3 Mar 2023 13:54:00 -0800 Subject: [PATCH 27/38] Added fall back for Filter functions Signed-off-by: Guian Gumpac --- .../opensearch/sql/analysis/JsonSupportAnalyzer.java | 10 ++++++++-- .../main/java/org/opensearch/sql/sql/SQLService.java | 2 +- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java index 28af1f7e98..603dd63d96 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java @@ -13,14 +13,14 @@ import org.opensearch.sql.ast.expression.Function; import org.opensearch.sql.ast.expression.Literal; import org.opensearch.sql.ast.tree.Aggregation; +import org.opensearch.sql.ast.tree.Filter; import org.opensearch.sql.ast.tree.Project; public class JsonSupportAnalyzer extends AbstractNodeVisitor { @Override public Boolean visit(Node node, Object context) { // A node is supported if all of its children are supported. - return node.getChild().stream().filter(c -> c.accept(this, null) != null) - .allMatch(c -> c.accept(this, null)); + return node.getChild().stream().allMatch(c -> c.accept(this, null)); } @Override @@ -71,4 +71,10 @@ public Boolean visitProject(Project node, Object context) { public Boolean visitAggregation(Aggregation node, Object context) { return node.getGroupExprList().isEmpty(); } + + @Override + public Boolean visitFilter(Filter node, Object context) { + return visit(node, null) + && node.getCondition().accept(this, null); + } } diff --git a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java index bdd6f14477..178ef34d08 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java +++ b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java @@ -83,7 +83,7 @@ private AbstractPlan plan( // There is no full support for JSON format yet for in memory operations, aliases, literals, and casts // Aggregation has differences with legacy results - if (request.format().getFormatName().equals("json")) { + if (request.format().getFormatName().equals("json") && statement instanceof Query) { Boolean isJsonSupported = ((Query) statement).getPlan().accept(new JsonSupportAnalyzer(), null); if (!isJsonSupported) { From a31dc27512b5d52abe592106e6e7fb3afc628e3e Mon Sep 17 00:00:00 2001 From: Max Ksyunz Date: Fri, 3 Mar 2023 13:19:50 -0800 Subject: [PATCH 28/38] Address build failures (#1366) Xcontent classes were moved in this PR opensearch-project/OpenSearch#5902 causing builds to fail. --------- Signed-off-by: MaxKsyunz --- .../test/java/org/opensearch/sql/legacy/JSONRequestIT.java | 4 ++-- .../test/java/org/opensearch/sql/legacy/SourceFieldIT.java | 4 ++-- .../main/java/org/opensearch/sql/legacy/domain/Paramer.java | 2 +- .../org/opensearch/sql/legacy/domain/hints/HintFactory.java | 2 +- .../sql/legacy/executor/GetIndexRequestRestListener.java | 4 ++-- .../sql/legacy/executor/join/ElasticJoinExecutor.java | 2 +- .../opensearch/sql/legacy/executor/join/ElasticUtils.java | 6 +++--- .../java/org/opensearch/sql/legacy/query/QueryAction.java | 2 +- .../sql/legacy/query/join/JoinRequestBuilder.java | 4 ++-- .../legacy/query/join/NestedLoopsElasticRequestBuilder.java | 4 ++-- .../org/opensearch/sql/legacy/query/maker/AggMaker.java | 4 ++-- .../java/org/opensearch/sql/legacy/query/maker/Maker.java | 6 +++--- .../sql/legacy/query/multi/MultiQueryRequestBuilder.java | 4 ++-- .../legacy/query/planner/physical/node/scroll/Scroll.java | 1 - .../java/org/opensearch/sql/legacy/request/SqlRequest.java | 4 ++-- .../opensearch/sql/legacy/utils/JsonPrettyFormatter.java | 6 +++--- .../src/main/java/org/opensearch/sql/plugin/SQLPlugin.java | 2 +- .../opensearch/sql/plugin/rest/RestQuerySettingsAction.java | 4 ++-- 18 files changed, 32 insertions(+), 33 deletions(-) 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 5b7f9202bf..ec5972be78 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 @@ -17,9 +17,9 @@ import org.junit.Test; import org.opensearch.action.search.SearchResponse; import org.opensearch.common.xcontent.LoggingDeprecationHandler; -import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.common.xcontent.XContentFactory; -import org.opensearch.common.xcontent.XContentParser; +import org.opensearch.core.xcontent.NamedXContentRegistry; +import org.opensearch.core.xcontent.XContentParser; import org.opensearch.common.xcontent.XContentType; import org.opensearch.search.SearchHit; import org.opensearch.search.SearchHits; 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 91c8321c95..756cbcd064 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 @@ -15,9 +15,9 @@ import org.junit.Test; import org.opensearch.action.search.SearchResponse; import org.opensearch.common.xcontent.LoggingDeprecationHandler; -import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.common.xcontent.XContentFactory; -import org.opensearch.common.xcontent.XContentParser; +import org.opensearch.core.xcontent.NamedXContentRegistry; +import org.opensearch.core.xcontent.XContentParser; import org.opensearch.common.xcontent.XContentType; import org.opensearch.search.SearchHit; import org.opensearch.search.SearchHits; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/domain/Paramer.java b/legacy/src/main/java/org/opensearch/sql/legacy/domain/Paramer.java index b35ddcb0ed..5c2c84f9b8 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/domain/Paramer.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/domain/Paramer.java @@ -15,7 +15,7 @@ import java.util.List; import java.util.Map; import org.opensearch.common.Strings; -import org.opensearch.common.xcontent.ToXContent; +import org.opensearch.core.xcontent.ToXContent; import org.opensearch.index.query.MatchPhraseQueryBuilder; import org.opensearch.index.query.MatchQueryBuilder; import org.opensearch.index.query.MultiMatchQueryBuilder; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/domain/hints/HintFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/domain/hints/HintFactory.java index 59ecf2eba8..18c68d57ab 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/domain/hints/HintFactory.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/domain/hints/HintFactory.java @@ -14,8 +14,8 @@ import java.util.List; import java.util.Map; import org.opensearch.common.xcontent.LoggingDeprecationHandler; -import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.common.xcontent.yaml.YamlXContentParser; +import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.sql.legacy.exception.SqlParseException; /** diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/GetIndexRequestRestListener.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/GetIndexRequestRestListener.java index b930044345..ebd9648d5e 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/GetIndexRequestRestListener.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/GetIndexRequestRestListener.java @@ -17,8 +17,8 @@ import org.opensearch.cluster.metadata.MappingMetadata; import org.opensearch.common.collect.ImmutableOpenMap; import org.opensearch.common.settings.Settings; -import org.opensearch.common.xcontent.ToXContent; -import org.opensearch.common.xcontent.XContentBuilder; +import org.opensearch.core.xcontent.ToXContent; +import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.rest.BytesRestResponse; import org.opensearch.rest.RestChannel; import org.opensearch.rest.RestResponse; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticJoinExecutor.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticJoinExecutor.java index 811d444a7f..3bbfcff1dd 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticJoinExecutor.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticJoinExecutor.java @@ -23,7 +23,7 @@ import org.opensearch.common.document.DocumentField; import org.opensearch.common.text.Text; import org.opensearch.common.unit.TimeValue; -import org.opensearch.common.xcontent.XContentBuilder; +import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.index.mapper.MapperService; import org.opensearch.rest.BytesRestResponse; import org.opensearch.rest.RestChannel; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticUtils.java b/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticUtils.java index c3b03c653a..5a91184a09 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticUtils.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/executor/join/ElasticUtils.java @@ -6,7 +6,7 @@ package org.opensearch.sql.legacy.executor.join; -import static org.opensearch.common.xcontent.ToXContent.EMPTY_PARAMS; +import static org.opensearch.core.xcontent.ToXContent.EMPTY_PARAMS; import com.google.common.collect.ImmutableMap; import java.io.IOException; @@ -20,10 +20,10 @@ import org.opensearch.common.bytes.BytesReference; import org.opensearch.common.io.stream.BytesStreamOutput; import org.opensearch.common.unit.TimeValue; -import org.opensearch.common.xcontent.ToXContent.Params; -import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.common.xcontent.XContentType; +import org.opensearch.core.xcontent.ToXContent.Params; +import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.search.SearchHit; import org.opensearch.search.SearchHits; import org.opensearch.search.sort.FieldSortBuilder; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/QueryAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/QueryAction.java index 46a9dda1d7..7646639be4 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/QueryAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/QueryAction.java @@ -16,8 +16,8 @@ import org.opensearch.action.support.IndicesOptions; import org.opensearch.client.Client; import org.opensearch.common.xcontent.LoggingDeprecationHandler; -import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.common.xcontent.json.JsonXContentParser; +import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.index.query.QueryBuilders; import org.opensearch.search.collapse.CollapseBuilder; import org.opensearch.search.fetch.subphase.highlight.HighlightBuilder; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/join/JoinRequestBuilder.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/join/JoinRequestBuilder.java index b4ebbb2ad7..de6cc0cddd 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/join/JoinRequestBuilder.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/join/JoinRequestBuilder.java @@ -13,10 +13,10 @@ import org.opensearch.action.ActionResponse; import org.opensearch.action.search.MultiSearchRequest; import org.opensearch.common.bytes.BytesReference; -import org.opensearch.common.xcontent.ToXContent; -import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.common.xcontent.XContentType; +import org.opensearch.core.xcontent.ToXContent; +import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.sql.legacy.query.SqlElasticRequestBuilder; /** diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/join/NestedLoopsElasticRequestBuilder.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/join/NestedLoopsElasticRequestBuilder.java index 2e80c05ff2..6783441931 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/join/NestedLoopsElasticRequestBuilder.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/join/NestedLoopsElasticRequestBuilder.java @@ -11,10 +11,10 @@ import org.json.JSONObject; import org.json.JSONStringer; import org.opensearch.common.bytes.BytesReference; -import org.opensearch.common.xcontent.ToXContent; -import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.common.xcontent.XContentType; +import org.opensearch.core.xcontent.ToXContent; +import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.sql.legacy.domain.Condition; import org.opensearch.sql.legacy.domain.Where; import org.opensearch.sql.legacy.exception.SqlParseException; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/AggMaker.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/AggMaker.java index 87125721c0..c7ab574a38 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/AggMaker.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/AggMaker.java @@ -21,10 +21,10 @@ import org.opensearch.common.ParsingException; import org.opensearch.common.Strings; import org.opensearch.common.xcontent.LoggingDeprecationHandler; -import org.opensearch.common.xcontent.NamedXContentRegistry; -import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.json.JsonXContent; import org.opensearch.common.xcontent.json.JsonXContentParser; +import org.opensearch.core.xcontent.NamedXContentRegistry; +import org.opensearch.core.xcontent.XContentParser; import org.opensearch.geo.search.aggregations.bucket.geogrid.GeoHashGridAggregationBuilder; import org.opensearch.join.aggregations.JoinAggregationBuilders; import org.opensearch.script.Script; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/Maker.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/Maker.java index 951d87a2c8..409288503a 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/Maker.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/maker/Maker.java @@ -31,10 +31,10 @@ import org.opensearch.common.geo.builders.ShapeBuilder; import org.opensearch.common.geo.parsers.ShapeParser; import org.opensearch.common.xcontent.LoggingDeprecationHandler; -import org.opensearch.common.xcontent.NamedXContentRegistry; -import org.opensearch.common.xcontent.ToXContent; -import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.json.JsonXContent; +import org.opensearch.core.xcontent.NamedXContentRegistry; +import org.opensearch.core.xcontent.ToXContent; +import org.opensearch.core.xcontent.XContentParser; import org.opensearch.index.query.BoolQueryBuilder; import org.opensearch.index.query.GeoPolygonQueryBuilder; import org.opensearch.index.query.MatchNoneQueryBuilder; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/multi/MultiQueryRequestBuilder.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/multi/MultiQueryRequestBuilder.java index 4b5448ac30..1b20fbe3fb 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/multi/MultiQueryRequestBuilder.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/multi/MultiQueryRequestBuilder.java @@ -16,10 +16,10 @@ import org.opensearch.action.ActionResponse; import org.opensearch.action.search.SearchRequestBuilder; import org.opensearch.common.bytes.BytesReference; -import org.opensearch.common.xcontent.ToXContent; -import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.common.xcontent.XContentType; +import org.opensearch.core.xcontent.ToXContent; +import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.sql.legacy.domain.Field; import org.opensearch.sql.legacy.domain.Select; import org.opensearch.sql.legacy.query.SqlElasticRequestBuilder; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/scroll/Scroll.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/scroll/Scroll.java index b623e4c1f7..6dd8ff6e89 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/scroll/Scroll.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/scroll/Scroll.java @@ -15,7 +15,6 @@ import org.opensearch.client.Client; import org.opensearch.common.Strings; import org.opensearch.common.unit.TimeValue; -import org.opensearch.common.xcontent.MediaType; import org.opensearch.common.xcontent.XContentType; import org.opensearch.index.query.BoolQueryBuilder; import org.opensearch.index.query.QueryBuilder; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/request/SqlRequest.java b/legacy/src/main/java/org/opensearch/sql/legacy/request/SqlRequest.java index 14acb26b3c..605ef3c958 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/request/SqlRequest.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/request/SqlRequest.java @@ -12,10 +12,10 @@ import org.json.JSONObject; import org.opensearch.common.settings.Settings; import org.opensearch.common.xcontent.LoggingDeprecationHandler; -import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.common.xcontent.XContentFactory; -import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.XContentType; +import org.opensearch.core.xcontent.NamedXContentRegistry; +import org.opensearch.core.xcontent.XContentParser; import org.opensearch.index.query.BoolQueryBuilder; import org.opensearch.search.SearchModule; import org.opensearch.sql.legacy.exception.SqlParseException; diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/utils/JsonPrettyFormatter.java b/legacy/src/main/java/org/opensearch/sql/legacy/utils/JsonPrettyFormatter.java index b94ef06c25..24cc47b0c0 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/utils/JsonPrettyFormatter.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/utils/JsonPrettyFormatter.java @@ -9,11 +9,11 @@ import java.io.IOException; import org.opensearch.common.Strings; import org.opensearch.common.xcontent.LoggingDeprecationHandler; -import org.opensearch.common.xcontent.NamedXContentRegistry; -import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentFactory; -import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.XContentType; +import org.opensearch.core.xcontent.NamedXContentRegistry; +import org.opensearch.core.xcontent.XContentBuilder; +import org.opensearch.core.xcontent.XContentParser; /** * Utility Class for formatting Json string pretty. diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java b/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java index 98826c6f62..2c401af352 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/SQLPlugin.java @@ -40,7 +40,7 @@ import org.opensearch.common.settings.Settings; import org.opensearch.common.settings.SettingsFilter; import org.opensearch.common.util.concurrent.OpenSearchExecutors; -import org.opensearch.common.xcontent.NamedXContentRegistry; +import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.env.Environment; import org.opensearch.env.NodeEnvironment; import org.opensearch.plugins.ActionPlugin; diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java index 59518e7a9d..6249cfba6c 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/rest/RestQuerySettingsAction.java @@ -19,10 +19,10 @@ import org.opensearch.client.node.NodeClient; import org.opensearch.common.Strings; import org.opensearch.common.settings.Settings; -import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentFactory; -import org.opensearch.common.xcontent.XContentParser; import org.opensearch.common.xcontent.XContentType; +import org.opensearch.core.xcontent.XContentBuilder; +import org.opensearch.core.xcontent.XContentParser; import org.opensearch.rest.BaseRestHandler; import org.opensearch.rest.BytesRestResponse; import org.opensearch.rest.RestController; From bee2102f5ceb88ce4047af185e90d4f85bcc9ae6 Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Mon, 6 Mar 2023 08:53:07 -0800 Subject: [PATCH 29/38] Fixed checkstyle errors Signed-off-by: Guian Gumpac --- .../opensearch/sql/analysis/JsonSupportAnalyzer.java | 7 ++++--- .../main/java/org/opensearch/sql/sql/SQLService.java | 11 ++++++----- .../org/opensearch/sql/sql/antlr/SQLSyntaxParser.java | 8 +++++++- .../java/org/opensearch/sql/sql/SQLServiceTest.java | 9 +++++---- 4 files changed, 22 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java index 603dd63d96..4ad13769d5 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java @@ -26,8 +26,9 @@ public Boolean visit(Node node, Object context) { @Override public Boolean visitChildren(Node node, Object context) { for (Node child : node.getChild()) { - if (!child.accept(this, null)) + if (!child.accept(this, null)) { return false; + } } return true; } @@ -54,9 +55,9 @@ public Boolean visitCast(Cast node, Object context) { public Boolean visitAlias(Alias node, Object context) { // Alias node is accepted if it does not have a user-defined alias // and if the delegated expression is accepted. - if (!StringUtils.isEmpty(node.getAlias())) + if (!StringUtils.isEmpty(node.getAlias())) { return false; - else { + } else { return node.getDelegated().accept(this, null); } } diff --git a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java index 178ef34d08..895d03f9fd 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java +++ b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java @@ -7,7 +7,6 @@ package org.opensearch.sql.sql; import java.util.Optional; - import lombok.RequiredArgsConstructor; import org.antlr.v4.runtime.tree.ParseTree; import org.opensearch.sql.analysis.JsonSupportAnalyzer; @@ -68,8 +67,9 @@ private AbstractPlan plan( SQLQueryRequest request, Optional> queryListener, Optional> explainListener) { - if (parser.containsHints(request.getQuery())) + if (parser.containsHints(request.getQuery())) { throw new UnsupportedOperationException("Hints are not yet supported in the new engine."); + } // 1.Parse query and convert parse tree (CST) to abstract syntax tree (AST) ParseTree cst = parser.parse(request.getQuery()); @@ -81,10 +81,11 @@ private AbstractPlan plan( .isExplain(request.isExplainRequest()) .build())); - // There is no full support for JSON format yet for in memory operations, aliases, literals, and casts - // Aggregation has differences with legacy results + // There is no full support for JSON format yet for in memory operations, aliases, literals, + // and casts. Aggregation has differences with legacy results if (request.format().getFormatName().equals("json") && statement instanceof Query) { - Boolean isJsonSupported = ((Query) statement).getPlan().accept(new JsonSupportAnalyzer(), null); + Boolean isJsonSupported = ((Query) statement).getPlan() + .accept(new JsonSupportAnalyzer(), null); if (!isJsonSupported) { throw new UnsupportedOperationException( diff --git a/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java b/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java index 8ffba7d097..7e1f551b3a 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java +++ b/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java @@ -32,9 +32,15 @@ public ParseTree parse(String query) { return parser.root(); } + /** + * Checks if the query contains hints as it is not yet support in V2. + * @param query a SQL query + * @return boolean value if query contains hints + */ public boolean containsHints(String query) { OpenSearchSQLLexer lexer = new OpenSearchSQLLexer(new CaseInsensitiveCharStream(query)); - OpenSearchSQLParser hintsParser = new OpenSearchSQLParser(new CommonTokenStream(lexer, OpenSearchSQLLexer.SQLCOMMENT)); + OpenSearchSQLParser hintsParser = new OpenSearchSQLParser( + new CommonTokenStream(lexer, OpenSearchSQLLexer.SQLCOMMENT)); return hintsParser.root().getChildCount() > 1; } } diff --git a/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java b/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java index 1758d58526..801376a718 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java @@ -110,7 +110,8 @@ public void onFailure(Exception e) { @Test public void canThrowUnsupportedExceptionForAggregationJsonQuery() { sqlService.execute( - new SQLQueryRequest(new JSONObject(), "SELECT FIELD FROM TABLE GROUP BY FIELD", QUERY, "json"), + new SQLQueryRequest(new JSONObject(), + "SELECT FIELD FROM TABLE GROUP BY FIELD", QUERY, "json"), new ResponseListener() { @Override public void onResponse(QueryResponse response) { @@ -170,7 +171,7 @@ public void onResponse(QueryResponse response) { @Override public void onFailure(Exception e) { - assert(e instanceof UnsupportedOperationException); + assert (e instanceof UnsupportedOperationException); } }); } @@ -187,7 +188,7 @@ public void onResponse(QueryResponse response) { @Override public void onFailure(Exception e) { - assert(e instanceof UnsupportedOperationException); + assert (e instanceof UnsupportedOperationException); } }); } @@ -227,7 +228,7 @@ public void onResponse(QueryResponse response) { @Override public void onFailure(Exception e) { - assert(e instanceof UnsupportedOperationException); + assert (e instanceof UnsupportedOperationException); } }); } From 74f88ae574b06eb63778a511830e0c148cda8539 Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Tue, 7 Mar 2023 08:55:12 -0800 Subject: [PATCH 30/38] Addressed PR comments and fixed the visitor Signed-off-by: Guian Gumpac --- .../sql/analysis/JsonSupportAnalyzer.java | 81 ------------- .../sql/analysis/JsonSupportVisitor.java | 106 ++++++++++++++++++ .../org/opensearch/sql/sql/SQLService.java | 16 +-- .../sql/sql/antlr/SQLSyntaxParser.java | 4 +- .../sql/sql/antlr/SQLSyntaxParserTest.java | 8 +- 5 files changed, 116 insertions(+), 99 deletions(-) delete mode 100644 core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java create mode 100644 core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java diff --git a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java deleted file mode 100644 index 4ad13769d5..0000000000 --- a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportAnalyzer.java +++ /dev/null @@ -1,81 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.analysis; - -import org.apache.commons.lang3.StringUtils; -import org.opensearch.sql.ast.AbstractNodeVisitor; -import org.opensearch.sql.ast.Node; -import org.opensearch.sql.ast.expression.Alias; -import org.opensearch.sql.ast.expression.Cast; -import org.opensearch.sql.ast.expression.Function; -import org.opensearch.sql.ast.expression.Literal; -import org.opensearch.sql.ast.tree.Aggregation; -import org.opensearch.sql.ast.tree.Filter; -import org.opensearch.sql.ast.tree.Project; - -public class JsonSupportAnalyzer extends AbstractNodeVisitor { - @Override - public Boolean visit(Node node, Object context) { - // A node is supported if all of its children are supported. - return node.getChild().stream().allMatch(c -> c.accept(this, null)); - } - - @Override - public Boolean visitChildren(Node node, Object context) { - for (Node child : node.getChild()) { - if (!child.accept(this, null)) { - return false; - } - } - return true; - } - - @Override - public Boolean visitFunction(Function node, Object context) { - // queries with function calls are not supported. - return false; - } - - @Override - public Boolean visitLiteral(Literal node, Object context) { - // queries with literal values are not supported - return false; - } - - @Override - public Boolean visitCast(Cast node, Object context) { - // Queries with cast are not supported - return false; - } - - @Override - public Boolean visitAlias(Alias node, Object context) { - // Alias node is accepted if it does not have a user-defined alias - // and if the delegated expression is accepted. - if (!StringUtils.isEmpty(node.getAlias())) { - return false; - } else { - return node.getDelegated().accept(this, null); - } - } - - @Override - public Boolean visitProject(Project node, Object context) { - return visit(node, null) - && node.getProjectList().stream().allMatch(e -> e.accept(this, null)); - } - - @Override - public Boolean visitAggregation(Aggregation node, Object context) { - return node.getGroupExprList().isEmpty(); - } - - @Override - public Boolean visitFilter(Filter node, Object context) { - return visit(node, null) - && node.getCondition().accept(this, null); - } -} diff --git a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java new file mode 100644 index 0000000000..c8081677d6 --- /dev/null +++ b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java @@ -0,0 +1,106 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.analysis; + +import org.apache.commons.lang3.StringUtils; +import org.opensearch.sql.ast.AbstractNodeVisitor; +import org.opensearch.sql.ast.Node; +import org.opensearch.sql.ast.expression.Alias; +import org.opensearch.sql.ast.expression.Cast; +import org.opensearch.sql.ast.expression.Function; +import org.opensearch.sql.ast.expression.Literal; +import org.opensearch.sql.ast.tree.Aggregation; +import org.opensearch.sql.ast.tree.Filter; +import org.opensearch.sql.ast.tree.Project; + +public class JsonSupportVisitor extends AbstractNodeVisitor { + @Override + public Boolean visit(Node node, Object context) { + // A node is supported if all of its children are supported. + return node.getChild().stream().allMatch(c -> c.accept(this, null)); + } + + @Override + public Boolean visitChildren(Node node, Object context) { + for (Node child : node.getChild()) { + child.accept(this, null); + } + return Boolean.TRUE; + } + + @Override + public Boolean visitAggregation(Aggregation node, Object context) { + if (node.getGroupExprList().isEmpty()) { + return Boolean.TRUE; + } else { + throw new UnsupportedOperationException( + "Queries with aggregation are not yet supported with json format in the new engine"); + } + } + + @Override + public Boolean visitFilter(Filter node, Object context) { + return visit(node, null) + && node.getCondition().accept(this, null); + } + + @Override + public Boolean visitProject(Project node, Object context) { + // Overridden visit functions are done in memory and are not supported with json format + class UnsupportedProjectVisitor + extends AbstractNodeVisitor { + @Override + public Boolean visit(Node node, Object context) { + // A node is supported if all of its children are supported. + return node.getChild().stream().allMatch(c -> c.accept(this, null)); + } + + @Override + public Boolean visitChildren(Node node, Object context) { + for (Node child : node.getChild()) { + child.accept(this, null); + } + return Boolean.TRUE; + } + + @Override + public Boolean visitFunction(Function node, Object context) { + // queries with function calls are not supported. + throw new UnsupportedOperationException( + "Queries with functions are not yet supported with json format in the new engine"); + } + + @Override + public Boolean visitLiteral(Literal node, Object context) { + // queries with literal values are not supported + throw new UnsupportedOperationException( + "Queries with literals are not yet supported with json format in the new engine"); + } + + @Override + public Boolean visitCast(Cast node, Object context) { + // Queries with cast are not supported + throw new UnsupportedOperationException( + "Queries with casts are not yet supported with json format in the new engine"); + } + + @Override + public Boolean visitAlias(Alias node, Object context) { + // Alias node is accepted if it does not have a user-defined alias + // and if the delegated expression is accepted. + if (StringUtils.isEmpty(node.getAlias())) { + return node.getDelegated().accept(this, null); + } else { + throw new UnsupportedOperationException( + "Queries with aliases are not yet supported with json format in the new engine"); + } + } + } + + return visit(node, null) && node.getProjectList().stream() + .allMatch(e -> e.accept(new UnsupportedProjectVisitor(), context)); + } +} diff --git a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java index 895d03f9fd..82a9fe4a10 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java +++ b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java @@ -9,7 +9,7 @@ import java.util.Optional; import lombok.RequiredArgsConstructor; import org.antlr.v4.runtime.tree.ParseTree; -import org.opensearch.sql.analysis.JsonSupportAnalyzer; +import org.opensearch.sql.analysis.JsonSupportVisitor; import org.opensearch.sql.ast.statement.Query; import org.opensearch.sql.ast.statement.Statement; import org.opensearch.sql.common.response.ResponseListener; @@ -67,7 +67,7 @@ private AbstractPlan plan( SQLQueryRequest request, Optional> queryListener, Optional> explainListener) { - if (parser.containsHints(request.getQuery())) { + if (parser.parseHints(request.getQuery()).getChildCount() > 1) { throw new UnsupportedOperationException("Hints are not yet supported in the new engine."); } @@ -82,16 +82,10 @@ private AbstractPlan plan( .build())); // There is no full support for JSON format yet for in memory operations, aliases, literals, - // and casts. Aggregation has differences with legacy results + // and casts. Aggregation has differences with legacy results. if (request.format().getFormatName().equals("json") && statement instanceof Query) { - Boolean isJsonSupported = ((Query) statement).getPlan() - .accept(new JsonSupportAnalyzer(), null); - - if (!isJsonSupported) { - throw new UnsupportedOperationException( - "Queries with aggregation and in memory operations (cast, literals, alias, math, etc.)" - + " are not yet supported with json format in the new engine"); - } + // Go through the tree and throw exceptions when unsupported + ((Query) statement).getPlan().accept(new JsonSupportVisitor(), null); } return queryExecutionFactory.create(statement, queryListener, explainListener); diff --git a/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java b/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java index 7e1f551b3a..e8cd9f6909 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java +++ b/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java @@ -37,10 +37,10 @@ public ParseTree parse(String query) { * @param query a SQL query * @return boolean value if query contains hints */ - public boolean containsHints(String query) { + public ParseTree parseHints(String query) { OpenSearchSQLLexer lexer = new OpenSearchSQLLexer(new CaseInsensitiveCharStream(query)); OpenSearchSQLParser hintsParser = new OpenSearchSQLParser( new CommonTokenStream(lexer, OpenSearchSQLLexer.SQLCOMMENT)); - return hintsParser.root().getChildCount() > 1; + return hintsParser.root(); } } diff --git a/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java b/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java index 007f8ca317..25779f8179 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java @@ -7,10 +7,8 @@ package org.opensearch.sql.sql.antlr; import static org.junit.jupiter.api.Assertions.assertAll; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Streams; @@ -636,9 +634,9 @@ public void canParseMultiMatchAlternateSyntax() { } @Test - public void containHintsTest() { - assertTrue(parser.containsHints("SELECT /*! HINTS */ field FROM test")); - assertFalse(parser.containsHints("SELECT field FROM test")); + public void canParseHints() { + assertNotNull(parser.parseHints("SELECT /*! HINTS */ field FROM test")); + assertNotNull(parser.parseHints("SELECT field FROM test")); } private static Stream matchPhraseQueryComplexQueries() { From 6785f2358a23b798aff79b7e7a623c73d1fe041b Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Tue, 7 Mar 2023 09:06:18 -0800 Subject: [PATCH 31/38] Added comment to visitor class Signed-off-by: Guian Gumpac --- .../org/opensearch/sql/analysis/JsonSupportVisitor.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java index c8081677d6..b9fb346232 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java +++ b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java @@ -16,6 +16,12 @@ import org.opensearch.sql.ast.tree.Filter; import org.opensearch.sql.ast.tree.Project; +/** + * This visitor's sole purpose is to throw UnsupportedOperationExceptions + * for unsupported features in the new engine when JSON format is specified. + * Unsupported features in V2 are ones the produce results that differ from + * legacy results. + */ public class JsonSupportVisitor extends AbstractNodeVisitor { @Override public Boolean visit(Node node, Object context) { From 9e733aa44455b8dfbafca46dbb888f8ba2abd855 Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Thu, 9 Mar 2023 15:01:55 -0800 Subject: [PATCH 32/38] Addressed PR comments to improve visitor class Signed-off-by: Guian Gumpac --- .../sql/analysis/JsonSupportVisitor.java | 132 ++++++++++-------- .../analysis/JsonSupportVisitorContext.java | 22 +++ .../org/opensearch/sql/sql/SQLService.java | 14 +- 3 files changed, 104 insertions(+), 64 deletions(-) create mode 100644 core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitorContext.java diff --git a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java index b9fb346232..b5581a460d 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java +++ b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java @@ -22,91 +22,105 @@ * Unsupported features in V2 are ones the produce results that differ from * legacy results. */ -public class JsonSupportVisitor extends AbstractNodeVisitor { +public class JsonSupportVisitor extends AbstractNodeVisitor { @Override - public Boolean visit(Node node, Object context) { - // A node is supported if all of its children are supported. - return node.getChild().stream().allMatch(c -> c.accept(this, null)); + public Boolean visit(Node node, JsonSupportVisitorContext context) { + return visitChildren(node, context); } @Override - public Boolean visitChildren(Node node, Object context) { + public Boolean visitChildren(Node node, JsonSupportVisitorContext context) { for (Node child : node.getChild()) { - child.accept(this, null); + if (!child.accept(this, context)) { + return Boolean.FALSE; + } } return Boolean.TRUE; } @Override - public Boolean visitAggregation(Aggregation node, Object context) { + public Boolean visitAggregation(Aggregation node, JsonSupportVisitorContext context) { if (node.getGroupExprList().isEmpty()) { return Boolean.TRUE; } else { - throw new UnsupportedOperationException( - "Queries with aggregation are not yet supported with json format in the new engine"); + context.setUnsupportedOperationException(new UnsupportedOperationException( + "Queries with aggregation are not yet supported with json format in the new engine")); + return Boolean.FALSE; } } @Override - public Boolean visitFilter(Filter node, Object context) { - return visit(node, null) - && node.getCondition().accept(this, null); + public Boolean visitFilter(Filter node, JsonSupportVisitorContext context) { + return visit(node, context) + && node.getCondition().accept(this, context); } @Override - public Boolean visitProject(Project node, Object context) { - // Overridden visit functions are done in memory and are not supported with json format - class UnsupportedProjectVisitor - extends AbstractNodeVisitor { - @Override - public Boolean visit(Node node, Object context) { - // A node is supported if all of its children are supported. - return node.getChild().stream().allMatch(c -> c.accept(this, null)); - } + public Boolean visitFunction(Function node, JsonSupportVisitorContext context) { + // Supported if outside of Project + if (!context.isVisitingProject()) { + return Boolean.TRUE; + } - @Override - public Boolean visitChildren(Node node, Object context) { - for (Node child : node.getChild()) { - child.accept(this, null); - } - return Boolean.TRUE; - } + // queries with function calls are not supported. + context.setUnsupportedOperationException(new UnsupportedOperationException( + "Queries with functions are not yet supported with json format in the new engine")); + return Boolean.FALSE; + } - @Override - public Boolean visitFunction(Function node, Object context) { - // queries with function calls are not supported. - throw new UnsupportedOperationException( - "Queries with functions are not yet supported with json format in the new engine"); - } + @Override + public Boolean visitLiteral(Literal node, JsonSupportVisitorContext context) { + // Supported if outside of Project + if (!context.isVisitingProject()) { + return Boolean.TRUE; + } - @Override - public Boolean visitLiteral(Literal node, Object context) { - // queries with literal values are not supported - throw new UnsupportedOperationException( - "Queries with literals are not yet supported with json format in the new engine"); - } + // queries with literal values are not supported + context.setUnsupportedOperationException(new UnsupportedOperationException( + "Queries with literals are not yet supported with json format in the new engine")); + return Boolean.FALSE; + } - @Override - public Boolean visitCast(Cast node, Object context) { - // Queries with cast are not supported - throw new UnsupportedOperationException( - "Queries with casts are not yet supported with json format in the new engine"); - } + @Override + public Boolean visitCast(Cast node, JsonSupportVisitorContext context) { + // Supported if outside of Project + if (!context.isVisitingProject()) { + return Boolean.TRUE; + } - @Override - public Boolean visitAlias(Alias node, Object context) { - // Alias node is accepted if it does not have a user-defined alias - // and if the delegated expression is accepted. - if (StringUtils.isEmpty(node.getAlias())) { - return node.getDelegated().accept(this, null); - } else { - throw new UnsupportedOperationException( - "Queries with aliases are not yet supported with json format in the new engine"); - } - } + // Queries with cast are not supported + context.setUnsupportedOperationException(new UnsupportedOperationException( + "Queries with casts are not yet supported with json format in the new engine")); + return Boolean.FALSE; + } + + @Override + public Boolean visitAlias(Alias node, JsonSupportVisitorContext context) { + // Supported if outside of Project + if (!context.isVisitingProject()) { + return Boolean.TRUE; } - return visit(node, null) && node.getProjectList().stream() - .allMatch(e -> e.accept(new UnsupportedProjectVisitor(), context)); + // Alias node is accepted if it does not have a user-defined alias + // and if the delegated expression is accepted. + if (StringUtils.isEmpty(node.getAlias())) { + return node.getDelegated().accept(this, context); + } else { + context.setUnsupportedOperationException(new UnsupportedOperationException( + "Queries with aliases are not yet supported with json format in the new engine")); + return Boolean.FALSE; + } + } + + @Override + public Boolean visitProject(Project node, JsonSupportVisitorContext context) { + boolean isSupported = visit(node, context); + + context.setVisitingProject(true); + isSupported = isSupported ? node.getProjectList().stream() + .allMatch(e -> e.accept(this, context)) : Boolean.FALSE; + context.setVisitingProject(false); + + return isSupported; } } diff --git a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitorContext.java b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitorContext.java new file mode 100644 index 0000000000..9e5e892dbb --- /dev/null +++ b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitorContext.java @@ -0,0 +1,22 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.analysis; + +import lombok.Getter; +import lombok.Setter; + +/** + * The context used for JsonSupportVisitor. + */ +public class JsonSupportVisitorContext { + @Getter + @Setter + private boolean isVisitingProject = false; + + @Getter + @Setter + private UnsupportedOperationException unsupportedOperationException; +} diff --git a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java index 82a9fe4a10..4cdeea1c27 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java +++ b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java @@ -10,6 +10,7 @@ import lombok.RequiredArgsConstructor; import org.antlr.v4.runtime.tree.ParseTree; import org.opensearch.sql.analysis.JsonSupportVisitor; +import org.opensearch.sql.analysis.JsonSupportVisitorContext; import org.opensearch.sql.ast.statement.Query; import org.opensearch.sql.ast.statement.Statement; import org.opensearch.sql.common.response.ResponseListener; @@ -67,10 +68,6 @@ private AbstractPlan plan( SQLQueryRequest request, Optional> queryListener, Optional> explainListener) { - if (parser.parseHints(request.getQuery()).getChildCount() > 1) { - throw new UnsupportedOperationException("Hints are not yet supported in the new engine."); - } - // 1.Parse query and convert parse tree (CST) to abstract syntax tree (AST) ParseTree cst = parser.parse(request.getQuery()); Statement statement = @@ -84,8 +81,15 @@ private AbstractPlan plan( // There is no full support for JSON format yet for in memory operations, aliases, literals, // and casts. Aggregation has differences with legacy results. if (request.format().getFormatName().equals("json") && statement instanceof Query) { + if (parser.parseHints(request.getQuery()).getChildCount() > 1) { + throw new UnsupportedOperationException("Hints are not yet supported in the new engine."); + } + // Go through the tree and throw exceptions when unsupported - ((Query) statement).getPlan().accept(new JsonSupportVisitor(), null); + JsonSupportVisitorContext jsonSupportVisitorContext = new JsonSupportVisitorContext(); + if (!((Query) statement).getPlan().accept(new JsonSupportVisitor(), jsonSupportVisitorContext)) { + throw jsonSupportVisitorContext.getUnsupportedOperationException(); + } } return queryExecutionFactory.create(statement, queryListener, explainListener); From 232196d39ee1eab84d36032f4944e1739c46f078 Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Fri, 10 Mar 2023 09:57:10 -0800 Subject: [PATCH 33/38] Added unit tests for JsonSupportVisitor Signed-off-by: Guian Gumpac --- .../sql/analysis/JsonSupportVisitor.java | 7 -- .../sql/analysis/JsonSupportVisitorTest.java | 111 ++++++++++++++++++ .../org/opensearch/sql/sql/SQLService.java | 3 +- .../opensearch/sql/sql/SQLServiceTest.java | 91 +++----------- 4 files changed, 132 insertions(+), 80 deletions(-) create mode 100644 core/src/test/java/org/opensearch/sql/analysis/JsonSupportVisitorTest.java diff --git a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java index b5581a460d..64c7432ce9 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java +++ b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java @@ -13,7 +13,6 @@ import org.opensearch.sql.ast.expression.Function; import org.opensearch.sql.ast.expression.Literal; import org.opensearch.sql.ast.tree.Aggregation; -import org.opensearch.sql.ast.tree.Filter; import org.opensearch.sql.ast.tree.Project; /** @@ -49,12 +48,6 @@ public Boolean visitAggregation(Aggregation node, JsonSupportVisitorContext cont } } - @Override - public Boolean visitFilter(Filter node, JsonSupportVisitorContext context) { - return visit(node, context) - && node.getCondition().accept(this, context); - } - @Override public Boolean visitFunction(Function node, JsonSupportVisitorContext context) { // Supported if outside of Project diff --git a/core/src/test/java/org/opensearch/sql/analysis/JsonSupportVisitorTest.java b/core/src/test/java/org/opensearch/sql/analysis/JsonSupportVisitorTest.java new file mode 100644 index 0000000000..3f1db7952a --- /dev/null +++ b/core/src/test/java/org/opensearch/sql/analysis/JsonSupportVisitorTest.java @@ -0,0 +1,111 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.analysis; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.opensearch.sql.ast.dsl.AstDSL.qualifiedName; + +import com.google.common.collect.ImmutableList; +import java.util.Collections; +import org.junit.jupiter.api.Test; +import org.opensearch.sql.ast.dsl.AstDSL; +import org.opensearch.sql.ast.expression.Literal; +import org.opensearch.sql.ast.expression.UnresolvedExpression; +import org.opensearch.sql.ast.tree.UnresolvedPlan; + +class JsonSupportVisitorTest { + @Test + public void visitLiteralInProject() { + UnresolvedPlan project = AstDSL.project( + AstDSL.relation("table", "table"), + AstDSL.intLiteral(1)); + assertFalse(project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + } + + @Test + public void visitLiteralOutsideProject() { + Literal intLiteral = AstDSL.intLiteral(1); + assertTrue(intLiteral.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + } + + @Test + public void visitCastInProject() { + UnresolvedPlan project = AstDSL.project( + AstDSL.relation("table", "table"), + AstDSL.cast(AstDSL.intLiteral(1), AstDSL.stringLiteral("INT"))); + assertFalse(project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + } + + @Test + public void visitCastOutsideProject() { + UnresolvedExpression intCast = AstDSL.cast( + AstDSL.intLiteral(1), + AstDSL.stringLiteral("INT")); + assertTrue(intCast.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + } + + @Test + public void visitAliasInProject() { + UnresolvedPlan project = AstDSL.project( + AstDSL.relation("table", "table"), + AstDSL.alias("alias", AstDSL.intLiteral(1))); + assertFalse(project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + } + + @Test + public void visitAliasInProjectWithDelegated() { + UnresolvedPlan project = AstDSL.project( + AstDSL.relation("table", "table"), + AstDSL.alias("alias", AstDSL.intLiteral(1), "alias")); + assertFalse(project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + } + + @Test + public void visitAliasOutsideProject() { + UnresolvedExpression alias = AstDSL.alias("alias", AstDSL.intLiteral(1)); + assertTrue(alias.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + } + + @Test + public void visitFunctionInProject() { + UnresolvedPlan function = AstDSL.project( + AstDSL.relation("table", "table"), + AstDSL.function("abs", AstDSL.intLiteral(-1))); + assertFalse(function.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + } + + @Test + public void visitFunctionOutsideProject() { + UnresolvedExpression function = AstDSL.function("abs", AstDSL.intLiteral(-1)); + assertTrue(function.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + } + + @Test + public void visitAggregationWithGroupExprList() { + UnresolvedPlan projectAggr = AstDSL.project(AstDSL.agg( + AstDSL.relation("table", "table"), + Collections.emptyList(), + Collections.emptyList(), + ImmutableList.of(AstDSL.alias("alias", qualifiedName("integer_value"))), + Collections.emptyList())); + assertFalse(projectAggr.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + } + + @Test + public void visitAggregationWithAggExprList() { + UnresolvedPlan aggregation = AstDSL.agg( + AstDSL.relation("table", "table"), + ImmutableList.of( + AstDSL.alias("AVG(alias)", + AstDSL.aggregate("AVG", + qualifiedName("integer_value")))), + Collections.emptyList(), + Collections.emptyList(), + Collections.emptyList()); + assertTrue(aggregation.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + } +} diff --git a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java index 4cdeea1c27..cb0dd9b59c 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java +++ b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java @@ -87,7 +87,8 @@ private AbstractPlan plan( // Go through the tree and throw exceptions when unsupported JsonSupportVisitorContext jsonSupportVisitorContext = new JsonSupportVisitorContext(); - if (!((Query) statement).getPlan().accept(new JsonSupportVisitor(), jsonSupportVisitorContext)) { + if (!((Query) statement).getPlan() + .accept(new JsonSupportVisitor(), jsonSupportVisitorContext)) { throw jsonSupportVisitorContext.getUnsupportedOperationException(); } } diff --git a/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java b/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java index 801376a718..16dc6d781d 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java @@ -7,7 +7,6 @@ package org.opensearch.sql.sql; import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; @@ -85,7 +84,7 @@ public void onFailure(Exception e) { } @Test - public void canExecuteJsonFormatRequest() { + public void canExecuteCsvFormatRequest() { doAnswer(invocation -> { ResponseListener listener = invocation.getArgument(1); listener.onResponse(new QueryResponse(schema, Collections.emptyList(), "")); @@ -93,7 +92,7 @@ public void canExecuteJsonFormatRequest() { }).when(queryService).execute(any(), any()); sqlService.execute( - new SQLQueryRequest(new JSONObject(), "SELECT FIELD FROM TABLE", QUERY, "json"), + new SQLQueryRequest(new JSONObject(), "SELECT 123", QUERY, "csv"), new ResponseListener() { @Override public void onResponse(QueryResponse response) { @@ -108,44 +107,15 @@ public void onFailure(Exception e) { } @Test - public void canThrowUnsupportedExceptionForAggregationJsonQuery() { - sqlService.execute( - new SQLQueryRequest(new JSONObject(), - "SELECT FIELD FROM TABLE GROUP BY FIELD", QUERY, "json"), - new ResponseListener() { - @Override - public void onResponse(QueryResponse response) { - assertNotNull(response); - } - - @Override - public void onFailure(Exception e) { - assertTrue(e instanceof UnsupportedOperationException); - } - }); - } - - @Test - public void canThrowUnsupportedExceptionForAliasJsonQuery() { - sqlService.execute( - new SQLQueryRequest(new JSONObject(), "SELECT FIELD as FIELD FROM TABLE", QUERY, "json"), - new ResponseListener() { - @Override - public void onResponse(QueryResponse response) { - assertNotNull(response); - } - - @Override - public void onFailure(Exception e) { - assertTrue(e instanceof UnsupportedOperationException); - } - }); - } + public void canExecuteJsonFormatRequest() { + doAnswer(invocation -> { + ResponseListener listener = invocation.getArgument(1); + listener.onResponse(new QueryResponse(schema, Collections.emptyList(), "")); + return null; + }).when(queryService).execute(any(), any()); - @Test - public void canThrowUnsupportedExceptionForFunctionJsonQuery() { sqlService.execute( - new SQLQueryRequest(new JSONObject(), "SELECT 1 + 1", QUERY, "json"), + new SQLQueryRequest(new JSONObject(), "SELECT FIELD FROM TABLE", QUERY, "json"), new ResponseListener() { @Override public void onResponse(QueryResponse response) { @@ -154,15 +124,15 @@ public void onResponse(QueryResponse response) { @Override public void onFailure(Exception e) { - assertTrue(e instanceof UnsupportedOperationException); + fail(e); } }); } @Test - public void canThrowUnsupportedExceptionForLiteralJsonQuery() { + public void canThrowUnsupportedExceptionWithUnsupportedJsonQuery() { sqlService.execute( - new SQLQueryRequest(new JSONObject(), "SELECT 123", QUERY, "json"), + new SQLQueryRequest(new JSONObject(), "SELECT CAST(FIELD AS DOUBLE)", QUERY, "json"), new ResponseListener() { @Override public void onResponse(QueryResponse response) { @@ -177,9 +147,9 @@ public void onFailure(Exception e) { } @Test - public void canThrowUnsupportedExceptionForCastJsonQuery() { + public void canThrowUnsupportedExceptionForHintsQuery() { sqlService.execute( - new SQLQueryRequest(new JSONObject(), "SELECT CAST(FIELD AS DOUBLE)", QUERY, "json"), + new SQLQueryRequest(new JSONObject(), "SELECT /*! HINTS */ 123", QUERY, "json"), new ResponseListener() { @Override public void onResponse(QueryResponse response) { @@ -194,18 +164,12 @@ public void onFailure(Exception e) { } @Test - public void canExecuteCsvFormatRequest() { - doAnswer(invocation -> { - ResponseListener listener = invocation.getArgument(1); - listener.onResponse(new QueryResponse(schema, Collections.emptyList(), "")); - return null; - }).when(queryService).execute(any(), any()); - - sqlService.execute( - new SQLQueryRequest(new JSONObject(), "SELECT 123", QUERY, "csv"), - new ResponseListener() { + public void canExplainHintsQueryWithJsonFormat() { + sqlService.explain( + new SQLQueryRequest(new JSONObject(), "SELECT /*! HINTS */ 123", EXPLAIN, "json"), + new ResponseListener() { @Override - public void onResponse(QueryResponse response) { + public void onResponse(ExplainResponse response) { assertNotNull(response); } @@ -216,23 +180,6 @@ public void onFailure(Exception e) { }); } - @Test - public void canThrowUnsupportedExceptionForHintsQuery() { - sqlService.execute( - new SQLQueryRequest(new JSONObject(), "SELECT /*! HINTS */ 123", QUERY, "jdbc"), - new ResponseListener() { - @Override - public void onResponse(QueryResponse response) { - assertNotNull(response); - } - - @Override - public void onFailure(Exception e) { - assert (e instanceof UnsupportedOperationException); - } - }); - } - @Test public void canExplainSqlQuery() { doAnswer(invocation -> { From e0e2365ec55512d6f41996c17e95ff773ed59515 Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Fri, 10 Mar 2023 11:13:21 -0800 Subject: [PATCH 34/38] Added helper function for SQLServiceTest Signed-off-by: Guian Gumpac --- .../opensearch/sql/sql/SQLServiceTest.java | 88 +++++++------------ 1 file changed, 30 insertions(+), 58 deletions(-) diff --git a/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java b/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java index 16dc6d781d..cdb463f141 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/SQLServiceTest.java @@ -60,16 +60,9 @@ public void cleanup() throws InterruptedException { queryManager.awaitTermination(1, TimeUnit.SECONDS); } - @Test - public void canExecuteSqlQuery() { - doAnswer(invocation -> { - ResponseListener listener = invocation.getArgument(1); - listener.onResponse(new QueryResponse(schema, Collections.emptyList(), "")); - return null; - }).when(queryService).execute(any(), any()); - + private void execute(String query, String format) { sqlService.execute( - new SQLQueryRequest(new JSONObject(), "SELECT 123", QUERY, "jdbc"), + new SQLQueryRequest(new JSONObject(), query, QUERY, format), new ResponseListener() { @Override public void onResponse(QueryResponse response) { @@ -83,16 +76,9 @@ public void onFailure(Exception e) { }); } - @Test - public void canExecuteCsvFormatRequest() { - doAnswer(invocation -> { - ResponseListener listener = invocation.getArgument(1); - listener.onResponse(new QueryResponse(schema, Collections.emptyList(), "")); - return null; - }).when(queryService).execute(any(), any()); - + private void executeWithUnsupportedException(String query, String format) { sqlService.execute( - new SQLQueryRequest(new JSONObject(), "SELECT 123", QUERY, "csv"), + new SQLQueryRequest(new JSONObject(), query, QUERY, format), new ResponseListener() { @Override public void onResponse(QueryResponse response) { @@ -101,66 +87,52 @@ public void onResponse(QueryResponse response) { @Override public void onFailure(Exception e) { - fail(e); + assert (e instanceof UnsupportedOperationException); } }); } @Test - public void canExecuteJsonFormatRequest() { + public void canExecuteSqlQuery() { doAnswer(invocation -> { ResponseListener listener = invocation.getArgument(1); listener.onResponse(new QueryResponse(schema, Collections.emptyList(), "")); return null; }).when(queryService).execute(any(), any()); - sqlService.execute( - new SQLQueryRequest(new JSONObject(), "SELECT FIELD FROM TABLE", QUERY, "json"), - new ResponseListener() { - @Override - public void onResponse(QueryResponse response) { - assertNotNull(response); - } + execute("SELECT 123", "jdbc"); + } - @Override - public void onFailure(Exception e) { - fail(e); - } - }); + @Test + public void canExecuteCsvFormatRequest() { + doAnswer(invocation -> { + ResponseListener listener = invocation.getArgument(1); + listener.onResponse(new QueryResponse(schema, Collections.emptyList(), "")); + return null; + }).when(queryService).execute(any(), any()); + + execute("SELECT 123", "csv"); } @Test - public void canThrowUnsupportedExceptionWithUnsupportedJsonQuery() { - sqlService.execute( - new SQLQueryRequest(new JSONObject(), "SELECT CAST(FIELD AS DOUBLE)", QUERY, "json"), - new ResponseListener() { - @Override - public void onResponse(QueryResponse response) { - assertNotNull(response); - } + public void canExecuteJsonFormatRequest() { + doAnswer(invocation -> { + ResponseListener listener = invocation.getArgument(1); + listener.onResponse(new QueryResponse(schema, Collections.emptyList(), "")); + return null; + }).when(queryService).execute(any(), any()); - @Override - public void onFailure(Exception e) { - assert (e instanceof UnsupportedOperationException); - } - }); + execute("SELECT FIELD FROM TABLE", "json"); } @Test - public void canThrowUnsupportedExceptionForHintsQuery() { - sqlService.execute( - new SQLQueryRequest(new JSONObject(), "SELECT /*! HINTS */ 123", QUERY, "json"), - new ResponseListener() { - @Override - public void onResponse(QueryResponse response) { - assertNotNull(response); - } + public void canThrowUnsupportedExceptionWithUnsupportedJsonQuery() { + executeWithUnsupportedException("SELECT CAST(FIELD AS DOUBLE)", "json"); + } - @Override - public void onFailure(Exception e) { - assert (e instanceof UnsupportedOperationException); - } - }); + @Test + public void canThrowUnsupportedExceptionForHintsQuery() { + executeWithUnsupportedException("SELECT /*! HINTS */ 123", "json"); } @Test From b60da692f497b9461b04aa56ba82c81a35d01b8d Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Mon, 13 Mar 2023 09:27:47 -0700 Subject: [PATCH 35/38] Added expected failures Signed-off-by: Guian Gumpac --- .../opensearch/sql/legacy/DateFormatIT.java | 26 ++++++++++------ .../sql/legacy/QueryFunctionsIT.java | 5 +++- .../org/opensearch/sql/legacy/QueryIT.java | 30 ++++++++++++++----- 3 files changed, 43 insertions(+), 18 deletions(-) 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 a0b4b19898..1343558de9 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 @@ -28,6 +28,7 @@ import org.json.JSONObject; import org.junit.Ignore; import org.junit.Test; +import org.opensearch.client.ResponseException; import org.opensearch.sql.legacy.exception.SqlParseException; public class DateFormatIT extends SQLIntegTestCase { @@ -50,7 +51,8 @@ protected void init() throws Exception { * this ends up excluding some of the expected values causing the assertion to fail. LIMIT overrides this. */ - @Test + // DATE_FORMAT with timezone argument is not yet supported in V2 + @Test(expected = SqlParseException.class) public void equalTo() throws SqlParseException { assertThat( dateQuery( @@ -59,7 +61,8 @@ public void equalTo() throws SqlParseException { ); } - @Test + // DATE_FORMAT with timezone argument is not yet supported in V2 + @Test(expected = SqlParseException.class) public void lessThan() throws SqlParseException { assertThat( dateQuery( @@ -68,7 +71,8 @@ public void lessThan() throws SqlParseException { ); } - @Test + // DATE_FORMAT with timezone argument is not yet supported in V2 + @Test(expected = SqlParseException.class) public void lessThanOrEqualTo() throws SqlParseException { assertThat( dateQuery( @@ -79,7 +83,8 @@ public void lessThanOrEqualTo() throws SqlParseException { ); } - @Test + // DATE_FORMAT with timezone argument is not yet supported in V2 + @Test(expected = SqlParseException.class) public void greaterThan() throws SqlParseException { assertThat( dateQuery( @@ -88,7 +93,8 @@ public void greaterThan() throws SqlParseException { ); } - @Test + // DATE_FORMAT with timezone argument is not yet supported in V2 + @Test(expected = SqlParseException.class) public void greaterThanOrEqualTo() throws SqlParseException { assertThat( dateQuery( @@ -99,7 +105,8 @@ public void greaterThanOrEqualTo() throws SqlParseException { ); } - @Test + // DATE_FORMAT with timezone argument is not yet supported in V2 + @Test(expected = SqlParseException.class) public void and() throws SqlParseException { assertThat( dateQuery(SELECT_FROM + @@ -122,7 +129,8 @@ public void andWithDefaultTimeZone() throws SqlParseException { ); } - @Test + // DATE_FORMAT with timezone argument is not yet supported in V2 + @Test(expected = SqlParseException.class) public void or() throws SqlParseException { assertThat( dateQuery(SELECT_FROM + @@ -134,8 +142,8 @@ public void or() throws SqlParseException { ); } - - @Test + // DATE_FORMAT with timezone argument is not yet supported in V2 + @Test(expected = ResponseException.class) public void sortByDateFormat() throws IOException { // Sort by expression in descending order, but sort inside in ascending order, so we increase our confidence // that successful test isn't just random chance. 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 c59974a622..57a0a72278 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 @@ -28,6 +28,7 @@ import org.json.JSONObject; import org.junit.Test; import org.opensearch.action.search.SearchResponse; +import org.opensearch.client.ResponseException; import org.opensearch.common.xcontent.LoggingDeprecationHandler; import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.common.xcontent.XContentType; @@ -133,7 +134,9 @@ public void scoreQueryWithNestedField() throws IOException { ); } - @Test + // Specifying city.keyword is not supported in V2 + // Rework on data types is in progress and tracked with https://github.com/opensearch-project/sql/pull/1314 + @Test(expected = ResponseException.class) public void wildcardQuery() throws IOException { assertThat( query( 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 eeff107f15..679596f67f 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 @@ -102,7 +102,9 @@ public void multipleFromTest() throws IOException { Assert.assertEquals(14, getTotalHits(response)); } - @Test + // Duplicated column names like SELECT *, age is not yet supported in V2 + // Can be tracked with https://github.com/opensearch-project/sql/issues/785 + @Test(expected = ResponseException.class) public void selectAllWithFieldReturnsAll() throws IOException { JSONObject response = executeQuery(StringUtils.format( "SELECT *, age " + @@ -114,7 +116,9 @@ public void selectAllWithFieldReturnsAll() throws IOException { checkSelectAllAndFieldResponseSize(response); } - @Test + // Duplicated column names like SELECT *, age is not yet supported in V2 + // Can be tracked with https://github.com/opensearch-project/sql/issues/785 + @Test(expected = ResponseException.class) public void selectAllWithFieldReverseOrder() throws IOException { JSONObject response = executeQuery(StringUtils.format( "SELECT *, age " + @@ -126,7 +130,9 @@ public void selectAllWithFieldReverseOrder() throws IOException { checkSelectAllAndFieldResponseSize(response); } - @Test + // Duplicated column names like SELECT *, age is not yet supported in V2 + // Can be tracked with https://github.com/opensearch-project/sql/issues/785 + @Test(expected = ResponseException.class) public void selectAllWithMultipleFields() throws IOException { JSONObject response = executeQuery(StringUtils.format( "SELECT *, age, address " + @@ -138,7 +144,9 @@ public void selectAllWithMultipleFields() throws IOException { checkSelectAllAndFieldResponseSize(response); } - @Test + // Duplicated column names like SELECT *, age is not yet supported in V2 + // Can be tracked with https://github.com/opensearch-project/sql/issues/785 + @Test(expected = ResponseException.class) public void selectAllWithFieldAndOrderBy() throws IOException { JSONObject response = executeQuery(StringUtils.format( "SELECT *, age " + @@ -577,7 +585,7 @@ public void notBetweenTest() throws IOException { public void inTest() throws IOException { JSONObject response = executeQuery( String.format(Locale.ROOT, "SELECT age FROM %s WHERE age IN (20, 22) LIMIT 1000", - TestsConstants.TEST_INDEX_PHRASE)); + TestsConstants.TEST_INDEX_PEOPLE)); JSONArray hits = getHits(response); for (int i = 0; i < hits.length(); i++) { @@ -725,7 +733,9 @@ public void notInTest() throws IOException { } } - @Test + // Unsupported date format in V2 + // Can be tracked through https://github.com/opensearch-project/sql/issues/856 + @Test(expected = ResponseException.class) public void dateSearch() throws IOException { DateTimeFormatter formatter = DateTimeFormat.forPattern(TestsConstants.DATE_FORMAT); DateTime dateToCompare = new DateTime(2014, 8, 18, 0, 0, 0); @@ -770,7 +780,9 @@ public void dateSearchBraces() throws IOException { } } - @Test + // Unsupported date format in V2 + // Can be tracked through https://github.com/opensearch-project/sql/issues/856 + @Test(expected = ResponseException.class) public void dateBetweenSearch() throws IOException { DateTimeFormatter formatter = DateTimeFormat.forPattern(TestsConstants.DATE_FORMAT); @@ -1270,7 +1282,9 @@ public void queryWithDotAtStartOfIndexName() throws Exception { Assert.assertTrue(response.contains("PhD")); } - @Test + // Objects in IS NOT NULL not yet supported in V2 + // Can be tracked with https://github.com/opensearch-project/sql/issues/1425 + @Test(expected = ResponseException.class) public void notLikeTests() throws IOException { JSONObject response = executeQuery( String.format(Locale.ROOT, "SELECT name " + From c2cb0f3debf06073b68647fb0b1b2cb93193118a Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Mon, 13 Mar 2023 13:33:53 -0700 Subject: [PATCH 36/38] Reworked the visitor class to have type Void instead of Boolean Signed-off-by: Guian Gumpac --- .../sql/analysis/JsonSupportVisitor.java | 103 ++++++++---------- .../sql/analysis/JsonSupportVisitorTest.java | 42 ++++--- .../org/opensearch/sql/sql/SQLService.java | 5 +- 3 files changed, 74 insertions(+), 76 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java index 64c7432ce9..0155e1c56e 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java +++ b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitor.java @@ -21,99 +21,86 @@ * Unsupported features in V2 are ones the produce results that differ from * legacy results. */ -public class JsonSupportVisitor extends AbstractNodeVisitor { +public class JsonSupportVisitor extends AbstractNodeVisitor { @Override - public Boolean visit(Node node, JsonSupportVisitorContext context) { - return visitChildren(node, context); + public Void visit(Node node, JsonSupportVisitorContext context) { + visitChildren(node, context); + return null; } @Override - public Boolean visitChildren(Node node, JsonSupportVisitorContext context) { + public Void visitChildren(Node node, JsonSupportVisitorContext context) { for (Node child : node.getChild()) { - if (!child.accept(this, context)) { - return Boolean.FALSE; - } + child.accept(this, context); } - return Boolean.TRUE; + return null; } @Override - public Boolean visitAggregation(Aggregation node, JsonSupportVisitorContext context) { - if (node.getGroupExprList().isEmpty()) { - return Boolean.TRUE; - } else { - context.setUnsupportedOperationException(new UnsupportedOperationException( - "Queries with aggregation are not yet supported with json format in the new engine")); - return Boolean.FALSE; + public Void visitAggregation(Aggregation node, JsonSupportVisitorContext context) { + if (!node.getGroupExprList().isEmpty()) { + throw new UnsupportedOperationException( + "Queries with aggregation are not yet supported with json format in the new engine"); } + return null; } @Override - public Boolean visitFunction(Function node, JsonSupportVisitorContext context) { + public Void visitFunction(Function node, JsonSupportVisitorContext context) { // Supported if outside of Project - if (!context.isVisitingProject()) { - return Boolean.TRUE; + if (context.isVisitingProject()) { + // queries with function calls are not supported. + throw new UnsupportedOperationException( + "Queries with functions are not yet supported with json format in the new engine"); } - - // queries with function calls are not supported. - context.setUnsupportedOperationException(new UnsupportedOperationException( - "Queries with functions are not yet supported with json format in the new engine")); - return Boolean.FALSE; + return null; } @Override - public Boolean visitLiteral(Literal node, JsonSupportVisitorContext context) { + public Void visitLiteral(Literal node, JsonSupportVisitorContext context) { // Supported if outside of Project - if (!context.isVisitingProject()) { - return Boolean.TRUE; + if (context.isVisitingProject()) { + // queries with literal values are not supported + throw new UnsupportedOperationException( + "Queries with literals are not yet supported with json format in the new engine"); } - - // queries with literal values are not supported - context.setUnsupportedOperationException(new UnsupportedOperationException( - "Queries with literals are not yet supported with json format in the new engine")); - return Boolean.FALSE; + return null; } @Override - public Boolean visitCast(Cast node, JsonSupportVisitorContext context) { + public Void visitCast(Cast node, JsonSupportVisitorContext context) { // Supported if outside of Project - if (!context.isVisitingProject()) { - return Boolean.TRUE; + if (context.isVisitingProject()) { + // Queries with cast are not supported + throw new UnsupportedOperationException( + "Queries with casts are not yet supported with json format in the new engine"); } - - // Queries with cast are not supported - context.setUnsupportedOperationException(new UnsupportedOperationException( - "Queries with casts are not yet supported with json format in the new engine")); - return Boolean.FALSE; + return null; } @Override - public Boolean visitAlias(Alias node, JsonSupportVisitorContext context) { + public Void visitAlias(Alias node, JsonSupportVisitorContext context) { // Supported if outside of Project - if (!context.isVisitingProject()) { - return Boolean.TRUE; - } - - // Alias node is accepted if it does not have a user-defined alias - // and if the delegated expression is accepted. - if (StringUtils.isEmpty(node.getAlias())) { - return node.getDelegated().accept(this, context); - } else { - context.setUnsupportedOperationException(new UnsupportedOperationException( - "Queries with aliases are not yet supported with json format in the new engine")); - return Boolean.FALSE; + if (context.isVisitingProject()) { + // Alias node is accepted if it does not have a user-defined alias + // and if the delegated expression is accepted. + if (StringUtils.isEmpty(node.getAlias())) { + node.getDelegated().accept(this, context); + } else { + throw new UnsupportedOperationException( + "Queries with aliases are not yet supported with json format in the new engine"); + } } + return null; } @Override - public Boolean visitProject(Project node, JsonSupportVisitorContext context) { - boolean isSupported = visit(node, context); + public Void visitProject(Project node, JsonSupportVisitorContext context) { + visit(node, context); context.setVisitingProject(true); - isSupported = isSupported ? node.getProjectList().stream() - .allMatch(e -> e.accept(this, context)) : Boolean.FALSE; + node.getProjectList().forEach(e -> e.accept(this, context)); context.setVisitingProject(false); - - return isSupported; + return null; } } diff --git a/core/src/test/java/org/opensearch/sql/analysis/JsonSupportVisitorTest.java b/core/src/test/java/org/opensearch/sql/analysis/JsonSupportVisitorTest.java index 3f1db7952a..edd4fd99ca 100644 --- a/core/src/test/java/org/opensearch/sql/analysis/JsonSupportVisitorTest.java +++ b/core/src/test/java/org/opensearch/sql/analysis/JsonSupportVisitorTest.java @@ -5,8 +5,8 @@ package org.opensearch.sql.analysis; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.opensearch.sql.ast.dsl.AstDSL.qualifiedName; import com.google.common.collect.ImmutableList; @@ -23,13 +23,14 @@ public void visitLiteralInProject() { UnresolvedPlan project = AstDSL.project( AstDSL.relation("table", "table"), AstDSL.intLiteral(1)); - assertFalse(project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertThrows(UnsupportedOperationException.class, + () -> project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } @Test public void visitLiteralOutsideProject() { Literal intLiteral = AstDSL.intLiteral(1); - assertTrue(intLiteral.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertNull(intLiteral.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } @Test @@ -37,7 +38,8 @@ public void visitCastInProject() { UnresolvedPlan project = AstDSL.project( AstDSL.relation("table", "table"), AstDSL.cast(AstDSL.intLiteral(1), AstDSL.stringLiteral("INT"))); - assertFalse(project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertThrows(UnsupportedOperationException.class, + () -> project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } @Test @@ -45,7 +47,7 @@ public void visitCastOutsideProject() { UnresolvedExpression intCast = AstDSL.cast( AstDSL.intLiteral(1), AstDSL.stringLiteral("INT")); - assertTrue(intCast.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertNull(intCast.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } @Test @@ -53,21 +55,31 @@ public void visitAliasInProject() { UnresolvedPlan project = AstDSL.project( AstDSL.relation("table", "table"), AstDSL.alias("alias", AstDSL.intLiteral(1))); - assertFalse(project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertThrows(UnsupportedOperationException.class, + () -> project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } @Test - public void visitAliasInProjectWithDelegated() { + public void visitAliasInProjectWithUnsupportedDelegated() { UnresolvedPlan project = AstDSL.project( AstDSL.relation("table", "table"), AstDSL.alias("alias", AstDSL.intLiteral(1), "alias")); - assertFalse(project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertThrows(UnsupportedOperationException.class, + () -> project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + } + + @Test + public void visitAliasInProjectWithSupportedDelegated() { + UnresolvedPlan project = AstDSL.project( + AstDSL.relation("table", "table"), + AstDSL.alias("alias", AstDSL.field("field"))); + assertNull(project.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } @Test public void visitAliasOutsideProject() { UnresolvedExpression alias = AstDSL.alias("alias", AstDSL.intLiteral(1)); - assertTrue(alias.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertNull(alias.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } @Test @@ -75,13 +87,14 @@ public void visitFunctionInProject() { UnresolvedPlan function = AstDSL.project( AstDSL.relation("table", "table"), AstDSL.function("abs", AstDSL.intLiteral(-1))); - assertFalse(function.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertThrows(UnsupportedOperationException.class, + () -> function.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } @Test public void visitFunctionOutsideProject() { UnresolvedExpression function = AstDSL.function("abs", AstDSL.intLiteral(-1)); - assertTrue(function.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertNull(function.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } @Test @@ -92,7 +105,8 @@ public void visitAggregationWithGroupExprList() { Collections.emptyList(), ImmutableList.of(AstDSL.alias("alias", qualifiedName("integer_value"))), Collections.emptyList())); - assertFalse(projectAggr.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertThrows(UnsupportedOperationException.class, + () -> projectAggr.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } @Test @@ -106,6 +120,6 @@ public void visitAggregationWithAggExprList() { Collections.emptyList(), Collections.emptyList(), Collections.emptyList()); - assertTrue(aggregation.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); + assertNull(aggregation.accept(new JsonSupportVisitor(), new JsonSupportVisitorContext())); } } diff --git a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java index cb0dd9b59c..0f2e38629d 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java +++ b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java @@ -87,10 +87,7 @@ private AbstractPlan plan( // Go through the tree and throw exceptions when unsupported JsonSupportVisitorContext jsonSupportVisitorContext = new JsonSupportVisitorContext(); - if (!((Query) statement).getPlan() - .accept(new JsonSupportVisitor(), jsonSupportVisitorContext)) { - throw jsonSupportVisitorContext.getUnsupportedOperationException(); - } + ((Query) statement).getPlan().accept(new JsonSupportVisitor(), jsonSupportVisitorContext); } return queryExecutionFactory.create(statement, queryListener, explainListener); From 9233d7c1998f2dd802f20f4264c49eac76276b61 Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Mon, 13 Mar 2023 15:38:04 -0700 Subject: [PATCH 37/38] Fixed typo Signed-off-by: Guian Gumpac --- sql/src/main/java/org/opensearch/sql/sql/SQLService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java index b20f0d9786..0f2e38629d 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/SQLService.java +++ b/sql/src/main/java/org/opensearch/sql/sql/SQLService.java @@ -30,7 +30,7 @@ @RequiredArgsConstructor public class SQLService { - private final SQLSyntaxParser parser;sq + private final SQLSyntaxParser parser; private final QueryManager queryManager; From 33353549da3c36b165a5cc58cf079de2f51d334a Mon Sep 17 00:00:00 2001 From: Guian Gumpac Date: Tue, 14 Mar 2023 14:04:05 -0700 Subject: [PATCH 38/38] Added github link for tracking issue Signed-off-by: Guian Gumpac --- .../sql/analysis/JsonSupportVisitorContext.java | 4 ---- .../test/java/org/opensearch/sql/legacy/DateFormatIT.java | 8 ++++++++ 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitorContext.java b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitorContext.java index 9e5e892dbb..004e1a653b 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitorContext.java +++ b/core/src/main/java/org/opensearch/sql/analysis/JsonSupportVisitorContext.java @@ -15,8 +15,4 @@ public class JsonSupportVisitorContext { @Getter @Setter private boolean isVisitingProject = false; - - @Getter - @Setter - private UnsupportedOperationException unsupportedOperationException; } 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 1343558de9..3d15d88ce0 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 @@ -52,6 +52,7 @@ protected void init() throws Exception { */ // DATE_FORMAT with timezone argument is not yet supported in V2 + // Can be tracked with https://github.com/opensearch-project/sql/issues/1436 @Test(expected = SqlParseException.class) public void equalTo() throws SqlParseException { assertThat( @@ -62,6 +63,7 @@ public void equalTo() throws SqlParseException { } // DATE_FORMAT with timezone argument is not yet supported in V2 + // Can be tracked with https://github.com/opensearch-project/sql/issues/1436 @Test(expected = SqlParseException.class) public void lessThan() throws SqlParseException { assertThat( @@ -72,6 +74,7 @@ public void lessThan() throws SqlParseException { } // DATE_FORMAT with timezone argument is not yet supported in V2 + // Can be tracked with https://github.com/opensearch-project/sql/issues/1436 @Test(expected = SqlParseException.class) public void lessThanOrEqualTo() throws SqlParseException { assertThat( @@ -84,6 +87,7 @@ public void lessThanOrEqualTo() throws SqlParseException { } // DATE_FORMAT with timezone argument is not yet supported in V2 + // Can be tracked with https://github.com/opensearch-project/sql/issues/1436 @Test(expected = SqlParseException.class) public void greaterThan() throws SqlParseException { assertThat( @@ -94,6 +98,7 @@ public void greaterThan() throws SqlParseException { } // DATE_FORMAT with timezone argument is not yet supported in V2 + // Can be tracked with https://github.com/opensearch-project/sql/issues/1436 @Test(expected = SqlParseException.class) public void greaterThanOrEqualTo() throws SqlParseException { assertThat( @@ -106,6 +111,7 @@ public void greaterThanOrEqualTo() throws SqlParseException { } // DATE_FORMAT with timezone argument is not yet supported in V2 + // Can be tracked with https://github.com/opensearch-project/sql/issues/1436 @Test(expected = SqlParseException.class) public void and() throws SqlParseException { assertThat( @@ -130,6 +136,7 @@ public void andWithDefaultTimeZone() throws SqlParseException { } // DATE_FORMAT with timezone argument is not yet supported in V2 + // Can be tracked with https://github.com/opensearch-project/sql/issues/1436 @Test(expected = SqlParseException.class) public void or() throws SqlParseException { assertThat( @@ -143,6 +150,7 @@ public void or() throws SqlParseException { } // DATE_FORMAT with timezone argument is not yet supported in V2 + // Can be tracked with https://github.com/opensearch-project/sql/issues/1436 @Test(expected = ResponseException.class) public void sortByDateFormat() throws IOException { // Sort by expression in descending order, but sort inside in ascending order, so we increase our confidence