Uploaded image for project: 'Flink'
  1. Flink
  2. FLINK-3736

Move toRexNode and toAggCall logic into Expressions

Details

    • Improvement
    • Status: Closed
    • Minor
    • Resolution: Implemented
    • None
    • 1.1.0
    • Table SQL / API
    • None

    Description

      Since we have a one-to-one mapping from Flink Expression to Calcite RexNode, and we will implement more and more expressions to make flink-table a full-fledged module, I think it's worthwhile to let each Expression take care of its own behaviour on conversion to RexNode.

      Attachments

        Activity

          githubbot ASF GitHub Bot added a comment -

          GitHub user yjshen opened a pull request:

          https://github.com/apache/flink/pull/1870

          FLINK-3736[TableAPI]Move toRexNode & toAggCall logic into each expression's implementation

          Since we have a one-to-one mapping from Flink `Expression` to Calcite `RexNode`, I think it's a good practice to make `toRexNode` a behaviour of each Expression.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/yjshen/flink toRexNode

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/flink/pull/1870.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #1870


          commit 32085a3f559a9d5510a85de01c63a23ce5497535
          Author: Yijie Shen <henry.yijieshen@gmail.com>
          Date: 2016-04-11T17:33:15Z

          move toRexNode logic into each expression's implementation


          githubbot ASF GitHub Bot added a comment - GitHub user yjshen opened a pull request: https://github.com/apache/flink/pull/1870 FLINK-3736 [TableAPI] Move toRexNode & toAggCall logic into each expression's implementation Since we have a one-to-one mapping from Flink `Expression` to Calcite `RexNode`, I think it's a good practice to make `toRexNode` a behaviour of each Expression. You can merge this pull request into a Git repository by running: $ git pull https://github.com/yjshen/flink toRexNode Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/1870.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1870 commit 32085a3f559a9d5510a85de01c63a23ce5497535 Author: Yijie Shen <henry.yijieshen@gmail.com> Date: 2016-04-11T17:33:15Z move toRexNode logic into each expression's implementation
          githubbot ASF GitHub Bot added a comment -

          Github user fhueske commented on the pull request:

          https://github.com/apache/flink/pull/1870#issuecomment-208542966

          Hi @yjshen, thanks for the PR! I agree, moving the `RexNode` translation to the individual `Expression` makes a lot of sense. I only skimmed the PR and did not have a detailed look but everything looks fine.
          Will check the PR more thoroughly tomorrow and probably merge it.

          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on the pull request: https://github.com/apache/flink/pull/1870#issuecomment-208542966 Hi @yjshen, thanks for the PR! I agree, moving the `RexNode` translation to the individual `Expression` makes a lot of sense. I only skimmed the PR and did not have a detailed look but everything looks fine. Will check the PR more thoroughly tomorrow and probably merge it.
          githubbot ASF GitHub Bot added a comment -

          Github user yjshen commented on the pull request:

          https://github.com/apache/flink/pull/1870#issuecomment-208661457

          The build failure seems unrelated to this PR:
          ```
          [INFO] Compiling 43 source files to /home/travis/build/apache/flink/flink-libraries/flink-ml/target/classes at 1460411368867
          [ERROR] error: error while loading <root>, error in opening zip file
          [ERROR] error: scala.reflect.internal.MissingRequirementError: object scala.runtime in compiler mirror not found.
          [ERROR] at scala.reflect.internal.MissingRequirementError$.signal(MissingRequirementError.scala:16)
          [ERROR] at scala.reflect.internal.MissingRequirementError$.notFound(MissingRequirementError.scala:17)
          ```

          githubbot ASF GitHub Bot added a comment - Github user yjshen commented on the pull request: https://github.com/apache/flink/pull/1870#issuecomment-208661457 The build failure seems unrelated to this PR: ``` [INFO] Compiling 43 source files to /home/travis/build/apache/flink/flink-libraries/flink-ml/target/classes at 1460411368867 [ERROR] error: error while loading <root>, error in opening zip file [ERROR] error: scala.reflect.internal.MissingRequirementError: object scala.runtime in compiler mirror not found. [ERROR] at scala.reflect.internal.MissingRequirementError$.signal(MissingRequirementError.scala:16) [ERROR] at scala.reflect.internal.MissingRequirementError$.notFound(MissingRequirementError.scala:17) ```
          githubbot ASF GitHub Bot added a comment -

          Github user yjshen commented on the pull request:

          https://github.com/apache/flink/pull/1870#issuecomment-208663670

          @fhueske, thanks . Please let me know your thoughts when you give another pass.

          githubbot ASF GitHub Bot added a comment - Github user yjshen commented on the pull request: https://github.com/apache/flink/pull/1870#issuecomment-208663670 @fhueske, thanks . Please let me know your thoughts when you give another pass.
          githubbot ASF GitHub Bot added a comment -

          Github user fhueske commented on the pull request:

          https://github.com/apache/flink/pull/1870#issuecomment-208822898

          Changes look good.
          Will merge this PR.
          Thanks for the contribution @yjshen!

          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on the pull request: https://github.com/apache/flink/pull/1870#issuecomment-208822898 Changes look good. Will merge this PR. Thanks for the contribution @yjshen!
          fhueske Fabian Hueske added a comment -

          Hi yijieshen, I gave you contributor permissions for the Flink JIRA. You can now assign issues to yourself if you want to. Thanks, Fabian

          fhueske Fabian Hueske added a comment - Hi yijieshen , I gave you contributor permissions for the Flink JIRA. You can now assign issues to yourself if you want to. Thanks, Fabian
          githubbot ASF GitHub Bot added a comment -

          Github user yjshen commented on the pull request:

          https://github.com/apache/flink/pull/1870#issuecomment-208829993

          I am glad I was able to help. Thanks for the reviewing work @fhueske!

          githubbot ASF GitHub Bot added a comment - Github user yjshen commented on the pull request: https://github.com/apache/flink/pull/1870#issuecomment-208829993 I am glad I was able to help. Thanks for the reviewing work @fhueske!
          yijieshen Yijie Shen added a comment -

          Ah I see, thanks

          yijieshen Yijie Shen added a comment - Ah I see, thanks
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/flink/pull/1870

          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/1870
          fhueske Fabian Hueske added a comment -

          Implemented with ba46ab6b659ffca60ea4a7b69f637622b9eb000c

          Thanks for the contribution!

          fhueske Fabian Hueske added a comment - Implemented with ba46ab6b659ffca60ea4a7b69f637622b9eb000c Thanks for the contribution!

          People

            yijieshen Yijie Shen
            yijieshen Yijie Shen
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: