You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by jh...@apache.org on 2020/01/14 02:02:15 UTC
[calcite] 01/02: [CALCITE-3466] JDBC adapter incorrectly drops
GROUP BY clause of sub-query (Wang Weidong)
This is an automated email from the ASF dual-hosted git repository.
jhyde pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/calcite.git
commit 571cf95a1564c925bf73e4dae3a6c63199b17c4e
Author: Wang Weidong <we...@126.com>
AuthorDate: Tue Nov 26 14:45:37 2019 +0800
[CALCITE-3466] JDBC adapter incorrectly drops GROUP BY clause of sub-query (Wang Weidong)
Close apache/calcite#1606
---
.../apache/calcite/rel/rel2sql/SqlImplementor.java | 51 ++++++++++++++--------
.../calcite/rel/rel2sql/RelToSqlConverterTest.java | 47 ++++++++++++++++++++
2 files changed, 80 insertions(+), 18 deletions(-)
diff --git a/core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java b/core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java
index b1fdb05..dc26f0d 100644
--- a/core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java
+++ b/core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java
@@ -1240,22 +1240,7 @@ public abstract class SqlImplementor {
* @return A builder
*/
public Builder builder(RelNode rel, Clause... clauses) {
- final Clause maxClause = maxClause();
- boolean needNew = false;
- // If old and new clause are equal and belong to below set,
- // then new SELECT wrap is not required
- Set<Clause> nonWrapSet = ImmutableSet.of(Clause.SELECT);
- for (Clause clause : clauses) {
- if (maxClause.ordinal() > clause.ordinal()
- || (maxClause == clause && !nonWrapSet.contains(clause))) {
- needNew = true;
- }
- }
- if (rel instanceof Aggregate
- && !dialect.supportsNestedAggregations()
- && hasNestedAggregations((Aggregate) rel)) {
- needNew = true;
- }
+ final boolean needNew = needNewSubQuery(rel, clauses);
SqlSelect select;
Expressions.FluentList<Clause> clauseList = Expressions.list();
@@ -1327,6 +1312,37 @@ public abstract class SqlImplementor {
needNew ? null : aliases);
}
+ /** Returns whether a new sub-query is required. */
+ private boolean needNewSubQuery(RelNode rel, Clause[] clauses) {
+ final Clause maxClause = maxClause();
+ // If old and new clause are equal and belong to below set,
+ // then new SELECT wrap is not required
+ final Set<Clause> nonWrapSet = ImmutableSet.of(Clause.SELECT);
+ for (Clause clause : clauses) {
+ if (maxClause.ordinal() > clause.ordinal()
+ || (maxClause == clause
+ && !nonWrapSet.contains(clause))) {
+ return true;
+ }
+ }
+
+ if (rel instanceof Aggregate) {
+ final Aggregate agg = (Aggregate) rel;
+ final boolean hasNestedAgg = hasNestedAggregations(agg);
+ if (!dialect.supportsNestedAggregations()
+ && hasNestedAgg) {
+ return true;
+ }
+
+ if (this.clauses.contains(Clause.GROUP_BY)) {
+ // Avoid losing the distinct attribute of inner aggregate.
+ return !hasNestedAgg || Aggregate.isNotGrandTotal(agg);
+ }
+ }
+
+ return false;
+ }
+
private boolean hasNestedAggregations(Aggregate rel) {
if (node instanceof SqlSelect) {
final SqlNodeList selectList = ((SqlSelect) node).getSelectList();
@@ -1352,8 +1368,7 @@ public abstract class SqlImplementor {
return false;
}
- // make private?
- public Clause maxClause() {
+ private Clause maxClause() {
Clause maxClause = null;
for (Clause clause : clauses) {
if (maxClause == null || clause.ordinal() > maxClause.ordinal()) {
diff --git a/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java b/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
index a090166..b9025ab 100644
--- a/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
+++ b/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
@@ -349,6 +349,53 @@ public class RelToSqlConverterTest {
.ok(expectedMySql);
}
+ /** Tests a query with GROUP BY and a sub-query which is also with GROUP BY.
+ * If we flatten sub-queries, the number of rows going into AVG becomes
+ * incorrect. */
+ @Test public void testSelectQueryWithGroupBySubQuery1() {
+ final String query = "select \"product_class_id\", avg(\"product_id\")\n"
+ + "from (select \"product_class_id\", \"product_id\", avg(\"product_class_id\")\n"
+ + "from \"product\"\n"
+ + "group by \"product_class_id\", \"product_id\") as t\n"
+ + "group by \"product_class_id\"";
+ final String expected = "SELECT \"product_class_id\", AVG(\"product_id\")\n"
+ + "FROM (SELECT \"product_class_id\", \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\"\n"
+ + "GROUP BY \"product_class_id\", \"product_id\") AS \"t1\"\n"
+ + "GROUP BY \"product_class_id\"";
+ sql(query).ok(expected);
+ }
+
+ /** Tests query without GROUP BY but an aggregate function
+ * and a sub-query which is with GROUP BY. */
+ @Test public void testSelectQueryWithGroupBySubQuery2() {
+ final String query = "select sum(\"product_id\")\n"
+ + "from (select \"product_class_id\", \"product_id\"\n"
+ + "from \"product\"\n"
+ + "group by \"product_class_id\", \"product_id\") as t";
+ final String expected = "SELECT SUM(\"product_id\")\n"
+ + "FROM (SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\"\n"
+ + "GROUP BY \"product_class_id\", \"product_id\") AS \"t1\"";
+ final String expectedMysql = "SELECT SUM(`product_id`)\n"
+ + "FROM (SELECT `product_id`\n"
+ + "FROM `foodmart`.`product`\n"
+ + "GROUP BY `product_class_id`, `product_id`) AS `t1`";
+ sql(query)
+ .ok(expected)
+ .withMysql()
+ .ok(expectedMysql);
+
+ // Equivalent sub-query that uses SELECT DISTINCT
+ final String query2 = "select sum(\"product_id\")\n"
+ + "from (select distinct \"product_class_id\", \"product_id\"\n"
+ + " from \"product\") as t";
+ sql(query2)
+ .ok(expected)
+ .withMysql()
+ .ok(expectedMysql);
+ }
+
/** CUBE of one column is equivalent to ROLLUP, and Calcite recognizes
* this. */
@Test public void testSelectQueryWithSingletonCube() {