You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/07/12 19:54:02 UTC
[GitHub] [calcite] NobiGo opened a new pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
NobiGo opened a new pull request #2458:
URL: https://github.com/apache/calcite/pull/2458
When the group key is a superset of the union of the group keys, The RelNode will generate the wrong SQL.
--
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
[GitHub] [calcite] vlsi commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r690577028
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -504,9 +542,18 @@ protected Builder buildAggregate(Aggregate e, Builder builder,
SqlStdOperatorTable.ROLLUP.createCall(SqlParserPos.ZERO, groupKeys));
default:
case OTHER:
+ // Make sure that the group sets contains all bits.
+ final List<ImmutableBitSet> groupSets;
+ if (aggregate.getGroupSet()
+ .equals(ImmutableBitSet.union(aggregate.groupSets))) {
+ groupSets = aggregate.getGroupSets();
+ } else {
+ groupSets = new ArrayList<>(aggregate.getGroupSets());
+ groupSets.add(0, aggregate.getGroupSet());
Review comment:
I've no idea how many group sets would appear here, however, it would probably be safer to pre-size the array as `groupSets.size + 1`, then `groupSets.add(aggregate.getGroupSet())`, then `groupSets.addAll(aggregate.getGroupSets())`.
I guess the difference is not significant, however, it would be safer and it would avoid questions during review
--
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
[GitHub] [calcite] NobiGo commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
NobiGo commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r677070221
##########
File path: core/src/main/java/org/apache/calcite/tools/RelBuilder.java
##########
@@ -934,7 +934,15 @@ public GroupKey groupKey(ImmutableBitSet groupSet) {
*
* <p>This method of creating a group key does not allow you to group on new
* expressions, only column projections, but is efficient, especially when you
- * are coming from an existing {@link Aggregate}. */
+ * are coming from an existing {@link Aggregate}.
+ *
+ * <p>This method of creating a group key allow you to create groupKey be a
Review comment:
done
--
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
[GitHub] [calcite] NobiGo commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
NobiGo commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r690868662
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -504,9 +542,18 @@ protected Builder buildAggregate(Aggregate e, Builder builder,
SqlStdOperatorTable.ROLLUP.createCall(SqlParserPos.ZERO, groupKeys));
default:
case OTHER:
+ // Make sure that the group sets contains all bits.
+ final List<ImmutableBitSet> groupSets;
+ if (aggregate.getGroupSet()
+ .equals(ImmutableBitSet.union(aggregate.groupSets))) {
+ groupSets = aggregate.getGroupSets();
+ } else {
+ groupSets = new ArrayList<>(aggregate.getGroupSets());
+ groupSets.add(0, aggregate.getGroupSet());
Review comment:
Done. Thank you for your advice.
--
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
[GitHub] [calcite] hannerwang commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
hannerwang commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r675280915
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -329,7 +330,20 @@ public Result visit(Filter e) {
ImmutableSet.of(Clause.HAVING));
parseCorrelTable(e, x);
final Builder builder = x.builder(e);
- builder.setHaving(builder.context.toSql(null, e.getCondition()));
+ SqlNode condition = builder.context.toSql(null, e.getCondition());
+ if (ImmutableBitSet.union(aggregate.getGroupSets()).cardinality()
+ != aggregate.getGroupSet().cardinality()) {
Review comment:
may "!=" replaced by "<" be better?
--
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
[GitHub] [calcite] NobiGo commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
NobiGo commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r690868395
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -329,7 +330,17 @@ public Result visit(Filter e) {
ImmutableSet.of(Clause.HAVING));
parseCorrelTable(e, x);
final Builder builder = x.builder(e);
- builder.setHaving(builder.context.toSql(null, e.getCondition()));
+ SqlNode condition = builder.context.toSql(null, e.getCondition());
+ SqlNode existHaving = x.asSelect().getHaving();
+ if (existHaving != null) {
+ // if input Aggregate RelNode contains existHaving, need
+ // to create AND expression with connect condition and existHaving
+ // then update input Aggregate's Having condition.
+ condition = SqlUtil.andExpressions(condition, existHaving);
+ x.asSelect().setHaving(condition);
+ } else {
+ builder.setHaving(condition);
+ }
Review comment:
done.
--
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
[GitHub] [calcite] vlsi commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r690570603
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -329,7 +330,17 @@ public Result visit(Filter e) {
ImmutableSet.of(Clause.HAVING));
parseCorrelTable(e, x);
final Builder builder = x.builder(e);
- builder.setHaving(builder.context.toSql(null, e.getCondition()));
+ SqlNode condition = builder.context.toSql(null, e.getCondition());
+ SqlNode existHaving = x.asSelect().getHaving();
+ if (existHaving != null) {
+ // if input Aggregate RelNode contains existHaving, need
+ // to create AND expression with connect condition and existHaving
+ // then update input Aggregate's Having condition.
+ condition = SqlUtil.andExpressions(condition, existHaving);
+ x.asSelect().setHaving(condition);
+ } else {
+ builder.setHaving(condition);
+ }
Review comment:
A minor style note: it often works better if a shorter `if` branch is located the first. It makes it easier to relate `else` and the corresponding `if` condition.
```suggestion
if (existHaving == null) {
builder.setHaving(condition);
} else {
// if input Aggregate RelNode contains existHaving, need
// to create AND expression with connect condition and existHaving
// then update input Aggregate's Having condition.
condition = SqlUtil.andExpressions(condition, existHaving);
x.asSelect().setHaving(condition);
}
```
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -504,9 +542,18 @@ protected Builder buildAggregate(Aggregate e, Builder builder,
SqlStdOperatorTable.ROLLUP.createCall(SqlParserPos.ZERO, groupKeys));
default:
case OTHER:
+ // Make sure that the group sets contains all bits.
+ final List<ImmutableBitSet> groupSets;
+ if (aggregate.getGroupSet()
+ .equals(ImmutableBitSet.union(aggregate.groupSets))) {
+ groupSets = aggregate.getGroupSets();
+ } else {
+ groupSets = new ArrayList<>(aggregate.getGroupSets());
+ groupSets.add(0, aggregate.getGroupSet());
Review comment:
I've no idea how many group sets would appear here, however, it would probably be safer to pre-size the array as `groupSets.size + 1`, then `groupSets.add(aggregate.getGroupSet())`, then `groupSets.addAll(aggregate.getGroupSets())`.
I guess the difference is not significant, however, it would be safer and it would avoid questions during review
--
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
[GitHub] [calcite] NobiGo commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
NobiGo commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r694420794
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -332,14 +332,14 @@ public Result visit(Filter e) {
final Builder builder = x.builder(e);
SqlNode condition = builder.context.toSql(null, e.getCondition());
SqlNode existHaving = x.asSelect().getHaving();
- if (existHaving != null) {
+ if (existHaving == null) {
+ builder.setHaving(condition);
+ } else {
Review comment:
done.
--
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
[GitHub] [calcite] julianhyde commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r694203691
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -332,14 +332,14 @@ public Result visit(Filter e) {
final Builder builder = x.builder(e);
SqlNode condition = builder.context.toSql(null, e.getCondition());
SqlNode existHaving = x.asSelect().getHaving();
- if (existHaving != null) {
+ if (existHaving == null) {
+ builder.setHaving(condition);
+ } else {
Review comment:
you could replace this whole `if` block with a call to `SqlUtil.andExpressions`
--
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
[GitHub] [calcite] hannerwang commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
hannerwang commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r675281231
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -504,12 +518,16 @@ protected Builder buildAggregate(Aggregate e, Builder builder,
SqlStdOperatorTable.ROLLUP.createCall(SqlParserPos.ZERO, groupKeys));
default:
case OTHER:
+ List<SqlNode> groupingSetsList = aggregate.getGroupSets().stream()
+ .map(groupSet -> groupItem(groupKeys, groupSet, aggregate.getGroupSet()))
+ .collect(Collectors.toList());
+ if (ImmutableBitSet.union(aggregate.getGroupSets()).cardinality()
+ != aggregate.getGroupSet().cardinality()) {
Review comment:
"!=" to "<"?
--
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
[GitHub] [calcite] hannerwang commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
hannerwang commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r675281772
##########
File path: core/src/test/java/org/apache/calcite/test/RelBuilderTest.java
##########
@@ -1692,6 +1692,40 @@ private RelNode buildRelWithDuplicateAggregates(
assertThat(root, hasTree(expected));
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4665">[CALCITE-4665]
+ * groupKeys contain unused column.</a>. */
+ @Test void testGroupingSetWithGroupKeysContainUnusedColumn() {
Review comment:
Containing may be better
--
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
[GitHub] [calcite] wnob commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
wnob commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r687097548
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -462,6 +484,22 @@ protected Builder buildAggregate(Aggregate e, Builder builder,
// as there is at least one aggregate function.
builder.setGroupBy(new SqlNodeList(groupByList, POS));
}
+
+ if (builder.clauses.contains(Clause.HAVING) && !e.getGroupSet()
+ .equals(ImmutableBitSet.union(e.getGroupSets()))) {
+ // groupSet contains at least one column that is not in any groupSets.
+ // To make such columns must appear in the output (their value will
+ // always be NULL), we generate an extra grouping set, then filter
+ // it out using a "HAVING GROUPING_ID(groupSets) <> 0".
+ // We want to generate the
+ final SqlNodeList groupingList = new SqlNodeList(POS);
+ e.getGroupSet().forEach(g ->
+ groupingList.add(builder.context.field(g)));
+ SqlNumericLiteral zero = SqlNumericLiteral.createExactNumeric(String.valueOf(0), POS);
+ SqlNode condition = SqlStdOperatorTable.NOT_EQUALS.createCall(POS,
+ SqlStdOperatorTable.GROUPING_ID.createCall(groupingList), zero);
Review comment:
nit: This should be `GROUPING` which is now equivalent to `GROUPING_ID` except the former is standardized in ANSI SQL. See [CALCITE-1652](https://issues.apache.org/jira/browse/CALCITE-1652)
--
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
[GitHub] [calcite] julianhyde commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r675922631
##########
File path: core/src/test/java/org/apache/calcite/test/RelBuilderTest.java
##########
@@ -1692,6 +1692,40 @@ private RelNode buildRelWithDuplicateAggregates(
assertThat(root, hasTree(expected));
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4665">[CALCITE-4665]
+ * groupKeys contain unused column.</a>. */
+ @Test void testGroupingSetWithGroupKeysContainUnusedColumn() {
Review comment:
The comment should be the actual JIRA subject. Which I think should be 'Allow Aggregate.groupSet to contain columns not in any of the groupSets'.
--
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
[GitHub] [calcite] zinking commented on pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
zinking commented on pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#issuecomment-884146434
LGTM
--
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
[GitHub] [calcite] vlsi commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r690570603
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -329,7 +330,17 @@ public Result visit(Filter e) {
ImmutableSet.of(Clause.HAVING));
parseCorrelTable(e, x);
final Builder builder = x.builder(e);
- builder.setHaving(builder.context.toSql(null, e.getCondition()));
+ SqlNode condition = builder.context.toSql(null, e.getCondition());
+ SqlNode existHaving = x.asSelect().getHaving();
+ if (existHaving != null) {
+ // if input Aggregate RelNode contains existHaving, need
+ // to create AND expression with connect condition and existHaving
+ // then update input Aggregate's Having condition.
+ condition = SqlUtil.andExpressions(condition, existHaving);
+ x.asSelect().setHaving(condition);
+ } else {
+ builder.setHaving(condition);
+ }
Review comment:
A minor style note: it often works better if a shorter `if` branch is located the first. It makes it easier to relate `else` and the corresponding `if` condition.
```suggestion
if (existHaving == null) {
builder.setHaving(condition);
} else {
// if input Aggregate RelNode contains existHaving, need
// to create AND expression with connect condition and existHaving
// then update input Aggregate's Having condition.
condition = SqlUtil.andExpressions(condition, existHaving);
x.asSelect().setHaving(condition);
}
```
--
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
[GitHub] [calcite] hannerwang commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
hannerwang commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r675285017
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -504,12 +518,16 @@ protected Builder buildAggregate(Aggregate e, Builder builder,
SqlStdOperatorTable.ROLLUP.createCall(SqlParserPos.ZERO, groupKeys));
default:
case OTHER:
+ List<SqlNode> groupingSetsList = aggregate.getGroupSets().stream()
+ .map(groupSet -> groupItem(groupKeys, groupSet, aggregate.getGroupSet()))
+ .collect(Collectors.toList());
+ if (ImmutableBitSet.union(aggregate.getGroupSets()).cardinality()
+ != aggregate.getGroupSet().cardinality()) {
+ groupingSetsList.add(
Review comment:
According to _Aggregate_ constructor doc, the grouping sets must be strictly ordered by inclusion, so the superset grouping set should always be the first.
--
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
[GitHub] [calcite] julianhyde commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r675922400
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -504,12 +518,16 @@ protected Builder buildAggregate(Aggregate e, Builder builder,
SqlStdOperatorTable.ROLLUP.createCall(SqlParserPos.ZERO, groupKeys));
default:
case OTHER:
+ List<SqlNode> groupingSetsList = aggregate.getGroupSets().stream()
+ .map(groupSet -> groupItem(groupKeys, groupSet, aggregate.getGroupSet()))
+ .collect(Collectors.toList());
+ if (ImmutableBitSet.union(aggregate.getGroupSets()).cardinality()
+ != aggregate.getGroupSet().cardinality()) {
Review comment:
Or, even better, `!ImmutableBitSet.union(aggregate.getGroupSets()).equals(aggregate.getGroupSet())`.
--
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
[GitHub] [calcite] julianhyde closed pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
julianhyde closed pull request #2458:
URL: https://github.com/apache/calcite/pull/2458
--
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
[GitHub] [calcite] julianhyde commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r687099866
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -462,6 +484,22 @@ protected Builder buildAggregate(Aggregate e, Builder builder,
// as there is at least one aggregate function.
builder.setGroupBy(new SqlNodeList(groupByList, POS));
}
+
+ if (builder.clauses.contains(Clause.HAVING) && !e.getGroupSet()
+ .equals(ImmutableBitSet.union(e.getGroupSets()))) {
+ // groupSet contains at least one column that is not in any groupSets.
+ // To make such columns must appear in the output (their value will
+ // always be NULL), we generate an extra grouping set, then filter
+ // it out using a "HAVING GROUPING_ID(groupSets) <> 0".
+ // We want to generate the
+ final SqlNodeList groupingList = new SqlNodeList(POS);
+ e.getGroupSet().forEach(g ->
+ groupingList.add(builder.context.field(g)));
+ SqlNumericLiteral zero = SqlNumericLiteral.createExactNumeric(String.valueOf(0), POS);
+ SqlNode condition = SqlStdOperatorTable.NOT_EQUALS.createCall(POS,
+ SqlStdOperatorTable.GROUPING_ID.createCall(groupingList), zero);
Review comment:
yes, `GROUPING` is preferred, even though they are equivalent.
--
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
[GitHub] [calcite] NobiGo commented on pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
NobiGo commented on pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#issuecomment-885226800
@zabetak @amaliujia If you have more time, please help me review this. Thank you.
--
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
[GitHub] [calcite] NobiGo commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
NobiGo commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r690868395
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -329,7 +330,17 @@ public Result visit(Filter e) {
ImmutableSet.of(Clause.HAVING));
parseCorrelTable(e, x);
final Builder builder = x.builder(e);
- builder.setHaving(builder.context.toSql(null, e.getCondition()));
+ SqlNode condition = builder.context.toSql(null, e.getCondition());
+ SqlNode existHaving = x.asSelect().getHaving();
+ if (existHaving != null) {
+ // if input Aggregate RelNode contains existHaving, need
+ // to create AND expression with connect condition and existHaving
+ // then update input Aggregate's Having condition.
+ condition = SqlUtil.andExpressions(condition, existHaving);
+ x.asSelect().setHaving(condition);
+ } else {
+ builder.setHaving(condition);
+ }
Review comment:
done.
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -504,9 +542,18 @@ protected Builder buildAggregate(Aggregate e, Builder builder,
SqlStdOperatorTable.ROLLUP.createCall(SqlParserPos.ZERO, groupKeys));
default:
case OTHER:
+ // Make sure that the group sets contains all bits.
+ final List<ImmutableBitSet> groupSets;
+ if (aggregate.getGroupSet()
+ .equals(ImmutableBitSet.union(aggregate.groupSets))) {
+ groupSets = aggregate.getGroupSets();
+ } else {
+ groupSets = new ArrayList<>(aggregate.getGroupSets());
+ groupSets.add(0, aggregate.getGroupSet());
Review comment:
Done. Thank you for your advice.
--
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
[GitHub] [calcite] NobiGo commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
NobiGo commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r677069953
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -504,12 +518,16 @@ protected Builder buildAggregate(Aggregate e, Builder builder,
SqlStdOperatorTable.ROLLUP.createCall(SqlParserPos.ZERO, groupKeys));
default:
case OTHER:
+ List<SqlNode> groupingSetsList = aggregate.getGroupSets().stream()
+ .map(groupSet -> groupItem(groupKeys, groupSet, aggregate.getGroupSet()))
+ .collect(Collectors.toList());
+ if (ImmutableBitSet.union(aggregate.getGroupSets()).cardinality()
+ != aggregate.getGroupSet().cardinality()) {
+ groupingSetsList.add(
Review comment:
done
##########
File path: core/src/test/java/org/apache/calcite/test/RelBuilderTest.java
##########
@@ -1692,6 +1692,40 @@ private RelNode buildRelWithDuplicateAggregates(
assertThat(root, hasTree(expected));
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4665">[CALCITE-4665]
+ * groupKeys contain unused column.</a>. */
+ @Test void testGroupingSetWithGroupKeysContainUnusedColumn() {
Review comment:
done
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -504,12 +518,16 @@ protected Builder buildAggregate(Aggregate e, Builder builder,
SqlStdOperatorTable.ROLLUP.createCall(SqlParserPos.ZERO, groupKeys));
default:
case OTHER:
+ List<SqlNode> groupingSetsList = aggregate.getGroupSets().stream()
+ .map(groupSet -> groupItem(groupKeys, groupSet, aggregate.getGroupSet()))
+ .collect(Collectors.toList());
+ if (ImmutableBitSet.union(aggregate.getGroupSets()).cardinality()
+ != aggregate.getGroupSet().cardinality()) {
Review comment:
done
--
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
[GitHub] [calcite] julianhyde commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r675923009
##########
File path: core/src/main/java/org/apache/calcite/tools/RelBuilder.java
##########
@@ -934,7 +934,15 @@ public GroupKey groupKey(ImmutableBitSet groupSet) {
*
* <p>This method of creating a group key does not allow you to group on new
* expressions, only column projections, but is efficient, especially when you
- * are coming from an existing {@link Aggregate}. */
+ * are coming from an existing {@link Aggregate}.
+ *
+ * <p>This method of creating a group key allow you to create groupKey be a
Review comment:
Yes. I've modified the javadoc with an example based on yours.
--
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
[GitHub] [calcite] NobiGo commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
NobiGo commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r677070162
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -329,7 +330,20 @@ public Result visit(Filter e) {
ImmutableSet.of(Clause.HAVING));
parseCorrelTable(e, x);
final Builder builder = x.builder(e);
- builder.setHaving(builder.context.toSql(null, e.getCondition()));
+ SqlNode condition = builder.context.toSql(null, e.getCondition());
+ if (ImmutableBitSet.union(aggregate.getGroupSets()).cardinality()
+ != aggregate.getGroupSet().cardinality()) {
Review comment:
use !ImmutableBitSet.union(aggregate.getGroupSets()).equals(aggregate.getGroupSet()) repalce.
--
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
[GitHub] [calcite] zinking commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
zinking commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r673920828
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -329,7 +330,20 @@ public Result visit(Filter e) {
ImmutableSet.of(Clause.HAVING));
parseCorrelTable(e, x);
final Builder builder = x.builder(e);
- builder.setHaving(builder.context.toSql(null, e.getCondition()));
+ SqlNode condition = builder.context.toSql(null, e.getCondition());
+ if (ImmutableBitSet.union(aggregate.getGroupSets()).cardinality()
Review comment:
can this be extracted to a name variable
--
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
[GitHub] [calcite] hannerwang commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
hannerwang commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r675280158
##########
File path: core/src/main/java/org/apache/calcite/tools/RelBuilder.java
##########
@@ -934,7 +934,15 @@ public GroupKey groupKey(ImmutableBitSet groupSet) {
*
* <p>This method of creating a group key does not allow you to group on new
* expressions, only column projections, but is efficient, especially when you
- * are coming from an existing {@link Aggregate}. */
+ * are coming from an existing {@link Aggregate}.
+ *
+ * <p>This method of creating a group key allow you to create groupKey be a
Review comment:
How about "This method of creating a group key allows you to create a group key which is a superset of the union of the groupingSets, for example: group by 0, 1, 2, grouping sets((0, 1), 0). When converting RelNode To Sql, the {@link org.apache.calcite.rel.rel2sql.RelToSqlConverter#visit(Aggregate)} will generate the superset grouping set, then by having clause filtering it out"
--
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
[GitHub] [calcite] NobiGo commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
NobiGo commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r687300129
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -462,6 +484,22 @@ protected Builder buildAggregate(Aggregate e, Builder builder,
// as there is at least one aggregate function.
builder.setGroupBy(new SqlNodeList(groupByList, POS));
}
+
+ if (builder.clauses.contains(Clause.HAVING) && !e.getGroupSet()
+ .equals(ImmutableBitSet.union(e.getGroupSets()))) {
+ // groupSet contains at least one column that is not in any groupSets.
+ // To make such columns must appear in the output (their value will
+ // always be NULL), we generate an extra grouping set, then filter
+ // it out using a "HAVING GROUPING_ID(groupSets) <> 0".
+ // We want to generate the
+ final SqlNodeList groupingList = new SqlNodeList(POS);
+ e.getGroupSet().forEach(g ->
+ groupingList.add(builder.context.field(g)));
+ SqlNumericLiteral zero = SqlNumericLiteral.createExactNumeric(String.valueOf(0), POS);
+ SqlNode condition = SqlStdOperatorTable.NOT_EQUALS.createCall(POS,
+ SqlStdOperatorTable.GROUPING_ID.createCall(groupingList), zero);
Review comment:
done
--
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
[GitHub] [calcite] julianhyde commented on a change in pull request #2458: [CALCITE-4665] When group key contains unused bits RelNode generate wrong SQL(NobiGo)
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2458:
URL: https://github.com/apache/calcite/pull/2458#discussion_r675922927
##########
File path: core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
##########
@@ -504,12 +518,16 @@ protected Builder buildAggregate(Aggregate e, Builder builder,
SqlStdOperatorTable.ROLLUP.createCall(SqlParserPos.ZERO, groupKeys));
default:
case OTHER:
+ List<SqlNode> groupingSetsList = aggregate.getGroupSets().stream()
+ .map(groupSet -> groupItem(groupKeys, groupSet, aggregate.getGroupSet()))
+ .collect(Collectors.toList());
+ if (ImmutableBitSet.union(aggregate.getGroupSets()).cardinality()
+ != aggregate.getGroupSet().cardinality()) {
+ groupingSetsList.add(
Review comment:
Good point. Strictly, that only applies to the `Aggregate.groupSets` data structure (list of `ImmutableBitSet`), not the `SqlNodeList`, but it's a good idea to follow the same pattern. I will make the change.
--
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