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

Support multiple columns in PARTITION BY clause of window function

    Details

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

      Description

      When I add multiple partition columns to a window function:

      
        @Test public void testSelect() throws SQLException {
          checkSql("model", "select NAME,DEPTNO,count(*) over (partition by NAME,DEPTNO) from EMPS");
        }
      
      

      Following exception is thrown:

      java.sql.SQLException: Error while executing SQL "select NAME,DEPTNO,count(*) over (partition by NAME,DEPTNO) from EMPS": Error while compiling generated Java code:
      

      However the document from calcite.apache.org/docs/reference.html states multiple partition columns should be allowed:

      windowSpec:
            [ windowName ]
            '('
            [ ORDER BY orderItem [, orderItem ]* ]
            [ PARTITION BY expression [, expression ]* ]
            [
                RANGE numericOrIntervalExpression { PRECEDING | FOLLOWING }
            |   ROWS numericExpression { PRECEDING | FOLLOWING }
            ]
            ')'
      
      

      After searching it seems no one is reporting the same issue. I'm opening this JIAR as a placeholder, will try to fix this issue

        Issue Links

          Activity

          Hide
          mahongbin hongbin ma added a comment -

          patch attached. Since only no-parameter constructor exists for SyntheticRecordType:

          org.apache.calcite.adapter.enumerable.EnumerableRelImplementor#implementRoot

          // Here a constructor without parameter is used because the generated
          // code could cause error if number of fields is too large.

          I have to manually assign each field value.

          Had little experience on modifying Calcite's source code, please help to review.
          BTW, where should I add more test case queries? I assume there should be a list of test queries to run on both Calcite and another DB, so that execution correctness can be ensured via comparing.

          Show
          mahongbin hongbin ma added a comment - patch attached. Since only no-parameter constructor exists for SyntheticRecordType: org.apache.calcite.adapter.enumerable.EnumerableRelImplementor#implementRoot // Here a constructor without parameter is used because the generated // code could cause error if number of fields is too large. I have to manually assign each field value. Had little experience on modifying Calcite's source code, please help to review. BTW, where should I add more test case queries? I assume there should be a list of test queries to run on both Calcite and another DB, so that execution correctness can be ensured via comparing.
          Hide
          julianhyde Julian Hyde added a comment -

          The easiest way to add a test is to edit winagg.iq, which is run as part of QuidemTest.

          Quidem has a new !verify command (see https://github.com/julianhyde/quidem#verify) but you can't us it in this case because hsqldb doesn't support OVER.

          Code change looks mostly good. Can you fix formatting to make checkstyle happy, and I see that one method now has two javadoc comments.

          Show
          julianhyde Julian Hyde added a comment - The easiest way to add a test is to edit winagg.iq , which is run as part of QuidemTest. Quidem has a new !verify command (see https://github.com/julianhyde/quidem#verify ) but you can't us it in this case because hsqldb doesn't support OVER. Code change looks mostly good. Can you fix formatting to make checkstyle happy, and I see that one method now has two javadoc comments.
          Hide
          mahongbin hongbin ma added a comment -

          Refine the patch to make checkstyle happy.

          I was looking for some code fomatter for Calcite but I didn't find any (https://mail-archives.apache.org/mod_mbox/calcite-dev/201510.mbox/%3CCAKa9qD=TyCnpddQOcfzfXq-4Wfv7NKAtiHfhMumkeZtiuMJofw@mail.gmail.com%3E). I also tried google java code formatter, which will bring massive blank character changes to existing calcite code.

          I find it hard make calcite's checkstyle happy without a standard code formatter, especially when my intellij is configured with code formatter from other projects. Any suggestions?

          Show
          mahongbin hongbin ma added a comment - Refine the patch to make checkstyle happy. I was looking for some code fomatter for Calcite but I didn't find any ( https://mail-archives.apache.org/mod_mbox/calcite-dev/201510.mbox/%3CCAKa9qD=TyCnpddQOcfzfXq-4Wfv7NKAtiHfhMumkeZtiuMJofw@mail.gmail.com%3E ). I also tried google java code formatter, which will bring massive blank character changes to existing calcite code. I find it hard make calcite's checkstyle happy without a standard code formatter, especially when my intellij is configured with code formatter from other projects. Any suggestions?
          Hide
          julianhyde Julian Hyde added a comment -

          You can configure Intellij with a different format for each project. But if you can't find a way to configure Intellij to leave existing code alone, I think you should find an IDE that does: it's a fundamental requirement in open source software.

          Show
          julianhyde Julian Hyde added a comment - You can configure Intellij with a different format for each project. But if you can't find a way to configure Intellij to leave existing code alone, I think you should find an IDE that does: it's a fundamental requirement in open source software.
          Hide
          julianhyde Julian Hyde added a comment -

          hongbin ma, Code looks good. Can you please add a test?

          Show
          julianhyde Julian Hyde added a comment - hongbin ma , Code looks good. Can you please add a test?
          Hide
          mahongbin hongbin ma added a comment -

          sorry my previous question might be confusing. I DO know how to configure different code style for different projects. What I'm asking for is something like http://hbase.apache.org/0.94/book/ides.html, which shows how to config the right FORMATTER for the project with a simple XML:hbase_eclipse_formatter.xml. Calcite seem not to have such thing, so I cannot conveniently configure the IDE's Code Formatter.

          Calcite even has some customized check style rules, like HydromaticFileSetCheck, which makes it hard to configure checkstyle IDE plugins(check its related issue https://issues.apache.org/jira/browse/CALCITE-1127).

          With above situations, I have to laboriously repeat running "mvn clean install -DskipTests" and refine the my codes according to the outputs. Can you share your experience on this?

          Show
          mahongbin hongbin ma added a comment - sorry my previous question might be confusing. I DO know how to configure different code style for different projects. What I'm asking for is something like http://hbase.apache.org/0.94/book/ides.html , which shows how to config the right FORMATTER for the project with a simple XML:hbase_eclipse_formatter.xml. Calcite seem not to have such thing, so I cannot conveniently configure the IDE's Code Formatter. Calcite even has some customized check style rules, like HydromaticFileSetCheck, which makes it hard to configure checkstyle IDE plugins(check its related issue https://issues.apache.org/jira/browse/CALCITE-1127 ). With above situations, I have to laboriously repeat running "mvn clean install -DskipTests" and refine the my codes according to the outputs. Can you share your experience on this?
          Hide
          mahongbin hongbin ma added a comment - - edited

          new patch uploaded with test case added

          Show
          mahongbin hongbin ma added a comment - - edited new patch uploaded with test case added
          Hide
          julianhyde Julian Hyde added a comment -
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/00509094 . Thanks for the patch, hongbin ma !
          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.11.0 (2017-01-11).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.11.0 (2017-01-11).

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              mahongbin hongbin ma
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development