Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-1288

Avoid doing the same join twice if count(distinct) exists

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.9.0
    • Component/s: None
    • Labels:
      None

      Description

      When the query has one distinct aggregate and one or more non-distinct aggregates, the join instance need not produce the join-based plan. We can generate multi-phase aggregates. Another approach would be to use grouping sets. However, this transformation will be useful when systems don't support grouping sets and instead rely on the join-based plans (see the plan below)

      select emp.empno, count(*), avg(distinct dept.deptno) 
      from sales.emp emp inner join sales.dept dept 
      on emp.deptno = dept.deptno 
      group by emp.empno
      
      LogicalProject(EMPNO=[$0], EXPR$1=[$1], EXPR$2=[$3])
        LogicalJoin(condition=[IS NOT DISTINCT FROM($0, $2)], joinType=[inner])
          LogicalAggregate(group=[{0}], EXPR$1=[COUNT()])
            LogicalProject(EMPNO=[$0], DEPTNO0=[$9])
              LogicalJoin(condition=[=($7, $9)], joinType=[inner])
                LogicalTableScan(table=[[CATALOG, SALES, EMP]])
                LogicalTableScan(table=[[CATALOG, SALES, DEPT]])
          LogicalAggregate(group=[{0}], EXPR$2=[AVG($1)])
            LogicalAggregate(group=[{0, 1}])
              LogicalProject(EMPNO=[$0], DEPTNO0=[$9])
                LogicalJoin(condition=[=($7, $9)], joinType=[inner])
                  LogicalTableScan(table=[[CATALOG, SALES, EMP]])
                  LogicalTableScan(table=[[CATALOG, SALES, DEPT]])
      

      The more efficient form should look like

      select emp.empno, count(*), avg(distinct dept.deptno) 
      from sales.emp emp inner join sales.dept dept 
      on emp.deptno = dept.deptno 
      group by emp.empno
      
      LogicalAggregate(group=[{0}], EXPR$1=[SUM($2)], EXPR$2=[AVG($1)])
        LogicalAggregate(group=[{0, 1}], EXPR$1=[COUNT()])
          LogicalProject(EMPNO=[$0], DEPTNO0=[$9])
            LogicalJoin(condition=[=($7, $9)], joinType=[inner])
              LogicalTableScan(table=[[CATALOG, SALES, EMP]])
              LogicalTableScan(table=[[CATALOG, SALES, DEPT]])
      

        Issue Links

          Activity

          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Resolved in release 1.9.0 (2016-09-22)

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Resolved in release 1.9.0 (2016-09-22)
          Hide
          gparai Gautam Kumar Parai added a comment -

          Thanks Julian HydeJesus Camacho Rodriguez for figuring it out

          Show
          gparai Gautam Kumar Parai added a comment - Thanks Julian Hyde Jesus Camacho Rodriguez for figuring it out
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/c7cbfdf9 . Gautam Kumar Parai , thanks for the PR!
          Hide
          julianhyde Julian Hyde added a comment -

          Thanks - I thought so! I'll fix the commit author accordingly.

          Show
          julianhyde Julian Hyde added a comment - Thanks - I thought so! I'll fix the commit author accordingly.
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          AFAIK, Gautam Kumar Parai did not do most of the work... He did ALL the work Must be some kind of mistake...

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - AFAIK, Gautam Kumar Parai did not do most of the work... He did ALL the work Must be some kind of mistake...
          Hide
          julianhyde Julian Hyde added a comment -

          Gautam Kumar Parai, Git says that Jesus Camacho Rodriguez is the author of https://github.com/apache/calcite/pull/247/commits/81081a576cd129c7481df4fff263deb3a141bece. Is that right? I thought you did most of the work. I want to get the attribution right.

          Show
          julianhyde Julian Hyde added a comment - Gautam Kumar Parai , Git says that Jesus Camacho Rodriguez is the author of https://github.com/apache/calcite/pull/247/commits/81081a576cd129c7481df4fff263deb3a141bece . Is that right? I thought you did most of the work. I want to get the attribution right.
          Hide
          gparai Gautam Kumar Parai added a comment -

          Julian Hyde I have updated the pull request based on your comments. Can you please take a look? Thanks!

          Show
          gparai Gautam Kumar Parai added a comment - Julian Hyde I have updated the pull request based on your comments. Can you please take a look? Thanks!
          Hide
          gparai Gautam Kumar Parai added a comment -

          Julian Hyde Yes, I will start working on it. I was caught up with other issues so did not get a chance earlier.

          Show
          gparai Gautam Kumar Parai added a comment - Julian Hyde Yes, I will start working on it. I was caught up with other issues so did not get a chance earlier.
          Hide
          julianhyde Julian Hyde added a comment -

          Gautam Kumar Parai, Are you planning to fix review comments in the PR? Please also squash and rebase.

          Show
          julianhyde Julian Hyde added a comment - Gautam Kumar Parai , Are you planning to fix review comments in the PR? Please also squash and rebase.
          Hide
          julianhyde Julian Hyde added a comment -

          FYI, while writing tests for this issue in https://github.com/julianhyde/calcite/commits/1288-distinct-count, I discovered CALCITE-1293. (The issue existed before this PR.)

          Show
          julianhyde Julian Hyde added a comment - FYI, while writing tests for this issue in https://github.com/julianhyde/calcite/commits/1288-distinct-count , I discovered CALCITE-1293 . (The issue existed before this PR.)
          Hide
          gparai Gautam Kumar Parai added a comment -

          I was working on Drill's forked version and that the transformation might be useful in Master was an afterthought (when I was about to submit the pull request). I will remember to do so earlier from now on Julian Hyde.

          Show
          gparai Gautam Kumar Parai added a comment - I was working on Drill's forked version and that the transformation might be useful in Master was an afterthought (when I was about to submit the pull request). I will remember to do so earlier from now on Julian Hyde .
          Hide
          julianhyde Julian Hyde added a comment -

          It would help a lot if you logged a JIRA case before starting work, rather than when you have a PR. Then we can have a design discussion.

          Show
          julianhyde Julian Hyde added a comment - It would help a lot if you logged a JIRA case before starting work, rather than when you have a PR. Then we can have a design discussion.
          Hide
          gparai Gautam Kumar Parai added a comment -

          Aman Sinha thank you for the clarification. I should have explicitly mentioned it in the JIRA. Updated it.

          Show
          gparai Gautam Kumar Parai added a comment - Aman Sinha thank you for the clarification. I should have explicitly mentioned it in the JIRA. Updated it.
          Hide
          amansinha100 Aman Sinha added a comment -

          Julian Hyde, for systems that don't support Grouping Sets (which is the enhancement you implemented in CALCITE-732), it would be useful to have this rewrite. Drill currently does not have GS but I would imagine some other systems may also benefit, even though this rewrite is specific to a single agg(distinct) combined with other non-distinct aggregates. What do you think ?

          Show
          amansinha100 Aman Sinha added a comment - Julian Hyde , for systems that don't support Grouping Sets (which is the enhancement you implemented in CALCITE-732 ), it would be useful to have this rewrite. Drill currently does not have GS but I would imagine some other systems may also benefit, even though this rewrite is specific to a single agg(distinct) combined with other non-distinct aggregates. What do you think ?
          Hide
          julianhyde Julian Hyde added a comment -

          Did you consider leveraging CALCITE-732?

          Show
          julianhyde Julian Hyde added a comment - Did you consider leveraging CALCITE-732 ?
          Hide
          gparai Gautam Kumar Parai added a comment -

          I have created the pull request at: https://github.com/apache/calcite/pull/247

          Aman Sinha Julian Hyde Could you please take a look and provide feedback? Thanks!!

          Show
          gparai Gautam Kumar Parai added a comment - I have created the pull request at: https://github.com/apache/calcite/pull/247 Aman Sinha Julian Hyde Could you please take a look and provide feedback? Thanks!!

            People

            • Assignee:
              gparai Gautam Kumar Parai
              Reporter:
              gparai Gautam Kumar Parai
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development