[CALCITE-5416] RelToSql converter generates invalid code when merging…#2997
[CALCITE-5416] RelToSql converter generates invalid code when merging…#2997JiajunBernoulli wants to merge 4 commits intoapache:mainfrom
Conversation
… rollup and sort clauses
JiajunBernoulli
force-pushed
the
CALCITE-5416
branch
from
1447fb3 to
e4c468a
Compare
| + "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" |
There was a problem hiding this 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).
There was a problem hiding this comment.
I restored to old solution. Thanks for your review.
… rollup and sort clauses
julianhyde
left a comment
julianhyde
left a comment
There was a problem hiding this comment.
LGTM, with the suggestions I made.
Also, improve the commit message and bug summary. Mention MySQL, JDBC adapter, GROUP BY WITH ROLLUP, ORDER BY.
| 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); |
There was a problem hiding this comment.
for offset and fetch I think if would be clearer than ?
There was a problem hiding this comment.
Yes, if is clearer.
| final Builder builder = | ||
| visitAggregate(aggregate, ImmutableList.copyOf(groupList), | ||
| visitAggregate(aggregate, | ||
| rollupList, |
There was a problem hiding this comment.
put rollupList on previous line
| groupList.add(aggregate.getGroupSet().nth(fc.getFieldIndex())); | ||
| } | ||
| groupList.addAll(Aggregate.Group.getRollup(aggregate.getGroupSets())); | ||
| boolean isImplicitlySort = rollupList.subList(0, sortList.size()).equals(sortList); |
There was a problem hiding this comment.
I didn't have this consciousness before, thank you for letting me learn.
| return result; | ||
| } | ||
| // It does not allow "WITH ROLLUP" in combination with "ORDER BY", | ||
| // so generate the grouped result apply ORDER BY to it. |
…"(GROUP BY x, y WITH ROLLUP) ORDER BY y, x" for MySQL 5
Thanks for your review. |
… ROLLUP and ORDER BY clauses
JDBC adapter should convert
SELECT ...
GROUP BY ROLLUP(x, y)
ORDER BY y, x
to
SELECT ...
FROM (SELECT ...
GROUP BY x, y WITH ROLLUP)
ORDER BY y, x
because MySQL 5 does not allow GROUP BY WITH ROLLUP and
ORDER BY in the same query.
Close apache#2997
3 participants
… rollup and sort clauses