[
https://issues.apache.org/jira/browse/CALCITE-4726?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17397693#comment-17397693
]
Julian Hyde commented on CALCITE-4726:
--------------------------------------
Review comments:
* I got a couple of checkstyle errors; please fix
* Make sure greaterThan is tested in RelBuilderTest
* In Javadoc method comments, use declarative ("Registers") rather than
imperative ("Register"). Use {{<p>}} markers for paragraphs.
* I am uncomfortable with {{AGG_FUNCTIONS_THAT_IGNORE_NULL}}, because people
will forget to maintain it. The fact is, almost all aggregate functions ignore
null. We *almost* have the metadata you need already:
** {{AggregateCall}} has {{boolean ignoreNulls}}. Hopefully it is set correctly
most of the time. This rule should probably assume that it is set correctly.
** What is 'correct'? We have the method
{{SqlAggFunction.allowsNullTreatment}}, but it doesn't help much. The only
aggregate functions that allow treatment are FIRST_VALUE, LAST_VALUE, LEAD,
LAG, NTH_VALUE, all of them windowed aggregate functions that keep nulls by
default.
** I think you should add a method {{boolean
SqlAggFunction.ignoresNullsByDefault()}}. Its default implementation would be
"!allowsNullTreatment()". In other words, functions that allow null treatment
will process nulls unless overridden by a {{IGNORE NULLS}} clause; other
functions will always ignore nulls. Then add a {{Preconditions.checkArgument}}
to {{AggregateCall}}'s constructor to make sure that {{ignoreNulls}} is correct.
* RelOptRulesTest methods have good coverage and good comments. Add a "Test
case for ..." comment to one of them; it will help future maintenance.
* Add at least one query test to {{within-distinct.iq}}, and check that the
results look sane. You can run it via {{CoreQuidemTest}}.
Overall, nice work, [~wnoble]! Most of the above comments are quick fixes, but
adding the {{SqlAggFunction.ignoresNullsByDefault}} method is something we have
wanted for a while, and might not be trivial; if it causes a bunch of errors,
let me know, and we find a simpler solution.
> Add support for filters in AggregateExpandWithinDistinctRule
> ------------------------------------------------------------
>
> Key: CALCITE-4726
> URL: https://issues.apache.org/jira/browse/CALCITE-4726
> Project: Calcite
> Issue Type: Improvement
> Reporter: Will Noble
> Priority: Blocker
>
> Currently, {{AggregateExpandWithinDistinctRule}} does not fire if any of the
> aggregate calls in the aggregate have a filter. We should support filters.
> For example, the following query will not be handled by the current rule due
> to the {{FILTER}} clause:
> {code:java}
> SELECT deptno,
> SUM(sal),
> SUM(sal) WITHIN DISTINCT (job) FILTER (WHERE ename LIKE 'A%')
> FROM emp
> GROUP BY deptno
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
