From 7f9af23a24b69e7c59a37803eb06d5336daf448c Mon Sep 17 00:00:00 2001 From: Rupal Mahajan <> Date: Thu, 10 Dec 2020 14:24:13 -0800 Subject: [PATCH 1/4] add functions with literal --- .../sql/sql/DateTimeFunctionIT.java | 9 +++++++++ .../sql/sql/parser/AstAggregationBuilder.java | 11 +++++++++++ .../sql/sql/parser/AstAggregationBuilderTest.java | 7 +++++++ 3 files changed, 27 insertions(+) diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/DateTimeFunctionIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/DateTimeFunctionIT.java index 677621d2bc..8edd2b6c9a 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/DateTimeFunctionIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/DateTimeFunctionIT.java @@ -60,6 +60,15 @@ public void testDateInGroupBy() throws IOException{ rows("2018-08-11")); } + @Test + public void testDateWithHavingClauseOnly() throws IOException { + JSONObject result = + executeQuery(String.format("SELECT (TO_DAYS(DATE('2050-01-01')) - 693961) FROM %s HAVING (COUNT(1) > 0)",TEST_INDEX_BANK) ); + verifySchema(result, + schema("(TO_DAYS(DATE('2050-01-01')) - 693961)", null, "long")); + verifyDataRows(result, rows(54787)); + } + @Test public void testAddDate() throws IOException { JSONObject result = 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 40f4d734f4..77213002bf 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 @@ -21,6 +21,7 @@ import com.amazon.opendistroforelasticsearch.sql.ast.Node; import com.amazon.opendistroforelasticsearch.sql.ast.expression.AggregateFunction; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Alias; +import com.amazon.opendistroforelasticsearch.sql.ast.expression.Function; import com.amazon.opendistroforelasticsearch.sql.ast.expression.Literal; import com.amazon.opendistroforelasticsearch.sql.ast.expression.UnresolvedExpression; import com.amazon.opendistroforelasticsearch.sql.ast.tree.Aggregation; @@ -126,6 +127,7 @@ private Optional findNonAggregatedItemInSelect() { return querySpec.getSelectItems().stream() .filter(this::isNonLiteral) .filter(this::isNonAggregatedExpression) + .filter(this::isNonLiteralFunction) .findFirst(); } @@ -137,6 +139,15 @@ private boolean isNonLiteral(UnresolvedExpression expr) { return !(expr instanceof Literal); } + private boolean isNonLiteralFunction(UnresolvedExpression expr) { + if (expr instanceof Function) { + List children = expr.getChild(); + return children.stream() + .allMatch(child -> isNonLiteral((UnresolvedExpression) child)); + } + return true; + } + private boolean isNonAggregatedExpression(UnresolvedExpression expr) { if (expr instanceof AggregateFunction) { return false; 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 5e74917810..1bb8f30278 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 @@ -112,6 +112,13 @@ void can_build_implicit_group_by_for_aggregator_in_having_clause() { hasGroupByItems(), hasAggregators( alias("AVG(age)", aggregate("AVG", qualifiedName("age")))))); + + assertThat( + buildAggregation("SELECT ABS(1.5) FROM test HAVING AVG(age) > 30"), + allOf( + hasGroupByItems(), + hasAggregators( + alias("AVG(age)", aggregate("AVG", qualifiedName("age")))))); } @Test From 41e9d7391abf1f2cdf8d38a4f6fe1a8b880a02b2 Mon Sep 17 00:00:00 2001 From: Rupal Mahajan <> Date: Fri, 11 Dec 2020 17:52:19 -0800 Subject: [PATCH 2/4] address PR comments --- .../sql/sql/MathematicalFunctionIT.java | 11 +++++++++++ .../test/resources/correctness/bugfixes/899.txt | 4 ++++ .../sql/sql/parser/AstAggregationBuilder.java | 12 ++++++++++-- .../sql/sql/parser/AstAggregationBuilderTest.java | 14 ++++++++++++++ 4 files changed, 39 insertions(+), 2 deletions(-) create mode 100644 integ-test/src/test/resources/correctness/bugfixes/899.txt diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/MathematicalFunctionIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/MathematicalFunctionIT.java index 7c55cbcc8e..e4adb1feca 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/MathematicalFunctionIT.java +++ b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/MathematicalFunctionIT.java @@ -15,6 +15,7 @@ package com.amazon.opendistroforelasticsearch.sql.sql; +import static com.amazon.opendistroforelasticsearch.sql.legacy.TestsConstants.TEST_INDEX_BANK; import static com.amazon.opendistroforelasticsearch.sql.legacy.plugin.RestSqlAction.QUERY_API_ENDPOINT; import static com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils.rows; import static com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils.schema; @@ -38,6 +39,16 @@ public class MathematicalFunctionIT extends SQLIntegTestCase { public void init() throws Exception { super.init(); TestUtils.enableNewQueryEngine(client()); + loadIndex(Index.BANK); + } + + @Test + public void testPI() throws IOException { + JSONObject result = + executeQuery(String.format("SELECT PI() FROM %s HAVING (COUNT(1) > 0)",TEST_INDEX_BANK) ); + verifySchema(result, + schema("PI()", null, "double")); + verifyDataRows(result, rows(3.141592653589793)); } @Test diff --git a/integ-test/src/test/resources/correctness/bugfixes/899.txt b/integ-test/src/test/resources/correctness/bugfixes/899.txt new file mode 100644 index 0000000000..b272037939 --- /dev/null +++ b/integ-test/src/test/resources/correctness/bugfixes/899.txt @@ -0,0 +1,4 @@ +SELECT 'hello' FROM kibana_sample_data_flights HAVING (COUNT(1) > 0) +SELECT UPPER('hello') AS `literal` FROM kibana_sample_data_flights HAVING (COUNT(1) > 0) +SELECT UPPER(UPPER('hello')) AS `literal` FROM kibana_sample_data_flights HAVING (COUNT(1) > 0) +SELECT SUBSTRING(CONCAT(UPPER('hello'), 'world'), 1, 6) AS `literal` FROM kibana_sample_data_flights HAVING (COUNT(1) > 0) 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 77213002bf..235774f558 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 @@ -140,10 +140,18 @@ private boolean isNonLiteral(UnresolvedExpression expr) { } private boolean isNonLiteralFunction(UnresolvedExpression expr) { + // The base case for recursion + if (expr instanceof Literal) { + return false; + } if (expr instanceof Function) { List children = expr.getChild(); - return children.stream() - .allMatch(child -> isNonLiteral((UnresolvedExpression) child)); + // The base case for functions without input. e.g. PI(), NOW(), etc. + if (children.isEmpty()) { + return false; + } + return children.stream().anyMatch(child -> + isNonLiteralFunction((UnresolvedExpression) child)); } return true; } 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 1bb8f30278..87183e4051 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 @@ -113,12 +113,26 @@ void can_build_implicit_group_by_for_aggregator_in_having_clause() { hasAggregators( alias("AVG(age)", aggregate("AVG", qualifiedName("age")))))); + assertThat( + buildAggregation("SELECT PI() FROM test HAVING AVG(age) > 30"), + allOf( + hasGroupByItems(), + hasAggregators( + alias("AVG(age)", aggregate("AVG", qualifiedName("age")))))); + assertThat( buildAggregation("SELECT ABS(1.5) FROM test HAVING AVG(age) > 30"), allOf( hasGroupByItems(), hasAggregators( alias("AVG(age)", aggregate("AVG", qualifiedName("age")))))); + + assertThat( + buildAggregation("SELECT ABS(ABS(1.5)) FROM test HAVING AVG(age) > 30"), + allOf( + hasGroupByItems(), + hasAggregators( + alias("AVG(age)", aggregate("AVG", qualifiedName("age")))))); } @Test From b3b8282cbd9c5c90a409a68c5156604f1eac9e69 Mon Sep 17 00:00:00 2001 From: Rupal Mahajan <> Date: Fri, 11 Dec 2020 17:58:37 -0800 Subject: [PATCH 3/4] remove unnecessary check. anyMatch returns false for empty list. --- .../sql/sql/parser/AstAggregationBuilder.java | 4 ---- 1 file changed, 4 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 235774f558..4c56d3c64d 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 @@ -146,10 +146,6 @@ private boolean isNonLiteralFunction(UnresolvedExpression expr) { } if (expr instanceof Function) { List children = expr.getChild(); - // The base case for functions without input. e.g. PI(), NOW(), etc. - if (children.isEmpty()) { - return false; - } return children.stream().anyMatch(child -> isNonLiteralFunction((UnresolvedExpression) child)); } From 5dace04d726b646cd9530b66c928be1c8e501754 Mon Sep 17 00:00:00 2001 From: Rupal Mahajan <> Date: Mon, 14 Dec 2020 10:28:53 -0800 Subject: [PATCH 4/4] remove isNonLiteral() --- .../sql/sql/parser/AstAggregationBuilder.java | 5 ----- 1 file changed, 5 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 4c56d3c64d..39ac8575fa 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 @@ -125,7 +125,6 @@ private List replaceGroupByItemIfAliasOrOrdinal() { */ private Optional findNonAggregatedItemInSelect() { return querySpec.getSelectItems().stream() - .filter(this::isNonLiteral) .filter(this::isNonAggregatedExpression) .filter(this::isNonLiteralFunction) .findFirst(); @@ -135,10 +134,6 @@ private boolean isAggregatorNotFoundAnywhere() { return querySpec.getAggregators().isEmpty(); } - private boolean isNonLiteral(UnresolvedExpression expr) { - return !(expr instanceof Literal); - } - private boolean isNonLiteralFunction(UnresolvedExpression expr) { // The base case for recursion if (expr instanceof Literal) {