julianhyde commented on code in PR #2997:
URL: https://github.com/apache/calcite/pull/2997#discussion_r1051695803
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -868,19 +867,35 @@ public Result visit(Sort e) {
if (hasTrickyRollup(e, aggregate)) {
// MySQL 5 does not support standard "GROUP BY ROLLUP(x, y)", only
// the non-standard "GROUP BY x, y WITH ROLLUP".
- // It does not allow "WITH ROLLUP" in combination with "ORDER BY",
- // but "GROUP BY x, y WITH ROLLUP" implicitly sorts by x, y,
+ List<Integer> rollupList =
Aggregate.Group.getRollup(aggregate.getGroupSets());
+ List<Integer> sortList = e.getCollation()
+ .getFieldCollations()
+ .stream()
+ .map(f -> aggregate.getGroupSet().nth(f.getFieldIndex()))
+ .collect(Collectors.toList());
+ // "GROUP BY x, y WITH ROLLUP" implicitly sorts by x, y,
// so skip the ORDER BY.
- final Set<Integer> groupList = new LinkedHashSet<>();
- for (RelFieldCollation fc : e.collation.getFieldCollations()) {
- groupList.add(aggregate.getGroupSet().nth(fc.getFieldIndex()));
- }
- groupList.addAll(Aggregate.Group.getRollup(aggregate.getGroupSets()));
+ boolean isImplicitlySort = rollupList.subList(0,
sortList.size()).equals(sortList);
final Builder builder =
- visitAggregate(aggregate, ImmutableList.copyOf(groupList),
+ visitAggregate(aggregate,
+ rollupList,
Clause.GROUP_BY, Clause.OFFSET, Clause.FETCH);
- offsetFetch(e, builder);
- return builder.result();
+ Result result = builder.result();
+ if (sortList.isEmpty()
+ || isImplicitlySort) {
+ offsetFetch(e, builder);
+ return result;
+ }
+ // It does not allow "WITH ROLLUP" in combination with "ORDER BY",
+ // so generate the grouped result apply ORDER BY to it.
+ SqlSelect sqlSelect = result.subSelect();
+ SqlNodeList sortExps = exprList(builder.context, e.getSortExps());
+ sqlSelect.setOrderBy(sortExps);
+ SqlNode offset = e.offset == null ? null : builder.context.toSql(null,
e.offset);
Review Comment:
for offset and fetch I think `if` would be clearer than `?`
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -868,19 +867,35 @@ public Result visit(Sort e) {
if (hasTrickyRollup(e, aggregate)) {
// MySQL 5 does not support standard "GROUP BY ROLLUP(x, y)", only
// the non-standard "GROUP BY x, y WITH ROLLUP".
- // It does not allow "WITH ROLLUP" in combination with "ORDER BY",
- // but "GROUP BY x, y WITH ROLLUP" implicitly sorts by x, y,
+ List<Integer> rollupList =
Aggregate.Group.getRollup(aggregate.getGroupSets());
+ List<Integer> sortList = e.getCollation()
+ .getFieldCollations()
+ .stream()
+ .map(f -> aggregate.getGroupSet().nth(f.getFieldIndex()))
+ .collect(Collectors.toList());
+ // "GROUP BY x, y WITH ROLLUP" implicitly sorts by x, y,
// so skip the ORDER BY.
- final Set<Integer> groupList = new LinkedHashSet<>();
- for (RelFieldCollation fc : e.collation.getFieldCollations()) {
- groupList.add(aggregate.getGroupSet().nth(fc.getFieldIndex()));
- }
- groupList.addAll(Aggregate.Group.getRollup(aggregate.getGroupSets()));
+ boolean isImplicitlySort = rollupList.subList(0,
sortList.size()).equals(sortList);
Review Comment:
should be `final`
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -868,19 +867,35 @@ public Result visit(Sort e) {
if (hasTrickyRollup(e, aggregate)) {
// MySQL 5 does not support standard "GROUP BY ROLLUP(x, y)", only
// the non-standard "GROUP BY x, y WITH ROLLUP".
- // It does not allow "WITH ROLLUP" in combination with "ORDER BY",
- // but "GROUP BY x, y WITH ROLLUP" implicitly sorts by x, y,
+ List<Integer> rollupList =
Aggregate.Group.getRollup(aggregate.getGroupSets());
+ List<Integer> sortList = e.getCollation()
+ .getFieldCollations()
+ .stream()
+ .map(f -> aggregate.getGroupSet().nth(f.getFieldIndex()))
+ .collect(Collectors.toList());
+ // "GROUP BY x, y WITH ROLLUP" implicitly sorts by x, y,
// so skip the ORDER BY.
- final Set<Integer> groupList = new LinkedHashSet<>();
- for (RelFieldCollation fc : e.collation.getFieldCollations()) {
- groupList.add(aggregate.getGroupSet().nth(fc.getFieldIndex()));
- }
- groupList.addAll(Aggregate.Group.getRollup(aggregate.getGroupSets()));
+ boolean isImplicitlySort = rollupList.subList(0,
sortList.size()).equals(sortList);
final Builder builder =
- visitAggregate(aggregate, ImmutableList.copyOf(groupList),
+ visitAggregate(aggregate,
+ rollupList,
Review Comment:
put `rollupList` on previous line
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -868,19 +867,35 @@ public Result visit(Sort e) {
if (hasTrickyRollup(e, aggregate)) {
// MySQL 5 does not support standard "GROUP BY ROLLUP(x, y)", only
// the non-standard "GROUP BY x, y WITH ROLLUP".
- // It does not allow "WITH ROLLUP" in combination with "ORDER BY",
- // but "GROUP BY x, y WITH ROLLUP" implicitly sorts by x, y,
+ List<Integer> rollupList =
Aggregate.Group.getRollup(aggregate.getGroupSets());
+ List<Integer> sortList = e.getCollation()
+ .getFieldCollations()
+ .stream()
+ .map(f -> aggregate.getGroupSet().nth(f.getFieldIndex()))
+ .collect(Collectors.toList());
+ // "GROUP BY x, y WITH ROLLUP" implicitly sorts by x, y,
// so skip the ORDER BY.
- final Set<Integer> groupList = new LinkedHashSet<>();
- for (RelFieldCollation fc : e.collation.getFieldCollations()) {
- groupList.add(aggregate.getGroupSet().nth(fc.getFieldIndex()));
- }
- groupList.addAll(Aggregate.Group.getRollup(aggregate.getGroupSets()));
+ boolean isImplicitlySort = rollupList.subList(0,
sortList.size()).equals(sortList);
final Builder builder =
- visitAggregate(aggregate, ImmutableList.copyOf(groupList),
+ visitAggregate(aggregate,
+ rollupList,
Clause.GROUP_BY, Clause.OFFSET, Clause.FETCH);
- offsetFetch(e, builder);
- return builder.result();
+ Result result = builder.result();
+ if (sortList.isEmpty()
+ || isImplicitlySort) {
+ offsetFetch(e, builder);
+ return result;
+ }
+ // It does not allow "WITH ROLLUP" in combination with "ORDER BY",
+ // so generate the grouped result apply ORDER BY to it.
Review Comment:
Change "It" to "MySQL"
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
