address review comments

apache · Oct 29, 2024 · 2235732 · 2235732
1 parent ac36438
commit 2235732
Show file tree

Hide file tree

Showing 2 changed files with 10 additions and 5 deletions.
diff --git a/...src/main/java/org/apache/pinot/core/query/aggregation/groupby/DefaultGroupByExecutor.java b/...src/main/java/org/apache/pinot/core/query/aggregation/groupby/DefaultGroupByExecutor.java
@@ -139,6 +139,12 @@ public DefaultGroupByExecutor(QueryContext queryContext, AggregationFunction[] a
     }
   }
 
+  /**
+   * Retrieve the sizes of GroupBy expressions from IN an EQ predicates found in the filter context, if available.
+   * 1. If the filter context is null or lacks GroupBy expressions, return null.
+   * 2. Ensure the top-level filter context consists solely of AND-type filters; other types for example OR we cannot
+   *    guarantee deterministic sizes for GroupBy expressions.
+   */
   private Map<ExpressionContext, Integer> getGroupByExpressionSizesFromPredicates(QueryContext queryContext) {
     FilterContext filterContext = queryContext.getFilter();
     if (filterContext == null || queryContext.getGroupByExpressions() == null) {
@@ -169,7 +175,7 @@ private Map<ExpressionContext, Integer> getGroupByExpressionSizesFromPredicates(
             predicate -> (predicate.getType() == Predicate.Type.IN)
                 ? ((InPredicate) predicate).getValues().size()
                 : 1,
-            Integer::sum
+            Integer::min
         ));
 
     // Populate the group-by expressions with sizes from the predicate map

diff --git a/...org/apache/pinot/core/query/aggregation/groupby/DictionaryBasedGroupKeyGeneratorTest.java b/...org/apache/pinot/core/query/aggregation/groupby/DictionaryBasedGroupKeyGeneratorTest.java
@@ -57,7 +57,9 @@
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 
-import static org.testng.Assert.*;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertTrue;
 
 
 public class DictionaryBasedGroupKeyGeneratorTest {
@@ -459,9 +461,6 @@ public Object[][] groupByResultHolderCapacityDataProvider() {
         // Mixed predicates
         {"SELECT COUNT(s9), s1, s3 FROM testTable WHERE s1 IN (1, 2, 3) AND s3 = 4 GROUP BY s1, s3 LIMIT 10;", 3},
         {"SELECT COUNT(*), s1, s3 FROM testTable WHERE s1 = 1 AND s3 IN (4, 5) GROUP BY s1, s3 LIMIT 10;", 2},
-        // Multiple IN Predicate columns with same column name and different values
-        {"SELECT COUNT(s9), s1, s2 FROM testTable WHERE s1 IN (1, 2, 3) AND s1 IN (4, 5) AND s2 IN (6, 7)"
-            + " GROUP BY s1, s2 LIMIT 10;", 10},
         // No filter -> s1 has cardinality 100
         {"SELECT COUNT(s9), s1 FROM testTable GROUP BY s1 LIMIT 1000;", 100},
         // No matching filter EQ predicate in group-by expression -> s2 has cardinality 100