address PR comments

opendistro-for-elasticsearch · Dec 12, 2020 · 41e9d73 · 41e9d73
1 parent 0260ffa
commit 41e9d73
Show file tree

Hide file tree

Showing 4 changed files with 39 additions and 2 deletions.
diff --git a/...t/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/MathematicalFunctionIT.java b/...t/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
@@ -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/...main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstAggregationBuilder.java b/...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<? extends Node> 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/.../java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstAggregationBuilderTest.java b/.../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