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

Add back NavigationExpander and NavigationReplacer in SqlValidatorImpl

    Details

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

      Description

      NavigationExpander and NavigationReplacer was deleted as a fix of issue CALCITE-1871. Just create this ticket to track whether we should add them back.

        Activity

        Hide
        dian.fu Dian Fu added a comment -

        For NavigationExpander, currently I have not got a quite convincing explanation we should add it back. While for NavigationReplacer, I think it has its values. NavigationReplacer can convert expressions like A.price > PREV(B.price) to PREV(A.price, 0) > PREV(B.price, 1). This will make the implementation unified:
        1) step 1: RexPatternFieldRef A.price or B.price
        2) step 2: RexCall (PREV)

        Show
        dian.fu Dian Fu added a comment - For NavigationExpander , currently I have not got a quite convincing explanation we should add it back. While for NavigationReplacer , I think it has its values. NavigationReplacer can convert expressions like A.price > PREV(B.price) to PREV(A.price, 0) > PREV(B.price, 1) . This will make the implementation unified: 1) step 1: RexPatternFieldRef A.price or B.price 2) step 2: RexCall (PREV)
        Hide
        dian.fu Dian Fu added a comment -

        I have created a PR: https://github.com/apache/calcite/pull/516

        Julian Hyde, Zhiqiang He, Please help to review.

        Show
        dian.fu Dian Fu added a comment - I have created a PR: https://github.com/apache/calcite/pull/516 Julian Hyde , Zhiqiang He , Please help to review.
        Hide
        julianhyde Julian Hyde added a comment -

        Why is PREV("A"."net_weight", 0) better than "A"."net_weight"?

        Show
        julianhyde Julian Hyde added a comment - Why is PREV("A"."net_weight", 0) better than "A"."net_weight" ?
        Hide
        julianhyde Julian Hyde added a comment -

        I get the sense that you have had design discussions in private. Nowhere have you explained what "navigation" is.

        Show
        julianhyde Julian Hyde added a comment - I get the sense that you have had design discussions in private. Nowhere have you explained what "navigation" is.
        Hide
        dian.fu Dian Fu added a comment -

        You're right. This is related to how to implement the above expression. I think PREV("A"."net_weight", 0) will make the implementation more unified.
        Let's image how we implement PREV("A"."net_weight", 0). This may be separated into two steps:
        1) Calculate "A"."net_weight" (suppose the result is result1)
        2) Calculate PREV(result1, 0)
        This means that the result of "A"."net_weight" is better not be an integer value (this depends on the implementation), otherwise it will be impossible to calculate PREV(result1, 0) as it makes no sense if the first argument of PREV is an integer value.

        So for expression A.net_weight > PREV(B.net_weight, 1), the type of A.net_weight may not be an integer value (depends on the implementation) and the compare may make no sense.

        Show
        dian.fu Dian Fu added a comment - You're right. This is related to how to implement the above expression. I think PREV("A"."net_weight", 0) will make the implementation more unified. Let's image how we implement PREV("A"."net_weight", 0) . This may be separated into two steps: 1) Calculate "A"."net_weight" (suppose the result is result1) 2) Calculate PREV(result1, 0) This means that the result of "A"."net_weight" is better not be an integer value (this depends on the implementation), otherwise it will be impossible to calculate PREV(result1, 0) as it makes no sense if the first argument of PREV is an integer value. So for expression A.net_weight > PREV(B.net_weight, 1) , the type of A.net_weight may not be an integer value (depends on the implementation) and the compare may make no sense.
        Hide
        dian.fu Dian Fu added a comment -

        In one word, if we don't transform A.net_weight to PREV(A.net_weight, 0), it will make it difficult to implement the following method in RexVisitor.

          R visitPatternFieldRef(RexPatternFieldRef fieldRef);
        

        If the return result of this method is the specified field of the element, then the negation will be impossible. If not, the comparation of A.net_weight > PREV(B.net_weight, 1) will make non sense.

        Show
        dian.fu Dian Fu added a comment - In one word, if we don't transform A.net_weight to PREV(A.net_weight, 0) , it will make it difficult to implement the following method in RexVisitor . R visitPatternFieldRef(RexPatternFieldRef fieldRef); If the return result of this method is the specified field of the element, then the negation will be impossible. If not, the comparation of A.net_weight > PREV(B.net_weight, 1) will make non sense.
        Hide
        julianhyde Julian Hyde added a comment -

        Makes sense. Please document that rationale in the code and I will accept the PR. Add a comment to this case when you have added the commit. No need to squash or rebase.

        Show
        julianhyde Julian Hyde added a comment - Makes sense. Please document that rationale in the code and I will accept the PR. Add a comment to this case when you have added the commit. No need to squash or rebase.
        Hide
        dian.fu Dian Fu added a comment -

        Julian Hyde, I have updated the PR, documented the rationale at the header of class NavigationReplacer and also updated the commit message.

        Show
        dian.fu Dian Fu added a comment - Julian Hyde , I have updated the PR, documented the rationale at the header of class NavigationReplacer and also updated the commit message.
        Hide
        julianhyde Julian Hyde added a comment -

        Looks good. I will commit when tests pass. I changed NavigationReplacer.visit to call super.visit; it's simpler than what you had.

        On reflection, your approach of modifying the SqlNode tree is an anti-pattern. Ask anyone who has tried to optimize queries by changing the AST. It quickly becomes a mess. The next time you need something like NavigationReplacer, it should operate on RexNode rather than SqlNode.

        Show
        julianhyde Julian Hyde added a comment - Looks good. I will commit when tests pass. I changed NavigationReplacer.visit to call super.visit; it's simpler than what you had. On reflection, your approach of modifying the SqlNode tree is an anti-pattern. Ask anyone who has tried to optimize queries by changing the AST. It quickly becomes a mess. The next time you need something like NavigationReplacer, it should operate on RexNode rather than SqlNode.
        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/d3a7c0d7 ; thanks for the PR, Dian Fu !
        Hide
        michaelmior Michael Mior added a comment -

        Resolved in release 1.14.0 (2017-10-01)

        Show
        michaelmior Michael Mior added a comment - Resolved in release 1.14.0 (2017-10-01)

          People

          • Assignee:
            julianhyde Julian Hyde
            Reporter:
            dian.fu Dian Fu
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development