kgyrtkirk commented on code in PR #3735:
URL: https://github.com/apache/calcite/pull/3735#discussion_r1528352739
##########
core/src/main/java/org/apache/calcite/rel/rules/AggregateExpandDistinctAggregatesRule.java:
##########
@@ -741,14 +741,10 @@ private static void doRewrite(RelBuilder relBuilder,
Aggregate aggregate, int n,
for (Integer arg : aggCall.getArgList()) {
newArgs.add(requireNonNull(sourceOf.get(arg), () -> "sourceOf.get(" +
arg + ")"));
}
- final int newFilterArg =
- aggCall.filterArg < 0 ? -1
- : requireNonNull(sourceOf.get(aggCall.filterArg),
- () -> "sourceOf.get(" + aggCall.filterArg + ")");
final AggregateCall newAggCall =
AggregateCall.create(aggCall.getAggregation(), false,
aggCall.isApproximate(), aggCall.ignoreNulls(), aggCall.rexList,
- newArgs, newFilterArg, aggCall.distinctKeys, aggCall.collation,
+ newArgs, -1, aggCall.distinctKeys, aggCall.collation,
aggCall.getType(), aggCall.getName());
assert refs.get(i) == null;
Review Comment:
adding this test exposes an additional issue with the current logic:
```java
@Test void testDistinctCountFilter3() {
final String sql = "select deptno, " +
"count(distinct sal) FILTER (WHERE sal > 1000), " +
"count(distinct sal) FILTER (WHERE sal > 500) " +
"from sales.emp group by deptno";
sql(sql)
.withRule(CoreRules.AGGREGATE_EXPAND_DISTINCT_AGGREGATES_TO_JOIN,
CoreRules.AGGREGATE_PROJECT_MERGE)
.check();
testSortUnionTranspose();
}
```
the current contract of `doRewrite` seems to be that it will:
* generate a join which will be filtered by `filterArg`
* computes all aggregates for those `args+filterarg`
however the loop which considers the aggregates to be included there does
not restrict the `aggCall`-s by their
[`filterArg`](https://github.com/apache/calcite/blob/9c0d690439920fff5120250e5f1899f857a5b1ec/core/src/main/java/org/apache/calcite/rel/rules/AggregateExpandDistinctAggregatesRule.java#L727-L736)
I think that should be added -> in which case the supression of the
`filterArg` becomes a straight forward consequence of that
other way around could be to support different filterargs-s for aggs with
the same arguments:
the current logic seem to be designed toward removing the `filter` from the
`aggCall` - and provide some other way to do the extraction of the filter into
a case separately
--
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]
