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

Validate that window functions have OVER clause

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.5.0
    • Component/s: core
    • Labels:
      None

      Description

      For instance,

      select rank() 
      from emp
      

      should have given exception.

        Activity

        Show
        seanhychu Sean Hsuan-Yi Chu added a comment - https://github.com/apache/incubator-calcite/pull/113
        Hide
        seanhychu Sean Hsuan-Yi Chu added a comment -

        Julian Hyde Can you help review? Thanks.

        Show
        seanhychu Sean Hsuan-Yi Chu added a comment - Julian Hyde Can you help review? Thanks.
        Hide
        julianhyde Julian Hyde added a comment -

        Looks good. A few minor things:

        1. Rename "isRequireOver" method and "requireOver" field to "requiresOver" to be consistent with "requiresOrder".

        2. I like the approach of having these properties be boolean fields. Please move the SqlRankFunction.requiresOrder field up to SqlAggFunction and add a constructor parameter. You should be able to remove requiresOrder() methods in sub-classes.

        3. Mark as "Deprecated // to be removed before 2.0" all but one constructor of SqlAggFunction. Fix existing code that uses deprecated constructors.

        Show
        julianhyde Julian Hyde added a comment - Looks good. A few minor things: 1. Rename "isRequireOver" method and "requireOver" field to "requiresOver" to be consistent with "requiresOrder". 2. I like the approach of having these properties be boolean fields. Please move the SqlRankFunction.requiresOrder field up to SqlAggFunction and add a constructor parameter. You should be able to remove requiresOrder() methods in sub-classes. 3. Mark as "Deprecated // to be removed before 2.0" all but one constructor of SqlAggFunction. Fix existing code that uses deprecated constructors.
        Hide
        seanhychu Sean Hsuan-Yi Chu added a comment -

        A new patch addressed the review comments above.

        Thanks for reviewing in advance!

        Show
        seanhychu Sean Hsuan-Yi Chu added a comment - A new patch addressed the review comments above. Thanks for reviewing in advance!
        Hide
        julianhyde Julian Hyde added a comment -

        Looks good. I've committed to my branch new-master https://github.com/julianhyde/incubator-calcite/commits/new-master. I will commit to apache master, and close this case and the pull request, when commits open following 1.4. Thanks for the patch!

        Show
        julianhyde Julian Hyde added a comment - Looks good. I've committed to my branch new-master https://github.com/julianhyde/incubator-calcite/commits/new-master . I will commit to apache master, and close this case and the pull request, when commits open following 1.4. Thanks for the patch!
        Show
        julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/77674a7c . Thanks for the patch, Sean Hsuan-Yi Chu !
        Hide
        jcamachorodriguez Jesus Camacho Rodriguez added a comment -

        Resolved in release 1.5.0 (2015-11-10)

        Show
        jcamachorodriguez Jesus Camacho Rodriguez added a comment - Resolved in release 1.5.0 (2015-11-10)

          People

          • Assignee:
            julianhyde Julian Hyde
            Reporter:
            seanhychu Sean Hsuan-Yi Chu
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development