[GitHub] [calcite] zabetak commented on a change in pull request #2482: CALCITE-4702 - Error when executing query with GROUP BY constant via JDBC adapter

[GitBox <gi...@apache.org>]

zabetak commented on a change in pull request #2482:
URL: https://github.com/apache/calcite/pull/2482#discussion_r797525949



##########
File path: core/src/main/java/org/apache/calcite/sql/SqlDialect.java
##########
@@ -711,6 +711,20 @@ public boolean supportsCharSet() {
     return true;
   }
 
+  /**
+   * Returns whether the dialect supports GROUP BY literals.
+   *
+   * <p>For instance, in {@link DatabaseProduct#REDSHIFT}, the query below is illegal.</p>

Review comment:
       Include also some examples with other types of literals as mentioned in the JIRA case.

##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -200,6 +201,39 @@ private static boolean skipItem(RexNode expr) {
           && "item".equalsIgnoreCase(((RexCall) expr).getOperator().getName());
   }
 
+  @Test void testGroupByDateLiteralSimple() {
+    HepProgramBuilder builder = new HepProgramBuilder();
+    builder.addRuleInstance(AggregateProjectConstantToDummyJoinRule.Config.DEFAULT.toRule());
+    HepPlanner hepPlanner = new HepPlanner(builder.build());
+
+    final String query = "select avg(sal)\n"
+        + "from emp\n"
+        + "group by DATE '2022-01-01'";
+    sql(query).withPlanner(hepPlanner).check();
+  }
+
+  @Test void testGroupByBooleanConstantSimple() {

Review comment:
       Constant -> Literal

##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -200,6 +201,39 @@ private static boolean skipItem(RexNode expr) {
           && "item".equalsIgnoreCase(((RexCall) expr).getOperator().getName());
   }
 
+  @Test void testGroupByDateLiteralSimple() {
+    HepProgramBuilder builder = new HepProgramBuilder();
+    builder.addRuleInstance(AggregateProjectConstantToDummyJoinRule.Config.DEFAULT.toRule());
+    HepPlanner hepPlanner = new HepPlanner(builder.build());
+
+    final String query = "select avg(sal)\n"
+        + "from emp\n"
+        + "group by DATE '2022-01-01'";
+    sql(query).withPlanner(hepPlanner).check();
+  }
+
+  @Test void testGroupByBooleanConstantSimple() {
+    HepProgramBuilder builder = new HepProgramBuilder();
+    builder.addRuleInstance(AggregateProjectConstantToDummyJoinRule.Config.DEFAULT.toRule());
+    HepPlanner hepPlanner = new HepPlanner(builder.build());
+
+    final String query = "select avg(sal)\n"
+        + "from emp\n"
+        + "group by true";
+    sql(query).withPlanner(hepPlanner).check();
+  }
+
+  @Test void testGroupByBooleanConstantMultiple() {
+    HepProgramBuilder builder = new HepProgramBuilder();
+    builder.addRuleInstance(AggregateProjectConstantToDummyJoinRule.Config.DEFAULT.toRule());
+    HepPlanner hepPlanner = new HepPlanner(builder.build());
+
+    final String query = "select avg(sal)\n"
+        + "from emp\n"
+        + "group by false, deptno, true, true, empno, false, 'ab', DATE '2022-01-01'";
+    sql(query).withPlanner(hepPlanner).check();

Review comment:
       withPlanner -> withRule

##########
File path: core/src/main/java/org/apache/calcite/rel/rules/AggregateProjectConstantToDummyJoinRule.java
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.rel.rules;
+
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelRule;
+import org.apache.calcite.rel.core.Aggregate;
+import org.apache.calcite.rel.core.JoinRelType;
+import org.apache.calcite.rel.core.Project;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.tools.RelBuilder;
+import org.apache.calcite.util.ImmutableBitSet;
+
+import com.google.common.collect.ImmutableList;
+
+import org.immutables.value.Value;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Planner rule that recognizes  a {@link org.apache.calcite.rel.core.Aggregate}
+ * on top of a {@link org.apache.calcite.rel.core.Project} where the aggregate's group set
+ * contains boolean literals (true, false), and removes the literals from the group keys by joining
+ * with a dummy table of boolean literals.
+ *
+ * <pre>{@code
+ * select avg(sal)
+ * from emp
+ * group by true;
+ * }</pre>
+ * becomes
+ * <pre>{@code
+ * select avg(sal)
+ * from emp, (select true x) dummy
+ * group by dummy.x;
+ * }</pre>
+ */
+@Value.Enclosing
+public final class AggregateProjectConstantToDummyJoinRule
+    extends RelRule<AggregateProjectConstantToDummyJoinRule.Config> {
+
+  /** Creates an AggregateProjectConstantToDummyJoinRule. */
+  private AggregateProjectConstantToDummyJoinRule(Config config) {
+    super(config);
+  }
+
+  @Override public boolean matches(RelOptRuleCall call) {
+    final Aggregate aggregate = call.rel(0);
+    final Project project = call.rel(1);
+
+    for (int groupKey: aggregate.getGroupSet().asList()) {
+      if (groupKey >= aggregate.getRowType().getFieldCount()) {
+        continue;
+      }
+      RexNode groupKeyProject = project.getProjects().get(groupKey);
+      if (groupKeyProject instanceof RexLiteral) {
+        return true;
+      }
+    }
+
+    return false;
+  }
+
+  @Override public void onMatch(RelOptRuleCall call) {
+    final Aggregate aggregate = call.rel(0);
+    final Project project = call.rel(1);
+
+    RelBuilder builder = call.builder();
+    RexBuilder rexBuilder = builder.getRexBuilder();
+
+    builder.push(project.getInput());
+    int offset = project.getInput().getRowType().getFieldCount();
+
+    final ImmutableList.Builder<ImmutableList<RexLiteral>> tuples =
+        ImmutableList.builder();
+    ImmutableList.Builder<RexLiteral> b = ImmutableList.builder();
+    RelDataTypeFactory.Builder relDataTypeBuilder = rexBuilder.getTypeFactory().builder();
+
+    List<RexNode> projects = project.getProjects();
+    for (int i = 0; i < projects.size(); i++) {
+      RexNode node = projects.get(i);
+      if (node instanceof RexLiteral) {
+        b.add((RexLiteral) node);
+        relDataTypeBuilder.add(project.getRowType().getFieldList().get(i));
+      }
+    }
+    tuples.add(b.build());
+    builder.values(tuples.build(), relDataTypeBuilder.build());

Review comment:
       ```suggestion
       RelDataTypeFactory.Builder valuesType = rexBuilder.getTypeFactory().builder();
       List<RexLiteral> literals = new ArrayList<>();
       List<RexNode> projects = project.getProjects();
       for (int i = 0; i < projects.size(); i++) {
         RexNode node = projects.get(i);
         if (node instanceof RexLiteral) {
           literals.add((RexLiteral) node);
           valuesType.add(project.getRowType().getFieldList().get(i));
         }
       }
       builder.values(ImmutableList.of(literals), valuesType.build());
   ```
   Small suggestion for readability.

##########
File path: core/src/main/java/org/apache/calcite/rel/rules/AggregateProjectConstantToDummyJoinRule.java
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.rel.rules;
+
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelRule;
+import org.apache.calcite.rel.core.Aggregate;
+import org.apache.calcite.rel.core.JoinRelType;
+import org.apache.calcite.rel.core.Project;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.tools.RelBuilder;
+import org.apache.calcite.util.ImmutableBitSet;
+
+import com.google.common.collect.ImmutableList;
+
+import org.immutables.value.Value;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Planner rule that recognizes  a {@link org.apache.calcite.rel.core.Aggregate}
+ * on top of a {@link org.apache.calcite.rel.core.Project} where the aggregate's group set
+ * contains boolean literals (true, false), and removes the literals from the group keys by joining
+ * with a dummy table of boolean literals.
+ *
+ * <pre>{@code
+ * select avg(sal)
+ * from emp
+ * group by true;
+ * }</pre>
+ * becomes
+ * <pre>{@code
+ * select avg(sal)
+ * from emp, (select true x) dummy
+ * group by dummy.x;
+ * }</pre>
+ */
+@Value.Enclosing
+public final class AggregateProjectConstantToDummyJoinRule
+    extends RelRule<AggregateProjectConstantToDummyJoinRule.Config> {
+
+  /** Creates an AggregateProjectConstantToDummyJoinRule. */
+  private AggregateProjectConstantToDummyJoinRule(Config config) {
+    super(config);
+  }
+
+  @Override public boolean matches(RelOptRuleCall call) {
+    final Aggregate aggregate = call.rel(0);
+    final Project project = call.rel(1);
+
+    for (int groupKey: aggregate.getGroupSet().asList()) {
+      if (groupKey >= aggregate.getRowType().getFieldCount()) {
+        continue;
+      }
+      RexNode groupKeyProject = project.getProjects().get(groupKey);
+      if (groupKeyProject instanceof RexLiteral) {
+        return true;
+      }
+    }
+
+    return false;
+  }
+
+  @Override public void onMatch(RelOptRuleCall call) {
+    final Aggregate aggregate = call.rel(0);
+    final Project project = call.rel(1);
+
+    RelBuilder builder = call.builder();
+    RexBuilder rexBuilder = builder.getRexBuilder();
+
+    builder.push(project.getInput());
+    int offset = project.getInput().getRowType().getFieldCount();
+
+    final ImmutableList.Builder<ImmutableList<RexLiteral>> tuples =
+        ImmutableList.builder();
+    ImmutableList.Builder<RexLiteral> b = ImmutableList.builder();
+    RelDataTypeFactory.Builder relDataTypeBuilder = rexBuilder.getTypeFactory().builder();
+
+    List<RexNode> projects = project.getProjects();
+    for (int i = 0; i < projects.size(); i++) {
+      RexNode node = projects.get(i);
+      if (node instanceof RexLiteral) {
+        b.add((RexLiteral) node);
+        relDataTypeBuilder.add(project.getRowType().getFieldList().get(i));
+      }
+    }
+    tuples.add(b.build());
+    builder.values(tuples.build(), relDataTypeBuilder.build());
+    builder.join(JoinRelType.INNER, rexBuilder.makeLiteral(true));
+
+    List<RexNode> newProjects = new ArrayList<>();
+
+    int literalCounter = 0;
+    for (RexNode exp : project.getProjects()) {
+      if (exp instanceof RexLiteral) {
+        newProjects.add(builder.field(offset + literalCounter++));
+      } else {
+        newProjects.add(exp);
+      }
+    }
+
+    builder.project(newProjects);
+    builder.aggregate(
+        builder.groupKey(
+            aggregate.getGroupSet(), (Iterable<ImmutableBitSet>) aggregate.getGroupSets()),

Review comment:
       Is the `(Iterable<ImmutableBitSet>)` cast necessary?

##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -200,6 +201,39 @@ private static boolean skipItem(RexNode expr) {
           && "item".equalsIgnoreCase(((RexCall) expr).getOperator().getName());
   }
 
+  @Test void testGroupByDateLiteralSimple() {
+    HepProgramBuilder builder = new HepProgramBuilder();
+    builder.addRuleInstance(AggregateProjectConstantToDummyJoinRule.Config.DEFAULT.toRule());
+    HepPlanner hepPlanner = new HepPlanner(builder.build());
+
+    final String query = "select avg(sal)\n"
+        + "from emp\n"
+        + "group by DATE '2022-01-01'";
+    sql(query).withPlanner(hepPlanner).check();

Review comment:
       I think the test can be simplified by calling `withRule` instead of `withPlanner`.

##########
File path: core/src/main/java/org/apache/calcite/rel/rules/AggregateProjectConstantToDummyJoinRule.java
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.rel.rules;
+
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelRule;
+import org.apache.calcite.rel.core.Aggregate;
+import org.apache.calcite.rel.core.JoinRelType;
+import org.apache.calcite.rel.core.Project;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.tools.RelBuilder;
+import org.apache.calcite.util.ImmutableBitSet;
+
+import com.google.common.collect.ImmutableList;
+
+import org.immutables.value.Value;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Planner rule that recognizes  a {@link org.apache.calcite.rel.core.Aggregate}
+ * on top of a {@link org.apache.calcite.rel.core.Project} where the aggregate's group set
+ * contains boolean literals (true, false), and removes the literals from the group keys by joining
+ * with a dummy table of boolean literals.
+ *
+ * <pre>{@code
+ * select avg(sal)
+ * from emp
+ * group by true;
+ * }</pre>
+ * becomes
+ * <pre>{@code
+ * select avg(sal)
+ * from emp, (select true x) dummy
+ * group by dummy.x;
+ * }</pre>

Review comment:
       I think the comment about boolean literals is now obsolete. Can you update the description and examples?

##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -200,6 +201,39 @@ private static boolean skipItem(RexNode expr) {
           && "item".equalsIgnoreCase(((RexCall) expr).getOperator().getName());
   }
 
+  @Test void testGroupByDateLiteralSimple() {
+    HepProgramBuilder builder = new HepProgramBuilder();
+    builder.addRuleInstance(AggregateProjectConstantToDummyJoinRule.Config.DEFAULT.toRule());
+    HepPlanner hepPlanner = new HepPlanner(builder.build());
+
+    final String query = "select avg(sal)\n"
+        + "from emp\n"
+        + "group by DATE '2022-01-01'";
+    sql(query).withPlanner(hepPlanner).check();
+  }
+
+  @Test void testGroupByBooleanConstantSimple() {
+    HepProgramBuilder builder = new HepProgramBuilder();
+    builder.addRuleInstance(AggregateProjectConstantToDummyJoinRule.Config.DEFAULT.toRule());
+    HepPlanner hepPlanner = new HepPlanner(builder.build());
+
+    final String query = "select avg(sal)\n"
+        + "from emp\n"
+        + "group by true";
+    sql(query).withPlanner(hepPlanner).check();
+  }
+
+  @Test void testGroupByBooleanConstantMultiple() {

Review comment:
       We have constants which are not boolean. Moreover we agreed to use the term literal instead of contant. Please rename the method accordingly.

##########
File path: core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
##########
@@ -200,6 +201,39 @@ private static boolean skipItem(RexNode expr) {
           && "item".equalsIgnoreCase(((RexCall) expr).getOperator().getName());
   }
 
+  @Test void testGroupByDateLiteralSimple() {
+    HepProgramBuilder builder = new HepProgramBuilder();
+    builder.addRuleInstance(AggregateProjectConstantToDummyJoinRule.Config.DEFAULT.toRule());
+    HepPlanner hepPlanner = new HepPlanner(builder.build());
+
+    final String query = "select avg(sal)\n"
+        + "from emp\n"
+        + "group by DATE '2022-01-01'";
+    sql(query).withPlanner(hepPlanner).check();
+  }
+
+  @Test void testGroupByBooleanConstantSimple() {
+    HepProgramBuilder builder = new HepProgramBuilder();
+    builder.addRuleInstance(AggregateProjectConstantToDummyJoinRule.Config.DEFAULT.toRule());
+    HepPlanner hepPlanner = new HepPlanner(builder.build());
+
+    final String query = "select avg(sal)\n"
+        + "from emp\n"
+        + "group by true";
+    sql(query).withPlanner(hepPlanner).check();

Review comment:
       withPlanner -> withRule




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org