[CALCITE-6214] Remove DISTINCT in aggregate function if field is unique

[JiajunBernoulli]

Here is JIRA ticket: https://issues.apache.org/jira/browse/CALCITE-6214

[mihaibudiu]

@julianhyde asked in JIRA to handle other aggregates too. Why just count?

[JiajunBernoulli]

I overlooked it before.

Now we can handle other aggregates.

[mihaibudiu]

If you handle other aggregates you should probably add tests like SELECT COUNT(x), SUM(x) FROM SELECT DISTINCT ...

[JiajunBernoulli]

Yes, now we can handle other aggregate functions after new commits.

[julianhyde]

Don't use backticks in jira summary or javadoc. They will be rendered as backticks.

[JiajunBernoulli]

Removed them.

[julianhyde]

Can you add at least one test where the outer query has a GROUP BY? The following query should benefit from the simplification:

SELECT deptno, COUNT(DISTINCT sal), 
FROM (
   SELECT DISTINCT deptno, sal
   FROM emp)
GROUP BY deptno

Note that sal is not distinct but it is distinct for each deptno (because (deptno, sal) is a key).

[JiajunBernoulli]

Added it.

[mihaibudiu]

Another question I have is whether this should be as you wrote it in RelBuilder, or it should be a rewrite rule in the optimizer. It is a matter of design choice rather than of correctness.

[mihaibudiu]

I would document that this is specifically for aggregates.
And perhaps a better function name would be removeRedundantAggregateDistinct.

[JiajunBernoulli]

Renamed to removeRedundantAggregateDistinct.

[mihaibudiu]

This flag is a little unintuitive, since it inhibits the optimization rather than enabling it.
All the other similar flags are in the opposite way.

[JiajunBernoulli]

Good catch.

I changed default value, now we can set true to optimize. (Default is false)

[mihaibudiu]

Where is the corresponding test?

[JiajunBernoulli]

Forgot to delete.

Removed it.

[mihaibudiu]

Why is the DISTINCT removed from this case?

[JiajunBernoulli]

We can know empno is primary key by using RelMetadataQuery#areColumnsUnique.

Here is metadata:

calcite/testkit/src/main/java/org/apache/calcite/test/catalog/MockCatalogReaderSimple.java

Line 80 in f837ffa

empTable.addColumn("EMPNO", fixture.intType, true);

[mihaibudiu]

There are so many empno references in the codebase that I couldn't figure out that this is a primary key.
There are also multiple definitions of this column in multiple files, and I didn't know which one is being used here.
Can you please add a comment explaining this in the code?

[JiajunBernoulli]

Added

[JiajunBernoulli]

Another question I have is whether this should be as you wrote it in RelBuilder, or it should be a rewrite rule in the optimizer. It is a matter of design choice rather than of correctness.

Thank you for your review.

1. RelBuilder optimization can reuse subexpressions.

• Using RelBuilder

LogicalProject(DEPTNO=[$0], CDS=[$1], CS=[$2], SDS=[$3], SS=[$3]) -- SDS is same as SS
  LogicalAggregate(group=[{0}], CDS=[COUNT($1)], CS=[COUNT()], SDS=[SUM($1)])
    LogicalAggregate(group=[{0, 1}])
      LogicalProject(DEPTNO=[$7], SAL=[$5])
        LogicalTableScan(table=[[CATALOG, SALES, EMP]])

• Using Rule

LogicalProject(DEPTNO=[$0], CDS=[$1], CS=[$2], SDS=[$3], SS=[$4]) -- SDS is same as SS
  LogicalAggregate(group=[{0}], CDS=[COUNT($1)], CS=[COUNT()], SDS=[SUM($1)], SS=[SUM($1)])
    LogicalAggregate(group=[{0, 1}])
      LogicalProject(DEPTNO=[$7], SAL=[$5])
        LogicalTableScan(table=[[CATALOG, SALES, EMP]])

We need other rules to remove same function.

2. RelBuilder is easier to use than Rule.

• RelBuilder: withRedundantDistinct(flag) to enable or disable.
• Rule: Add or remove rule in programs.

[sonarcloud]

Quality Gate passed

Issues
3 New issues

Measures
0 Security Hotspots
96.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud