[pinot] branch master updated: [multistage][bugfix] fix group-by without agg call plan failure (#9383)

[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 3112cac380 [multistage][bugfix] fix group-by without agg call plan failure (#9383)
3112cac380 is described below

commit 3112cac380218008ea50413eb9abeaef8ab814ce
Author: Rong Rong <wa...@gmail.com>
AuthorDate: Tue Sep 13 01:09:55 2022 -0700

    [multistage][bugfix] fix group-by without agg call plan failure (#9383)
    
    * add test
    
    * add rel rule to allow distinct agg
    
    Co-authored-by: Rong Rong <ro...@startree.ai>
---
 .../rel/rules/PinotAggregateExchangeNodeInsertRule.java     |  3 +--
 .../calcite/rel/rules/PinotJoinExchangeNodeInsertRule.java  |  3 +--
 .../rel/rules/PinotLogicalSortFetchEliminationRule.java     |  3 +--
 .../java/org/apache/calcite/rel/rules/PinotRuleUtils.java   |  9 +++++++++
 .../calcite/rel/rules/PinotSortExchangeNodeInsertRule.java  |  3 +--
 .../main/java/org/apache/pinot/query/QueryEnvironment.java  | 13 ++++++++-----
 .../org/apache/pinot/query/QueryEnvironmentTestBase.java    |  2 ++
 .../org/apache/pinot/query/runtime/QueryRunnerTest.java     |  4 ++++
 8 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotAggregateExchangeNodeInsertRule.java b/pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotAggregateExchangeNodeInsertRule.java
index fba5f4fc12..5545798b48 100644
--- a/pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotAggregateExchangeNodeInsertRule.java
+++ b/pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotAggregateExchangeNodeInsertRule.java
@@ -33,7 +33,6 @@ import org.apache.calcite.rel.RelDistributions;
 import org.apache.calcite.rel.RelNode;
 import org.apache.calcite.rel.core.Aggregate;
 import org.apache.calcite.rel.core.AggregateCall;
-import org.apache.calcite.rel.core.RelFactories;
 import org.apache.calcite.rel.hint.RelHint;
 import org.apache.calcite.rel.logical.LogicalAggregate;
 import org.apache.calcite.rel.logical.LogicalExchange;
@@ -69,7 +68,7 @@ import org.apache.pinot.query.planner.hints.PinotRelationalHints;
  */
 public class PinotAggregateExchangeNodeInsertRule extends RelOptRule {
   public static final PinotAggregateExchangeNodeInsertRule INSTANCE =
-      new PinotAggregateExchangeNodeInsertRule(RelFactories.LOGICAL_BUILDER);
+      new PinotAggregateExchangeNodeInsertRule(PinotRuleUtils.PINOT_REL_FACTORY);
   private static final Set<SqlKind> SUPPORTED_AGG_KIND = ImmutableSet.of(
       SqlKind.SUM, SqlKind.SUM0, SqlKind.MIN, SqlKind.MAX, SqlKind.COUNT);
 
diff --git a/pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotJoinExchangeNodeInsertRule.java b/pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotJoinExchangeNodeInsertRule.java
index 39af45cff7..dc623e3c8e 100644
--- a/pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotJoinExchangeNodeInsertRule.java
+++ b/pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotJoinExchangeNodeInsertRule.java
@@ -25,7 +25,6 @@ import org.apache.calcite.plan.RelOptRuleCall;
 import org.apache.calcite.rel.RelDistributions;
 import org.apache.calcite.rel.RelNode;
 import org.apache.calcite.rel.core.Join;
-import org.apache.calcite.rel.core.RelFactories;
 import org.apache.calcite.rel.hint.RelHint;
 import org.apache.calcite.rel.logical.LogicalExchange;
 import org.apache.calcite.rel.logical.LogicalJoin;
@@ -40,7 +39,7 @@ import org.apache.pinot.query.planner.hints.PinotRelationalHints;
  */
 public class PinotJoinExchangeNodeInsertRule extends RelOptRule {
   public static final PinotJoinExchangeNodeInsertRule INSTANCE =
-      new PinotJoinExchangeNodeInsertRule(RelFactories.LOGICAL_BUILDER);
+      new PinotJoinExchangeNodeInsertRule(PinotRuleUtils.PINOT_REL_FACTORY);
 
   public PinotJoinExchangeNodeInsertRule(RelBuilderFactory factory) {
     super(operand(LogicalJoin.class, any()), factory, null);
diff --git a/pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotLogicalSortFetchEliminationRule.java b/pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotLogicalSortFetchEliminationRule.java
index a85035b85b..e6a32c387b 100644
--- a/pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotLogicalSortFetchEliminationRule.java
+++ b/pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotLogicalSortFetchEliminationRule.java
@@ -20,7 +20,6 @@ package org.apache.calcite.rel.rules;
 
 import org.apache.calcite.plan.RelOptRule;
 import org.apache.calcite.plan.RelOptRuleCall;
-import org.apache.calcite.rel.core.RelFactories;
 import org.apache.calcite.rel.core.Sort;
 import org.apache.calcite.rel.logical.LogicalSort;
 import org.apache.calcite.tools.RelBuilderFactory;
@@ -31,7 +30,7 @@ import org.apache.calcite.tools.RelBuilderFactory;
  */
 public class PinotLogicalSortFetchEliminationRule extends RelOptRule {
   public static final PinotLogicalSortFetchEliminationRule INSTANCE =
-      new PinotLogicalSortFetchEliminationRule(RelFactories.LOGICAL_BUILDER);
+      new PinotLogicalSortFetchEliminationRule(PinotRuleUtils.PINOT_REL_FACTORY);
 
   public PinotLogicalSortFetchEliminationRule(RelBuilderFactory factory) {
     super(operand(LogicalSort.class, any()), factory, null);
diff --git a/pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotRuleUtils.java b/pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotRuleUtils.java
index 97b1074b7f..d2634018ef 100644
--- a/pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotRuleUtils.java
+++ b/pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotRuleUtils.java
@@ -18,12 +18,21 @@
  */
 package org.apache.calcite.rel.rules;
 
+import org.apache.calcite.plan.Contexts;
 import org.apache.calcite.plan.hep.HepRelVertex;
 import org.apache.calcite.rel.RelNode;
 import org.apache.calcite.rel.core.Exchange;
+import org.apache.calcite.rel.core.RelFactories;
+import org.apache.calcite.tools.RelBuilder;
+import org.apache.calcite.tools.RelBuilderFactory;
 
 
 public class PinotRuleUtils {
+  private static final RelBuilder.Config DEFAULT_CONFIG =
+      RelBuilder.Config.DEFAULT.withAggregateUnique(true).withPushJoinCondition(true);
+
+  public static final RelBuilderFactory PINOT_REL_FACTORY =
+      RelBuilder.proto(Contexts.of(RelFactories.DEFAULT_STRUCT, DEFAULT_CONFIG));
 
   private PinotRuleUtils() {
     // do not instantiate.
diff --git a/pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotSortExchangeNodeInsertRule.java b/pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotSortExchangeNodeInsertRule.java
index c590f2b652..5e1d0fedb8 100644
--- a/pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotSortExchangeNodeInsertRule.java
+++ b/pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotSortExchangeNodeInsertRule.java
@@ -25,7 +25,6 @@ import org.apache.calcite.plan.hep.HepRelVertex;
 import org.apache.calcite.rel.RelDistributions;
 import org.apache.calcite.rel.RelNode;
 import org.apache.calcite.rel.core.Exchange;
-import org.apache.calcite.rel.core.RelFactories;
 import org.apache.calcite.rel.core.Sort;
 import org.apache.calcite.rel.logical.LogicalExchange;
 import org.apache.calcite.rel.logical.LogicalSort;
@@ -37,7 +36,7 @@ import org.apache.calcite.tools.RelBuilderFactory;
  */
 public class PinotSortExchangeNodeInsertRule extends RelOptRule {
   public static final PinotSortExchangeNodeInsertRule INSTANCE =
-      new PinotSortExchangeNodeInsertRule(RelFactories.LOGICAL_BUILDER);
+      new PinotSortExchangeNodeInsertRule(PinotRuleUtils.PINOT_REL_FACTORY);
 
   public PinotSortExchangeNodeInsertRule(RelBuilderFactory factory) {
     super(operand(LogicalSort.class, any()), factory, null);
diff --git a/pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java b/pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java
index 1f84101e53..87156476e9 100644
--- a/pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java
+++ b/pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java
@@ -92,7 +92,12 @@ public class QueryEnvironment {
 
     _config = Frameworks.newConfigBuilder().traitDefs()
         .operatorTable(new ChainedSqlOperatorTable(Arrays.asList(SqlStdOperatorTable.instance(), _catalogReader)))
-        .defaultSchema(_rootSchema.plus()).build();
+        .defaultSchema(_rootSchema.plus())
+        .sqlToRelConverterConfig(SqlToRelConverter.config()
+            .withHintStrategyTable(getHintStrategyTable())
+            .addRelBuilderConfigTransform(c -> c.withPushJoinCondition(true))
+            .addRelBuilderConfigTransform(c -> c.withAggregateUnique(true)))
+        .build();
 
     // optimizer rules
     _logicalRuleSet = PinotQueryRuleSets.LOGICAL_OPT_RULES;
@@ -191,8 +196,7 @@ public class QueryEnvironment {
     RelOptCluster cluster = RelOptCluster.create(plannerContext.getRelOptPlanner(), rexBuilder);
     SqlToRelConverter sqlToRelConverter =
         new SqlToRelConverter(plannerContext.getPlanner(), plannerContext.getValidator(), _catalogReader, cluster,
-            StandardConvertletTable.INSTANCE,
-            SqlToRelConverter.config().withHintStrategyTable(getHintStrategyTable(plannerContext)));
+            StandardConvertletTable.INSTANCE, _config.getSqlToRelConverterConfig());
     return sqlToRelConverter.convertQuery(parsed, false, true);
   }
 
@@ -218,8 +222,7 @@ public class QueryEnvironment {
   // utils
   // --------------------------------------------------------------------------
 
-  // TODO: add hint strategy table based on plannerContext.
-  private HintStrategyTable getHintStrategyTable(PlannerContext plannerContext) {
+  private HintStrategyTable getHintStrategyTable() {
     return HintStrategyTable.builder().build();
   }
 }
diff --git a/pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java b/pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java
index d2939b1b7f..3d633e932f 100644
--- a/pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java
+++ b/pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java
@@ -65,6 +65,8 @@ public class QueryEnvironmentTestBase {
             + "AND AVG(a.col3) = 5"},
         new Object[]{"SELECT dateTrunc('DAY', ts) FROM a LIMIT 10"},
         new Object[]{"SELECT dateTrunc('DAY', a.ts + b.ts) FROM a JOIN b on a.col1 = b.col1 AND a.col2 = b.col2"},
+        new Object[]{"SELECT a.col2, a.col3 FROM a JOIN b ON a.col1 = b.col1 "
+            + " WHERE a.col3 >= 0 GROUP BY a.col2, a.col3"},
     };
   }
 }
diff --git a/pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTest.java b/pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTest.java
index 22d21106b7..d2ac8c95c8 100644
--- a/pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTest.java
+++ b/pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTest.java
@@ -174,6 +174,10 @@ public class QueryRunnerTest extends QueryRunnerTestBase {
         //   - on intermediate stage
         new Object[]{"SELECT dateTrunc('DAY', round(a.ts, b.ts)) FROM a JOIN b "
             + "ON a.col1 = b.col1 AND a.col2 = b.col2", 15},
+
+        // Distinct value done via GROUP BY with empty expr aggregation list.
+        new Object[]{"SELECT a.col2, a.col3 FROM a JOIN b ON a.col1 = b.col1 "
+            + " WHERE b.col3 > 0 GROUP BY a.col2, a.col3", 5},
     };
   }
 }


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