From d343a477ebea9e4b3cee217dd7d94e1e17019cb2 Mon Sep 17 00:00:00 2001 From: Dai Date: Fri, 9 Oct 2020 09:57:05 -0700 Subject: [PATCH 01/10] Enforce branch coverage and add UT --- sql/build.gradle | 6 +++++- .../sql/sql/parser/AstAggregationBuilder.java | 16 ++++++++++++---- .../sql/parser/AstAggregationBuilderTest.java | 9 +++++++++ 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/sql/build.gradle b/sql/build.gradle index 0443ba57c5..989e6a4a83 100644 --- a/sql/build.gradle +++ b/sql/build.gradle @@ -67,9 +67,13 @@ jacocoTestCoverageVerification { violationRules { rule { limit { + counter = 'LINE' + minimum = 1.0 + } + limit { + counter = 'BRANCH' minimum = 1.0 } - } } afterEvaluate { diff --git a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstAggregationBuilder.java b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstAggregationBuilder.java index 3a6aa50444..f9a18e04e1 100644 --- a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstAggregationBuilder.java +++ b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstAggregationBuilder.java @@ -27,6 +27,7 @@ import com.amazon.opendistroforelasticsearch.sql.ast.expression.UnresolvedExpression; import com.amazon.opendistroforelasticsearch.sql.ast.tree.Aggregation; import com.amazon.opendistroforelasticsearch.sql.ast.tree.UnresolvedPlan; +import com.amazon.opendistroforelasticsearch.sql.common.utils.StringUtils; import com.amazon.opendistroforelasticsearch.sql.exception.SemanticCheckException; import com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParser.GroupByClauseContext; import com.amazon.opendistroforelasticsearch.sql.sql.antlr.parser.OpenDistroSQLParserBaseVisitor; @@ -101,7 +102,7 @@ private UnresolvedPlan buildImplicitAggregation() { if (invalidSelectItem.isPresent()) { // Report semantic error to avoid fall back to old engine again - throw new SemanticCheckException(String.format( + throw new SemanticCheckException(StringUtils.format( "Explicit GROUP BY clause is required because expression [%s] " + "contains non-aggregated column", invalidSelectItem.get())); } @@ -148,14 +149,21 @@ private boolean isNonAggregatedExpression(UnresolvedExpression expr) { } private boolean isIntegerLiteral(UnresolvedExpression expr) { - return (expr instanceof Literal) - && (((Literal) expr).getType() == DataType.INTEGER); + if (!(expr instanceof Literal)) { + return false; + } + + if (((Literal) expr).getType() != DataType.INTEGER) { + throw new SemanticCheckException(StringUtils.format( + "Non-integer constant [%s] found in GROUP BY clause", expr)); + } + return true; } private UnresolvedExpression getSelectItemByOrdinal(UnresolvedExpression expr) { int ordinal = (Integer) ((Literal) expr).getValue(); if (ordinal <= 0 || ordinal > querySpec.getSelectItems().size()) { - throw new SemanticCheckException(String.format( + throw new SemanticCheckException(StringUtils.format( "Group by ordinal [%d] is out of bound of select item list", ordinal)); } final UnresolvedExpression groupExpr = querySpec.getSelectItems().get(ordinal - 1); diff --git a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstAggregationBuilderTest.java b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstAggregationBuilderTest.java index 3b601617fe..c72836e4b9 100644 --- a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstAggregationBuilderTest.java +++ b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstAggregationBuilderTest.java @@ -134,6 +134,15 @@ void should_replace_group_by_ordinal_by_expression_in_select_clause() { alias("ABS(age)", function("ABS", qualifiedName("age"))))); } + @Test + void should_report_error_for_non_integer_ordinal_in_group_by() { + SemanticCheckException error = assertThrows(SemanticCheckException.class, () -> + buildAggregation("SELECT state AS s FROM test GROUP BY 1.5")); + assertEquals( + "Non-integer constant [1.5] found in GROUP BY clause", + error.getMessage()); + } + @Disabled("This validation is supposed to be in analyzing phase") @Test void should_report_error_for_mismatch_between_select_and_group_by_items() { From fe047e8090ce6786e979b1d6ce5a211371962152 Mon Sep 17 00:00:00 2001 From: Dai Date: Fri, 9 Oct 2020 10:16:04 -0700 Subject: [PATCH 02/10] Add UT for sql query request --- .../sql/sql/domain/SQLQueryRequest.java | 22 +++++++++++++------ .../sql/sql/domain/SQLQueryRequestTest.java | 9 ++++++++ 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/domain/SQLQueryRequest.java b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/domain/SQLQueryRequest.java index 9649cb9010..15fe8bf380 100644 --- a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/domain/SQLQueryRequest.java +++ b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/domain/SQLQueryRequest.java @@ -17,6 +17,8 @@ package com.amazon.opendistroforelasticsearch.sql.sql.domain; import com.google.common.base.Strings; +import com.google.common.collect.ImmutableSet; +import java.util.Set; import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.RequiredArgsConstructor; @@ -32,6 +34,9 @@ @RequiredArgsConstructor public class SQLQueryRequest { + private static final Set QUERY_FIELD = ImmutableSet.of("query"); + private static final Set QUERY_AND_FETCH_SIZE = ImmutableSet.of("query", "fetch_size"); + /** * JSON payload in REST request. */ @@ -54,15 +59,15 @@ public class SQLQueryRequest { /** * Pre-check if the request can be supported by meeting the following criteria: - * 1.Only "query" field in payload. In other word, it's not a cursor request - * (with either non-zero "fetch_size" or "cursor" field) or request with extra field - * such as "filter". + * 1.Only "query" field or "query" and "fetch_size=0" in payload. In other word, + * it's not a cursor request with either non-zero "fetch_size" or "cursor" field, + * or request with extra field such as "filter". * 2.Response format expected is default JDBC format. * * @return true if supported. */ public boolean isSupported() { - return isOnlyQueryFieldInPayload() + return (isOnlyQueryFieldInPayload() || isOnlyQueryAndFetchSizeZeroInPayload()) && isDefaultFormat(); } @@ -75,9 +80,12 @@ public boolean isExplainRequest() { } private boolean isOnlyQueryFieldInPayload() { - return (jsonContent.keySet().size() == 1 && jsonContent.has("query")) - || (jsonContent.keySet().size() == 2 && jsonContent.has("query") - && jsonContent.has("fetch_size") && jsonContent.getInt("fetch_size") == 0); + return QUERY_FIELD.equals(jsonContent.keySet()); + } + + private boolean isOnlyQueryAndFetchSizeZeroInPayload() { + return QUERY_AND_FETCH_SIZE.equals(jsonContent.keySet()) + && (jsonContent.getInt("fetch_size") == 0); } private boolean isDefaultFormat() { diff --git a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/domain/SQLQueryRequestTest.java b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/domain/SQLQueryRequestTest.java index c1f4713a13..796801109e 100644 --- a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/domain/SQLQueryRequestTest.java +++ b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/domain/SQLQueryRequestTest.java @@ -38,6 +38,15 @@ public void shouldSupportQueryWithJDBCFormat() { assertTrue(request.isSupported()); } + @Test + public void shouldSupportQueryWithQueryFieldOnly() { + SQLQueryRequest request = + SQLQueryRequestBuilder.request("SELECT 1") + .jsonContent("{\"query\": \"SELECT 1\"}") + .build(); + assertTrue(request.isSupported()); + } + @Test public void shouldSupportQueryWithZeroFetchSize() { SQLQueryRequest request = From 341c67b4e70911e011b9e7626596fcc86ab2fef6 Mon Sep 17 00:00:00 2001 From: Dai Date: Fri, 9 Oct 2020 10:29:59 -0700 Subject: [PATCH 03/10] Add UT for ast aggregation builder --- .../sql/sql/parser/AstAggregationBuilder.java | 10 +++++----- .../sql/sql/parser/AstAggregationBuilderTest.java | 10 ---------- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstAggregationBuilder.java b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstAggregationBuilder.java index f9a18e04e1..c88641da38 100644 --- a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstAggregationBuilder.java +++ b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstAggregationBuilder.java @@ -139,12 +139,12 @@ private boolean isAllSelectItemNonAggregated() { } private boolean isNonAggregatedExpression(UnresolvedExpression expr) { - List children = expr.getChild(); - if (children.isEmpty()) { - return true; + if (expr instanceof AggregateFunction) { + return false; } - return !(expr instanceof AggregateFunction) - && children.stream() + + List children = expr.getChild(); + return children.stream() .allMatch(child -> isNonAggregatedExpression((UnresolvedExpression) child)); } diff --git a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstAggregationBuilderTest.java b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstAggregationBuilderTest.java index c72836e4b9..21c5372ea6 100644 --- a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstAggregationBuilderTest.java +++ b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstAggregationBuilderTest.java @@ -189,16 +189,6 @@ void should_report_error_for_group_by_ordinal_out_of_bound_of_select_list() { assertEquals("Group by ordinal [3] is out of bound of select item list", error2.getMessage()); } - @Disabled - @Test - void should_report_error_for_non_integer_ordinal_in_group_by_clause() { - SemanticCheckException error = assertThrows(SemanticCheckException.class, () -> - buildAggregation("SELECT age, AVG(balance) FROM tests GROUP BY 0.0")); - assertEquals( - "Expression [age] that contains non-aggregated column is not present in group by clause", - error.getMessage()); - } - private Matcher hasGroupByItems(UnresolvedExpression... exprs) { return featureValueOf("groupByItems", Aggregation::getGroupExprList, exprs); } From 5b16eeba1862517d217d5c55c43ace95bd19a067 Mon Sep 17 00:00:00 2001 From: Dai Date: Fri, 9 Oct 2020 10:45:06 -0700 Subject: [PATCH 04/10] Enforce branch coverage for elasticsearch module --- elasticsearch/build.gradle | 6 +++++- .../response/ElasticsearchResponseTest.java | 12 ++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/elasticsearch/build.gradle b/elasticsearch/build.gradle index e18a3a9a7e..261bb08409 100644 --- a/elasticsearch/build.gradle +++ b/elasticsearch/build.gradle @@ -54,9 +54,13 @@ jacocoTestCoverageVerification { 'com.amazon.opendistroforelasticsearch.sql.elasticsearch.security.SecurityAccess' ] limit { + counter = 'LINE' + minimum = 1.0 + } + limit { + counter = 'BRANCH' minimum = 1.0 } - } } afterEvaluate { diff --git a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/response/ElasticsearchResponseTest.java b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/response/ElasticsearchResponseTest.java index 2746a64885..b06da33519 100644 --- a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/response/ElasticsearchResponseTest.java +++ b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/response/ElasticsearchResponseTest.java @@ -16,6 +16,7 @@ package com.amazon.opendistroforelasticsearch.sql.elasticsearch.response; +import static java.util.Collections.emptyList; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -75,17 +76,20 @@ void isEmpty() { new TotalHits(2L, TotalHits.Relation.EQUAL_TO), 1.0F)); - ElasticsearchResponse response1 = new ElasticsearchResponse(esResponse, factory); - assertFalse(response1.isEmpty()); + assertFalse(new ElasticsearchResponse(esResponse, factory).isEmpty()); when(esResponse.getHits()).thenReturn(SearchHits.empty()); - ElasticsearchResponse response2 = new ElasticsearchResponse(esResponse, factory); - assertTrue(response2.isEmpty()); + when(esResponse.getAggregations()).thenReturn(null); + assertTrue(new ElasticsearchResponse(esResponse, factory).isEmpty()); when(esResponse.getHits()) .thenReturn(new SearchHits(null, new TotalHits(0, TotalHits.Relation.EQUAL_TO), 0)); ElasticsearchResponse response3 = new ElasticsearchResponse(esResponse, factory); assertTrue(response3.isEmpty()); + + when(esResponse.getHits()).thenReturn(SearchHits.empty()); + when(esResponse.getAggregations()).thenReturn(new Aggregations(emptyList())); + assertFalse(new ElasticsearchResponse(esResponse, factory).isEmpty()); } @Test From 950826dfe350d41fa809fe6dafa67e2b8729c812 Mon Sep 17 00:00:00 2001 From: Dai Date: Fri, 9 Oct 2020 11:01:21 -0700 Subject: [PATCH 05/10] Add UT for lucene query --- .../storage/script/filter/lucene/LuceneQueryTest.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/lucene/LuceneQueryTest.java b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/lucene/LuceneQueryTest.java index 33c47cd461..f8d194f76a 100644 --- a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/lucene/LuceneQueryTest.java +++ b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/lucene/LuceneQueryTest.java @@ -16,8 +16,12 @@ package com.amazon.opendistroforelasticsearch.sql.elasticsearch.storage.script.filter.lucene; +import static com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType.INTEGER; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; +import com.amazon.opendistroforelasticsearch.sql.expression.DSL; +import com.amazon.opendistroforelasticsearch.sql.expression.config.ExpressionConfig; import org.junit.jupiter.api.DisplayNameGeneration; import org.junit.jupiter.api.DisplayNameGenerator; import org.junit.jupiter.api.Test; @@ -25,6 +29,12 @@ @DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) class LuceneQueryTest { + @Test + void should_not_support_single_argument_by_default() { + DSL dsl = new ExpressionConfig().dsl(new ExpressionConfig().functionRepository()); + assertFalse(new LuceneQuery(){}.canSupport(dsl.abs(DSL.ref("age", INTEGER)))); + } + @Test void should_throw_exception_if_not_implemented() { assertThrows(UnsupportedOperationException.class, () -> From acf1201c315109f0ba6b92017cb179226ab0a359 Mon Sep 17 00:00:00 2001 From: Dai Date: Fri, 9 Oct 2020 11:56:49 -0700 Subject: [PATCH 06/10] Remove unused code --- .../elasticsearch/storage/script/core/ExpressionScript.java | 2 +- .../storage/script/filter/ExpressionFilterScript.java | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/core/ExpressionScript.java b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/core/ExpressionScript.java index 813f93e031..5a00e0455b 100644 --- a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/core/ExpressionScript.java +++ b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/core/ExpressionScript.java @@ -137,7 +137,7 @@ private Object getDocValue(ReferenceExpression field, String fieldName = getDocValueName(field); ScriptDocValues docValue = docProvider.get().get(fieldName); if (docValue == null || docValue.isEmpty()) { - return null; + return null; // No way to differentiate null and missing from doc value } Object value = docValue.get(0); diff --git a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/ExpressionFilterScript.java b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/ExpressionFilterScript.java index e64cf161bd..003969f7c6 100644 --- a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/ExpressionFilterScript.java +++ b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/ExpressionFilterScript.java @@ -53,11 +53,10 @@ public boolean execute() { return (Boolean) expressionScript.execute(this::getDoc, this::evaluateExpression); } - private ExprValue evaluateExpression(Expression expression, Environment valueEnv) { ExprValue result = expression.valueOf(valueEnv); - if (result.isNull() || result.isMissing()) { + if (result.isNull()) { return ExprBooleanValue.of(false); } From 873130f35a809f4761a3d8ec9b78281be1acf7f5 Mon Sep 17 00:00:00 2001 From: Dai Date: Fri, 9 Oct 2020 13:45:48 -0700 Subject: [PATCH 07/10] Add UT for filter query builder --- .../script/filter/FilterQueryBuilderTest.java | 65 ++++++++++++------- 1 file changed, 43 insertions(+), 22 deletions(-) diff --git a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/FilterQueryBuilderTest.java b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/FilterQueryBuilderTest.java index 452b694e2e..4de1956cdc 100644 --- a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/FilterQueryBuilderTest.java +++ b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/FilterQueryBuilderTest.java @@ -21,12 +21,11 @@ import static com.amazon.opendistroforelasticsearch.sql.elasticsearch.data.type.ElasticsearchDataType.ES_TEXT_KEYWORD; import static com.amazon.opendistroforelasticsearch.sql.expression.DSL.literal; import static com.amazon.opendistroforelasticsearch.sql.expression.DSL.ref; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; -import static org.mockito.Mockito.when; +import com.amazon.opendistroforelasticsearch.sql.common.utils.StringUtils; import com.amazon.opendistroforelasticsearch.sql.elasticsearch.storage.serialization.ExpressionSerializer; import com.amazon.opendistroforelasticsearch.sql.expression.DSL; import com.amazon.opendistroforelasticsearch.sql.expression.Expression; @@ -34,6 +33,7 @@ import com.amazon.opendistroforelasticsearch.sql.expression.config.ExpressionConfig; import com.google.common.collect.ImmutableMap; import java.util.Map; +import org.json.JSONObject; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayNameGeneration; import org.junit.jupiter.api.DisplayNameGenerator; @@ -60,7 +60,7 @@ void set_up() { @Test void should_build_term_query_for_equality_expression() { - assertEquals( + assertJsonEquals( "{\n" + " \"term\" : {\n" + " \"name\" : {\n" @@ -84,7 +84,7 @@ void should_build_range_query_for_comparison_expression() { dsl.gte(params), new Object[]{30, null, true, true}); ranges.forEach((expr, range) -> - assertEquals( + assertJsonEquals( "{\n" + " \"range\" : {\n" + " \"age\" : {\n" @@ -101,7 +101,7 @@ void should_build_range_query_for_comparison_expression() { @Test void should_build_wildcard_query_for_like_expression() { - assertEquals( + assertJsonEquals( "{\n" + " \"wildcard\" : {\n" + " \"name\" : {\n" @@ -116,13 +116,26 @@ void should_build_wildcard_query_for_like_expression() { } @Test - void should_build_script_query_for_function_expression() { - doAnswer(invocation -> { - Expression expr = invocation.getArgument(0); - return expr.toString(); - }).when(serializer).serialize(any()); + void should_build_script_query_for_unsupported_lucene_query() { + mockToStringSerializer(); + assertJsonEquals( + "{\n" + + " \"script\" : {\n" + + " \"script\" : {\n" + + " \"source\" : \"is not null(age)\",\n" + + " \"lang\" : \"opendistro_expression\"\n" + + " },\n" + + " \"boost\" : 1.0\n" + + " }\n" + + "}", + buildQuery( + dsl.isnotnull(ref("age", INTEGER)))); + } - assertEquals( + @Test + void should_build_script_query_for_function_expression() { + mockToStringSerializer(); + assertJsonEquals( "{\n" + " \"script\" : {\n" + " \"script\" : {\n" @@ -139,12 +152,8 @@ void should_build_script_query_for_function_expression() { @Test void should_build_script_query_for_comparison_between_fields() { - doAnswer(invocation -> { - Expression expr = invocation.getArgument(0); - return expr.toString(); - }).when(serializer).serialize(any()); - - assertEquals( + mockToStringSerializer(); + assertJsonEquals( "{\n" + " \"script\" : {\n" + " \"script\" : {\n" @@ -170,7 +179,7 @@ void should_build_bool_query_for_and_or_expression() { }; for (int i = 0; i < names.length; i++) { - assertEquals( + assertJsonEquals( "{\n" + " \"bool\" : {\n" + " \"" + names[i] + "\" : [\n" @@ -201,7 +210,7 @@ void should_build_bool_query_for_and_or_expression() { @Test void should_build_bool_query_for_not_expression() { - assertEquals( + assertJsonEquals( "{\n" + " \"bool\" : {\n" + " \"must_not\" : [\n" @@ -226,7 +235,7 @@ void should_build_bool_query_for_not_expression() { @Test void should_use_keyword_for_multi_field_in_equality_expression() { - assertEquals( + assertJsonEquals( "{\n" + " \"term\" : {\n" + " \"name.keyword\" : {\n" @@ -242,7 +251,7 @@ void should_use_keyword_for_multi_field_in_equality_expression() { @Test void should_use_keyword_for_multi_field_in_like_expression() { - assertEquals( + assertJsonEquals( "{\n" + " \"wildcard\" : {\n" + " \"name.keyword\" : {\n" @@ -256,8 +265,20 @@ void should_use_keyword_for_multi_field_in_like_expression() { ref("name", ES_TEXT_KEYWORD), literal("John%")))); } + private static void assertJsonEquals(String expected, String actual) { + assertTrue(new JSONObject(expected).similar(new JSONObject(actual)), + StringUtils.format("Expected: %s, actual: %s", expected, actual)); + } + private String buildQuery(Expression expr) { return filterQueryBuilder.build(expr).toString(); } + private void mockToStringSerializer() { + doAnswer(invocation -> { + Expression expr = invocation.getArgument(0); + return expr.toString(); + }).when(serializer).serialize(any()); + } + } \ No newline at end of file From 0415993352c8c4454b878ee50ffa6f395492cb96 Mon Sep 17 00:00:00 2001 From: Dai Date: Fri, 9 Oct 2020 13:51:00 -0700 Subject: [PATCH 08/10] Add UT for filter script --- .../filter/ExpressionFilterScriptTest.java | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/ExpressionFilterScriptTest.java b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/ExpressionFilterScriptTest.java index cd9499386f..bf97dfd5ef 100644 --- a/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/ExpressionFilterScriptTest.java +++ b/elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/script/filter/ExpressionFilterScriptTest.java @@ -23,7 +23,9 @@ import static com.amazon.opendistroforelasticsearch.sql.elasticsearch.data.type.ElasticsearchDataType.ES_TEXT_KEYWORD; import static com.amazon.opendistroforelasticsearch.sql.expression.DSL.literal; import static com.amazon.opendistroforelasticsearch.sql.expression.DSL.ref; +import static java.util.Collections.emptyList; import static java.util.Collections.emptyMap; +import static java.util.Collections.singletonList; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; @@ -38,6 +40,7 @@ import com.amazon.opendistroforelasticsearch.sql.expression.config.ExpressionConfig; import com.google.common.collect.ImmutableMap; import java.time.ZonedDateTime; +import java.util.List; import java.util.Map; import lombok.RequiredArgsConstructor; import org.apache.lucene.index.LeafReaderContext; @@ -132,6 +135,14 @@ void can_execute_expression_with_missing_field() { .shouldNotMatch(); } + @Test + void can_execute_expression_with_empty_doc_value() { + assertThat() + .docValues("name", emptyList()) + .filterBy(ref("name", STRING)) + .shouldNotMatch(); + } + @Test void cannot_execute_non_predicate_expression() { assertThrow(IllegalStateException.class, @@ -210,9 +221,13 @@ private LeafDocLookup mockLeafDocLookup(Map> docValue } } - @RequiredArgsConstructor private static class FakeScriptDocValues extends ScriptDocValues { - private final T value; + private final List values; + + @SuppressWarnings("unchecked") + public FakeScriptDocValues(T value) { + this.values = (value instanceof List) ? (List) value : singletonList(value); + } @Override public void setNextDocId(int docId) { @@ -221,12 +236,12 @@ public void setNextDocId(int docId) { @Override public T get(int index) { - return value; + return values.get(index); } @Override public int size() { - return 1; + return values.size(); } } From 3aab130999684b94d4c0a78f68ac4c2c440e1620 Mon Sep 17 00:00:00 2001 From: Dai Date: Fri, 9 Oct 2020 13:57:38 -0700 Subject: [PATCH 09/10] Remove unused code --- .../sql/elasticsearch/storage/ElasticsearchIndexScan.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndexScan.java b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndexScan.java index bac4bc82ed..c60ac55ac9 100644 --- a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndexScan.java +++ b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/storage/ElasticsearchIndexScan.java @@ -142,8 +142,7 @@ public void close() { } private boolean isBoolFilterQuery(QueryBuilder current) { - return (current instanceof BoolQueryBuilder) - && !((BoolQueryBuilder) current).filter().isEmpty(); + return (current instanceof BoolQueryBuilder); } } From 84806df3adea100df5b48332d01bfad97c508040 Mon Sep 17 00:00:00 2001 From: Dai Date: Mon, 12 Oct 2020 10:00:24 -0700 Subject: [PATCH 10/10] Add UT for window function --- .../sql/sql/parser/AstExpressionBuilderTest.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilderTest.java b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilderTest.java index 9451756bc6..e38a3afc3a 100644 --- a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilderTest.java +++ b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstExpressionBuilderTest.java @@ -254,7 +254,10 @@ public void canBuildWindowFunction() { ImmutableList.of(qualifiedName("state")), ImmutableList.of(ImmutablePair.of("ASC", qualifiedName("age")))), buildExprAst("RANK() OVER (PARTITION BY state ORDER BY age)")); + } + @Test + public void canBuildWindowFunctionWithoutPartitionBy() { assertEquals( window( function("DENSE_RANK"), @@ -263,6 +266,16 @@ public void canBuildWindowFunction() { buildExprAst("DENSE_RANK() OVER (ORDER BY age DESC)")); } + @Test + public void canBuildWindowFunctionWithoutOrderBy() { + assertEquals( + window( + function("RANK"), + ImmutableList.of(qualifiedName("state")), + ImmutableList.of()), + buildExprAst("RANK() OVER (PARTITION BY state)")); + } + @Test public void canBuildKeywordsAsIdentifiers() { assertEquals(