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

Calcite incorrectly permutes columns of OVER query

    Details

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

      Description

      To reproduce this issue, add ProjectToWindowRule to the Hep Planner.

      Then, a query below would fail with an Assertion Error (type mis-match)

      select count(*) over(partition by empno order by sal) as count1,
            count(*) over(partition by deptno order by sal) as count2, 
            sum(deptno)  over(partition by empno order by sal) as sum1
      from emp
      

      However, if the second and third columns are swapped, then it works:

      select count(*) over(partition by empno order by sal) as count1,
            sum(deptno)  over(partition by empno order by sal) as sum1,
            count(*) over(partition by deptno order by sal) as count2
      from emp
      

      Essentially, the window functions which "share the same window definition" should be placed consecutively in the select-list; Otherwise, the current linking mechanism seems having some difficulty.

        Activity

        Hide
        seanhychu Sean Hsuan-Yi Chu added a comment -

        If the step to reproduce this issue has not well illustrated in the description, please refer to this branch:
        https://github.com/hsuanyi/incubator-calcite/tree/CALCITE-827

        Show
        seanhychu Sean Hsuan-Yi Chu added a comment - If the step to reproduce this issue has not well illustrated in the description, please refer to this branch: https://github.com/hsuanyi/incubator-calcite/tree/CALCITE-827
        Hide
        seanhychu Sean Hsuan-Yi Chu added a comment -

        I have done some investigation for this issue, I would like to keep working on this one.

        Show
        seanhychu Sean Hsuan-Yi Chu added a comment - I have done some investigation for this issue, I would like to keep working on this one.
        Hide
        seanhychu Sean Hsuan-Yi Chu added a comment -

        Julian Hyde In LogicalWindow, Line 241, there is a comment:

        // TODO: The order that the "over" calls occur in the groups and
        // partitions may not match the order in which they occurred in the
        // original expression. We should add a project to permute them.
        

        This sounds like the root cause of this issue? So I will just work along that direction?

        Show
        seanhychu Sean Hsuan-Yi Chu added a comment - Julian Hyde In LogicalWindow, Line 241, there is a comment: // TODO: The order that the "over" calls occur in the groups and // partitions may not match the order in which they occurred in the // original expression. We should add a project to permute them. This sounds like the root cause of this issue? So I will just work along that direction?
        Hide
        julianhyde Julian Hyde added a comment -

        Yes, that sounds very likely. I wrote that comment so thank me for helping/blame me for not doing it right first time (your choice).

        Show
        julianhyde Julian Hyde added a comment - Yes, that sounds very likely. I wrote that comment so thank me for helping/blame me for not doing it right first time (your choice).
        Hide
        julianhyde Julian Hyde added a comment -

        Also, the fact that "shuttle" (declared at line 197 https://github.com/apache/incubator-calcite/blob/3e50232b681e8dadb921580ee6f3e0376dd0f664/core/src/main/java/org/apache/calcite/rel/logical/LogicalWindow.java#L197) is not used seems to be worrying. If the code used a shuttle to replace RexOvers inside the project expressions then the permutation would probably happen automatically.

        Show
        julianhyde Julian Hyde added a comment - Also, the fact that "shuttle" (declared at line 197 https://github.com/apache/incubator-calcite/blob/3e50232b681e8dadb921580ee6f3e0376dd0f664/core/src/main/java/org/apache/calcite/rel/logical/LogicalWindow.java#L197 ) is not used seems to be worrying. If the code used a shuttle to replace RexOvers inside the project expressions then the permutation would probably happen automatically.
        Hide
        seanhychu Sean Hsuan-Yi Chu added a comment -

        Right! It seems you had implemented the needed utility. We hooked them up here: https://github.com/apache/incubator-calcite/pull/115

        Show
        seanhychu Sean Hsuan-Yi Chu added a comment - Right! It seems you had implemented the needed utility. We hooked them up here: https://github.com/apache/incubator-calcite/pull/115
        Hide
        amansinha100 Aman Sinha added a comment -

        Changes look ok to me. Added one minor comment for the test file.
        +1

        Show
        amansinha100 Aman Sinha added a comment - Changes look ok to me. Added one minor comment for the test file. +1
        Hide
        seanhychu Sean Hsuan-Yi Chu added a comment -

        Julian Hyde Can you help us review? Thanks.

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

        Addressed the review comments. Julian Hyde Can you help us review? Thanks.

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

        Looks good. Running tests now.

        Should this be in release 1.4? I am inclined to say no. Even though this is a bug, it is not a regression.

        Show
        julianhyde Julian Hyde added a comment - Looks good. Running tests now. Should this be in release 1.4? I am inclined to say no. Even though this is a bug, it is not a regression.
        Hide
        seanhychu Sean Hsuan-Yi Chu added a comment -

        Right, It is not a regression. Since we could just apply this patch into Drill's calcite, I am neutral on that.

        Show
        seanhychu Sean Hsuan-Yi Chu added a comment - Right, It is not a regression. Since we could just apply this patch into Drill's calcite, I am neutral on that.
        Hide
        julianhyde Julian Hyde added a comment -

        I have committed to my https://github.com/julianhyde/incubator-calcite/tree/new-master branch. I will merge to apache/master after 1.4. Thanks for the patch, Sean Hsuan-Yi Chu!

        Show
        julianhyde Julian Hyde added a comment - I have committed to my https://github.com/julianhyde/incubator-calcite/tree/new-master branch. I will merge to apache/master after 1.4. Thanks for the patch, Sean Hsuan-Yi Chu !
        Show
        julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/b9670731 . 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:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development