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

Allow windowed aggregate on top of regular aggregate

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.3.0-incubating
    • Fix Version/s: 1.8.0
    • Component/s: None
    • Labels:
      None

      Description

      Window aggregate on top of a regular aggregate expression is a valid query but currently fails with a 'aggregate expression cannot be nested' error.

      SELECT avg(sum(sal)) over (partition by deptno) from emp group by deptno;
      
      SEVERE: org.apache.calcite.sql.validate.SqlValidatorException: Aggregate expressions cannot be nested
      

      We should allow one level nesting of aggregates under window aggregates, i.e. window_agg(regular_agg(args)) over window.

        Issue Links

          Activity

          Hide
          julianhyde Julian Hyde added a comment -

          The query is valid (i.e. has well-defined semantics) but IIRC the SQL standard disallows aggregates on top of windowed aggregates (or indeed windowed aggregates on top of windowed aggregates).

          Show
          julianhyde Julian Hyde added a comment - The query is valid (i.e. has well-defined semantics) but IIRC the SQL standard disallows aggregates on top of windowed aggregates (or indeed windowed aggregates on top of windowed aggregates).
          Hide
          julianhyde Julian Hyde added a comment -

          The workaround is to use a sub-query:

          SELECT avg(ss) OVER (PARTITION BY deptno)
          FROM (
            SELECT deptno, sum(sal) AS ss
            FROM emp
            GROUP BY deptno)
          
          Show
          julianhyde Julian Hyde added a comment - The workaround is to use a sub-query: SELECT avg(ss) OVER (PARTITION BY deptno) FROM ( SELECT deptno, sum(sal) AS ss FROM emp GROUP BY deptno)
          Hide
          amansinha100 Aman Sinha added a comment -

          Right, aggregates on top of window aggregates are not allowed, but I think window aggregates on top of grouped aggregates are allowed. The SQL 2011 spec has the following:
          "An <aggregate function> simply contained in a <window function> shall not simply contain a <hypothetical set function>."
          In another section: "Hypothetical set functions are related to the window functions RANK, DENSE_RANK, PERCENT_RANK, and CUME_DIST". This suggests that as long as the aggregate function is not a 'hypothetical set function', it is allowed.

          TPC-DS has several queries that have window aggregates on top of aggregate expressions (e.g query20, query47).

          The workaround of doing the grouped aggregation in the subquery is what we are doing currently in Drill but would be good to support the original syntax.

          Show
          amansinha100 Aman Sinha added a comment - Right, aggregates on top of window aggregates are not allowed, but I think window aggregates on top of grouped aggregates are allowed. The SQL 2011 spec has the following: "An <aggregate function> simply contained in a <window function> shall not simply contain a <hypothetical set function>." In another section: "Hypothetical set functions are related to the window functions RANK, DENSE_RANK, PERCENT_RANK, and CUME_DIST". This suggests that as long as the aggregate function is not a 'hypothetical set function', it is allowed. TPC-DS has several queries that have window aggregates on top of aggregate expressions (e.g query20, query47). The workaround of doing the grouped aggregation in the subquery is what we are doing currently in Drill but would be good to support the original syntax.
          Hide
          julianhyde Julian Hyde added a comment -

          I stand corrected. If it's standard SQL let's do it.

          Show
          julianhyde Julian Hyde added a comment - I stand corrected. If it's standard SQL let's do it.
          Hide
          amansinha100 Aman Sinha added a comment -

          Ok, I will assign this to myself and will work on it after wrapping up another task.

          Show
          amansinha100 Aman Sinha added a comment - Ok, I will assign this to myself and will work on it after wrapping up another task.
          Hide
          gparai Gautam Kumar Parai added a comment -

          I talked to Aman Sinha offline and he asked me to work on this bug. Can you please add my userid (gparai) to Calcite so that I can work on it?

          Thanks,
          Gautam

          Show
          gparai Gautam Kumar Parai added a comment - I talked to Aman Sinha offline and he asked me to work on this bug. Can you please add my userid (gparai) to Calcite so that I can work on it? Thanks, Gautam
          Hide
          julianhyde Julian Hyde added a comment -

          Done!

          Show
          julianhyde Julian Hyde added a comment - Done!
          Hide
          gparai Gautam Kumar Parai added a comment -

          I have created a pull request for this enhancement: https://github.com/apache/calcite/pull/238
          Please take a look and provide feedback. Thanks!

          Show
          gparai Gautam Kumar Parai added a comment - I have created a pull request for this enhancement: https://github.com/apache/calcite/pull/238 Please take a look and provide feedback. Thanks!
          Hide
          julianhyde Julian Hyde added a comment -

          Well, I can't deny that it works. But it's not ready to commit.

          • Using mutable state in SqlBasicCall is messy and will cause re-entrancy problems. Could you put that state in the visitor?
          • '@return Void' is the most useless javadoc comment I've ever seen.
          • You can remove most of the "CALCITE-750 tags. We don't track individual features in the source code. Just make comments where you need to clarify.
          • It's pretty sloppy, frankly, that you have "[Calcite -750]" in one place and and "[Caclite-750]" in another.
          • In the test case, follow the convention in other test cases, and use a hyperlink to the JIRA case.
          • Don't remove the code from SqlValidatorTest, convert it from a negative to positive test case.
          Show
          julianhyde Julian Hyde added a comment - Well, I can't deny that it works. But it's not ready to commit. Using mutable state in SqlBasicCall is messy and will cause re-entrancy problems. Could you put that state in the visitor? '@return Void' is the most useless javadoc comment I've ever seen. You can remove most of the " CALCITE-750 tags. We don't track individual features in the source code. Just make comments where you need to clarify. It's pretty sloppy, frankly, that you have " [Calcite -750] " in one place and and " [Caclite-750] " in another. In the test case, follow the convention in other test cases, and use a hyperlink to the JIRA case. Don't remove the code from SqlValidatorTest, convert it from a negative to positive test case.
          Hide
          gparai Gautam Kumar Parai added a comment -

          Thanks so much for the review Julian! I have updated the pull request based on your feedback. Please take a look. Thanks!

          Show
          gparai Gautam Kumar Parai added a comment - Thanks so much for the review Julian! I have updated the pull request based on your feedback. Please take a look. Thanks!
          Hide
          julianhyde Julian Hyde added a comment -

          Gautam Kumar Parai, Much improved - thanks. I have done some cleanup at https://github.com/julianhyde/calcite/tree/750-win-agg-on-regular-agg, removing mutable state that made it difficult to understand what was going on. I'm working on further cleanup before I commit.

          Show
          julianhyde Julian Hyde added a comment - Gautam Kumar Parai , Much improved - thanks. I have done some cleanup at https://github.com/julianhyde/calcite/tree/750-win-agg-on-regular-agg , removing mutable state that made it difficult to understand what was going on. I'm working on further cleanup before I commit.
          Hide
          julianhyde Julian Hyde added a comment -

          Gautam Kumar Parai, OK, I've finished fix up. Please review my dev branch. If OK I'll commit. (There are some cosmetic changes mixed in there, which I'll separate out when I squash.)

          Show
          julianhyde Julian Hyde added a comment - Gautam Kumar Parai , OK, I've finished fix up. Please review my dev branch. If OK I'll commit. (There are some cosmetic changes mixed in there, which I'll separate out when I squash.)
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/918e612b , with fix up in http://git-wip-us.apache.org/repos/asf/calcite/commit/e25ceef6 . Gautam Kumar Parai , thanks for the PR!
          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.8.0 (2016-06-13).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.8.0 (2016-06-13).

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              amansinha100 Aman Sinha
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development