[pinot] branch master updated: Broker Side validation for the query with aggregation and col but without group by (#7972)

[si...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

siddteotia pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git


The following commit(s) were added to refs/heads/master by this push:
     new a22cc00  Broker Side validation for the query with aggregation and col but without group by (#7972)
a22cc00 is described below

commit a22cc0022933df6de85b4b966556e95ab5b81c5f
Author: Prachi Prakash <pr...@gmail.com>
AuthorDate: Tue Feb 1 12:28:11 2022 +0530

    Broker Side validation for the query with aggregation and col but without group by (#7972)
    
    * validate that col expr and aggregate expr is not present without group by
    
    * fixing the AggregateMetricsRules test.
    
    * changes as per the review comments
---
 .../apache/pinot/sql/parsers/CalciteSqlParser.java | 31 +++++++++++++---------
 .../pinot/sql/parsers/CalciteSqlCompilerTest.java  | 18 +++++++++++--
 pinot-common/src/test/resources/sql_queries.list   |  4 +--
 .../rules/impl/AggregateMetricsRuleTest.java       |  2 +-
 4 files changed, 38 insertions(+), 17 deletions(-)

diff --git a/pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java b/pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
index 6b4d040..aa93d66 100644
--- a/pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
+++ b/pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
@@ -146,23 +146,30 @@ public class CalciteSqlParser {
 
   private static void validateGroupByClause(PinotQuery pinotQuery)
       throws SqlCompilationException {
-    if (pinotQuery.getGroupByList() == null) {
-      return;
-    }
-    // Sanity check group by query: All non-aggregate expression in selection list should be also included in group
-    // by list.
-    Set<Expression> groupByExprs = new HashSet<>(pinotQuery.getGroupByList());
+    boolean hasGroupByClause = pinotQuery.getGroupByList() != null;
+    Set<Expression> groupByExprs = hasGroupByClause ? new HashSet<>(pinotQuery.getGroupByList()) : null;
+    int aggregateExprCount = 0;
     for (Expression selectExpression : pinotQuery.getSelectList()) {
-      if (!isAggregateExpression(selectExpression) && expressionOutsideGroupByList(selectExpression, groupByExprs)) {
-        throw new SqlCompilationException(
+      if (isAggregateExpression(selectExpression)) {
+        aggregateExprCount++;
+      } else if (hasGroupByClause && expressionOutsideGroupByList(selectExpression, groupByExprs)) {
+          throw new SqlCompilationException(
             "'" + RequestUtils.prettyPrint(selectExpression) + "' should appear in GROUP BY clause.");
       }
     }
+
+    // block mixture of aggregate and non-aggregate expression when group by is absent
+    int nonAggregateExprCount = pinotQuery.getSelectListSize() - aggregateExprCount;
+    if (!hasGroupByClause && aggregateExprCount > 0 && nonAggregateExprCount > 0) {
+      throw new SqlCompilationException("Columns and Aggregate functions can't co-exist without GROUP BY clause");
+    }
     // Sanity check on group by clause shouldn't contain aggregate expression.
-    for (Expression groupByExpression : pinotQuery.getGroupByList()) {
-      if (isAggregateExpression(groupByExpression)) {
-        throw new SqlCompilationException("Aggregate expression '" + RequestUtils.prettyPrint(groupByExpression)
-            + "' is not allowed in GROUP BY clause.");
+    if (hasGroupByClause) {
+      for (Expression groupByExpression : pinotQuery.getGroupByList()) {
+        if (isAggregateExpression(groupByExpression)) {
+          throw new SqlCompilationException("Aggregate expression '" + RequestUtils.prettyPrint(groupByExpression)
+              + "' is not allowed in GROUP BY clause.");
+        }
       }
     }
   }
diff --git a/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java b/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java
index defad35..6c49c46 100644
--- a/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java
+++ b/pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java
@@ -80,7 +80,8 @@ public class CalciteSqlCompilerTest {
 
     pinotQuery = CalciteSqlParser.compileToPinotQuery(
         "SELECT Quantity,\n" + "SUM(CASE\n" + "    WHEN Quantity > 30 THEN 3\n" + "    WHEN Quantity > 20 THEN 2\n"
-            + "    WHEN Quantity > 10 THEN 1\n" + "    ELSE 0\n" + "END) AS new_sum_quant\n" + "FROM OrderDetails");
+            + "    WHEN Quantity > 10 THEN 1\n" + "    ELSE 0\n" + "END) AS new_sum_quant\n" + "FROM OrderDetails "
+            + "GROUP BY Quantity");
     Assert.assertEquals(pinotQuery.getSelectList().get(0).getIdentifier().getName(), "Quantity");
     asFunc = pinotQuery.getSelectList().get(1).getFunctionCall();
     Assert.assertEquals(asFunc.getOperator(), SqlKind.AS.name());
@@ -2477,7 +2478,7 @@ public class CalciteSqlCompilerTest {
     Assert.assertTrue(pinotQuery.getQueryOptions().containsKey("skipUpsert"));
 
     // Check for the query where the literal has semicolon
-    sql = "select col1, count(*) from foo where col1 = 'x;y' option(skipUpsert=true);";
+    sql = "select col1, count(*) from foo where col1 = 'x;y' GROUP BY col1 option(skipUpsert=true);";
     pinotQuery = CalciteSqlParser.compileToPinotQuery(sql);
     Assert.assertEquals(pinotQuery.getQueryOptionsSize(), 1);
     Assert.assertTrue(pinotQuery.getQueryOptions().containsKey("skipUpsert"));
@@ -2501,4 +2502,17 @@ public class CalciteSqlCompilerTest {
         "        SELECT col1, count(*) FROM foo GROUP BY col1;   "
             + "SELECT col2, count(*) FROM foo GROUP BY col2             "));
   }
+
+  @Test
+  public void testInvalidQueryWithAggregateFunction() {
+    Assert.expectThrows(SqlCompilationException.class,
+        () -> CalciteSqlParser.compileToPinotQuery("SELECT col1, count(*) from foo"));
+
+    Assert.expectThrows(SqlCompilationException.class,
+       () -> CalciteSqlParser.compileToPinotQuery("SELECT UPPER(col1), count(*) from foo"));
+
+    Assert.expectThrows(SqlCompilationException.class,
+        () -> CalciteSqlParser.compileToPinotQuery("SELECT UPPER(col1), avg(col2) from foo"));
+
+  }
 }
diff --git a/pinot-common/src/test/resources/sql_queries.list b/pinot-common/src/test/resources/sql_queries.list
index 3897ffa..b75d28f 100644
--- a/pinot-common/src/test/resources/sql_queries.list
+++ b/pinot-common/src/test/resources/sql_queries.list
@@ -820,8 +820,8 @@ select mapKey(mapField,k1) from baseballStats where mapKey(mapField,k1) = 'v1'
 SELECT count(c1), sum(c1), min(c1), max(c1), avg(c1), minmaxrange(c1), distinctcount(c1), distinctcounthll(c1) FROM foo
 SELECT distinctcountrawhll(c1), fasthll(c1), percentile90(c1), percentileest95(c1), percentiletdigest(c1, 99) FROM foo
 SELECT countmv(c1), summv(c1), minmv(c1), maxmv(c1), avgmv(c1), minmaxrangemv(c1), distinctcountmv(c1), distinctcounthllmv(c1) FROM foo
-SELECT distinctcountrawhllmv(c1), fasthllmv(c1), percentile90mv(c1), percentileest95mv(c1), percentiletdigestmv(c1, 99) FROM foo
+SELECT distinctcountrawhllmv(c1), percentile90mv(c1), percentileest95mv(c1), percentiletdigestmv(c1, 99) FROM foo
 SELECT count(c1), sum(add(c1,c2)), min(div(c1,c2)), max(sub(c1,c2)), avg(add(c1,c2)), minmaxrange(add(c1,c2)), distinctcount(sub(c1,c2)), distinctcounthll(c1) FROM foo
 SELECT distinctcountrawhll(sub(c1,c2)), fasthll(div(c1,c2)), percentile90(sub(c1,c2)), percentileest95(add(c1,c2)), percentiletdigest(add(c1,c2), 99) FROM foo
 SELECT countmv(c1), summv(add(c1,c2)), minmv(div(c1,c2)), maxmv(sub(c1,c2)), avgmv(add(c1,c2)), minmaxrangemv(min(c1,c2)), distinctcountmv(add(c1,c2)), distinctcounthllmv(min(c1,c2)) FROM foo
-SELECT distinctcountrawhllmv(c1), fasthllmv(add(sub(c1,c2),div(c1,c2))), percentile90mv(min(c1,c2)), percentileest95mv(add(c1,c2)), percentiletdigestmv(min(c1,c2), 99) FROM foo
\ No newline at end of file
+SELECT distinctcountrawhllmv(c1), percentile90mv(min(c1,c2)), percentileest95mv(add(c1,c2)), percentiletdigestmv(min(c1,c2), 99) FROM foo
\ No newline at end of file
diff --git a/pinot-controller/src/test/java/org/apache/pinot/controller/recommender/rules/impl/AggregateMetricsRuleTest.java b/pinot-controller/src/test/java/org/apache/pinot/controller/recommender/rules/impl/AggregateMetricsRuleTest.java
index 7ee7c21..283c556 100644
--- a/pinot-controller/src/test/java/org/apache/pinot/controller/recommender/rules/impl/AggregateMetricsRuleTest.java
+++ b/pinot-controller/src/test/java/org/apache/pinot/controller/recommender/rules/impl/AggregateMetricsRuleTest.java
@@ -52,7 +52,7 @@ public class AggregateMetricsRuleTest {
       throws Exception {
     Set<String> metrics = ImmutableSet.of("a", "b", "c");
     InputManager input =
-        createInput(metrics, "select sum(a), sum(b), sum(c) from tableT", "select sum(a), b from tableT2");
+        createInput(metrics, "select sum(a), sum(b), sum(c) from tableT", "select sum(a), avg(b) from tableT2");
     ConfigManager output = new ConfigManager();
     AggregateMetricsRule rule = new AggregateMetricsRule(input, output);
     rule.run();

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org