xuzifu666 commented on code in PR #4983:
URL: https://github.com/apache/calcite/pull/4983#discussion_r3353011397
##########
core/src/main/java/org/apache/calcite/rel/rules/AggregateExpandDistinctAggregatesRule.java:
##########
@@ -315,11 +325,15 @@ private static RelBuilder
convertSingletonDistinct(RelBuilder relBuilder,
final ImmutableBitSet originalGroupSet = aggregate.getGroupSet();
// Add the distinct aggregate column(s) to the group-by columns,
- // if not already a part of the group-by
+ // if not already a part of the group-by. Also add any ORDER BY columns.
final NavigableSet<Integer> bottomGroups = new
TreeSet<>(aggregate.getGroupSet().asList());
for (AggregateCall aggCall : originalAggCalls) {
if (aggCall.isDistinct()) {
bottomGroups.addAll(aggCall.getArgList());
+ // Also include ORDER BY columns from WITHIN GROUP
+ for (RelFieldCollation fc : aggCall.collation.getFieldCollations()) {
Review Comment:
> Wouldn't this cause DISTINCT to be ignored in certain scenarios?
>
> Consider the following example:
>
> ```
> SELECT deptno, SUM(DISTINCT sal) WITHIN GROUP (ORDER BY bonus)
> FROM EMP
> GROUP BY deptno
> ```
>
> With this modification, the rule would rewrite the query as:
>
> ```
> SELECT deptno, SUM(sal) WITHIN GROUP (ORDER BY bonus)
> FROM (
> SELECT deptno, sal, bonus FROM EMP GROUP BY deptno, sal, bonus
> )
> GROUP BY deptno
> ```
>
> This does not correctly enforce DISTINCT on sal: if two rows share the
same sal value but have different bonus values, both survive the inner GROUP BY
and sal ends up counted twice in the outer SUM, which violates the DISTINCT
semantics.
>
> PS: Please feel free to correct me or let me know if I am intervening
inappropriately.
I think the question you raised is quite valuable, and I've already fixed it
in a recent commit. You can take a look. @GoncaloCoutoDosSantos
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
