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 2022/12/04 06:58:39 UTC
[GitHub] [calcite] JiajunBernoulli opened a new pull request, #2997: [CALCITE-5416] RelToSql converter generates invalid code when merging…
JiajunBernoulli opened a new pull request, #2997:
URL: https://github.com/apache/calcite/pull/2997
… rollup and sort clauses
--
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] asfgit closed pull request #2997: [CALCITE-5416] RelToSql converter generates invalid code when merging…
Posted by "asfgit (via GitHub)" <gi...@apache.org>.
asfgit closed pull request #2997: [CALCITE-5416] RelToSql converter generates invalid code when merging…
URL: https://github.com/apache/calcite/pull/2997
--
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] JiajunBernoulli commented on pull request #2997: [CALCITE-5416] RelToSql converter generates invalid code when merging…
Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on PR #2997:
URL: https://github.com/apache/calcite/pull/2997#issuecomment-1357496002
> LGTM, with the suggestions I made.
>
> Also, improve the commit message and bug summary. Mention MySQL, JDBC adapter, GROUP BY WITH ROLLUP, ORDER BY.
Thanks for your review.
> LGTM, with the suggestions I made.
>
> Also, improve the commit message and bug summary. Mention MySQL, JDBC adapter, GROUP BY WITH ROLLUP, ORDER BY.
Thanks for your 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] julianhyde commented on a diff in pull request #2997: [CALCITE-5416] RelToSql converter generates invalid code when merging…
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] JiajunBernoulli commented on a diff in pull request #2997: [CALCITE-5416] RelToSql converter generates invalid code when merging…
Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on code in PR #2997:
URL: https://github.com/apache/calcite/pull/2997#discussion_r1052095333
##########
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:
Yes, `if` is clearer.
##########
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:
Ok
##########
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:
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] LeonidChistov commented on a diff in pull request #2997: [CALCITE-5416] RelToSql converter generates invalid code when merging…
Posted by GitBox <gi...@apache.org>.
LeonidChistov commented on code in PR #2997:
URL: https://github.com/apache/calcite/pull/2997#discussion_r1039293894
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -672,9 +672,11 @@ private static String toSql(RelNode root, SqlDialect dialect,
+ "FROM \"foodmart\".\"product\"\n"
+ "GROUP BY ROLLUP(\"product_class_id\", \"brand_name\")\n"
+ "ORDER BY \"product_class_id\", \"brand_name\"";
- final String expectedMysql = "SELECT `product_class_id`, `brand_name`\n"
+ final String expectedMysql = "SELECT *\n"
Review Comment:
That is probably excessive - if order of grouping fields matches the order of sort fields, we can omit ORDER BY clause (as it was done in the old solution).
--
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] JiajunBernoulli commented on a diff in pull request #2997: [CALCITE-5416] RelToSql converter generates invalid code when merging…
Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on code in PR #2997:
URL: https://github.com/apache/calcite/pull/2997#discussion_r1039640366
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -672,9 +672,11 @@ private static String toSql(RelNode root, SqlDialect dialect,
+ "FROM \"foodmart\".\"product\"\n"
+ "GROUP BY ROLLUP(\"product_class_id\", \"brand_name\")\n"
+ "ORDER BY \"product_class_id\", \"brand_name\"";
- final String expectedMysql = "SELECT `product_class_id`, `brand_name`\n"
+ final String expectedMysql = "SELECT *\n"
Review Comment:
I restored to old solution. Thanks for your 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] JiajunBernoulli commented on a diff in pull request #2997: [CALCITE-5416] RelToSql converter generates invalid code when merging…
Posted by GitBox <gi...@apache.org>.
JiajunBernoulli commented on code in PR #2997:
URL: https://github.com/apache/calcite/pull/2997#discussion_r1052093557
##########
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:
I didn't have this consciousness before, thank you for letting me learn.
--
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