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

ProjectPusher should use rel factories when creating new rels, e.g. project/filter

    Details

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

      Description

      I was playing around with some new ideas in drill, and wanted to use ProjectPusher, which has a lot of useful utility functions. ProjectPusher returns new nodes, but they are LogicalProjects/LogicalFilters. It would be more useful if the user of ProjectPusher could specify the rel factories to use.

        Activity

        Hide
        minjikim MinJi Kim added a comment -

        I made a small change to use rel factories in ProjectPusher. Thanks!

        https://github.com/apache/calcite/pull/265

        Show
        minjikim MinJi Kim added a comment - I made a small change to use rel factories in ProjectPusher. Thanks! https://github.com/apache/calcite/pull/265
        Hide
        julianhyde Julian Hyde added a comment -

        MinJi Kim, As of https://github.com/apache/calcite/pull/265/commits/e354a214f1e78c90d3147dce53b54f3f1090fbf0 I see lots of factory arguments to ProjectFilterTransposeRule. Can you convert those arguments to a single RelBuilder?

        It would be good to get this done in time for 1.9. Do you think that is achievable?

        Show
        julianhyde Julian Hyde added a comment - MinJi Kim , As of https://github.com/apache/calcite/pull/265/commits/e354a214f1e78c90d3147dce53b54f3f1090fbf0 I see lots of factory arguments to ProjectFilterTransposeRule. Can you convert those arguments to a single RelBuilder? It would be good to get this done in time for 1.9. Do you think that is achievable?
        Hide
        minjikim MinJi Kim added a comment -

        Will do! I am sorry for the delay. I got caught in something, and have been meaning to get back to this for a while. I will try to get to this this week (or this weekend!).

        Show
        minjikim MinJi Kim added a comment - Will do! I am sorry for the delay. I got caught in something, and have been meaning to get back to this for a while. I will try to get to this this week (or this weekend!).
        Hide
        minjikim MinJi Kim added a comment -

        Julian Hyde I uploaded an updated patch. Please let me know if there is anything that I should address. Thanks!

        Show
        minjikim MinJi Kim added a comment - Julian Hyde I uploaded an updated patch. Please let me know if there is anything that I should address. Thanks!
        Hide
        julianhyde Julian Hyde added a comment -

        MinJi Kim, I saw you modified the arguments to RelOptUtil.createProject(RelNode, List, List, boolean, ProjectFactory) and was going to suggest that retain a deprecated copy of that method. But then I saw that Jesus Camacho Rodriguez only recently created the method, in CALCITE-826. So we're good.

        I believe that RelOptUtil.createProject(RelNode, List, boolean, ProjectFactory) is not used, and it has not been released. Can you remove it?

        After that, +1. Can you please commit and push to Apache master?

        • Be sure to add a 'Close apache/calcite#NNN' line, to automatically close the PR.
        • Don't add ' (MinJi Kim)' at the end of the message - we only do that for non-committers.
        • Please mark this case resolved, and fixed in 1.9.0, after you commit.
        Show
        julianhyde Julian Hyde added a comment - MinJi Kim , I saw you modified the arguments to RelOptUtil.createProject(RelNode, List, List, boolean, ProjectFactory) and was going to suggest that retain a deprecated copy of that method. But then I saw that Jesus Camacho Rodriguez only recently created the method, in CALCITE-826 . So we're good. I believe that RelOptUtil.createProject(RelNode, List, boolean, ProjectFactory) is not used, and it has not been released. Can you remove it? After that, +1. Can you please commit and push to Apache master? Be sure to add a 'Close apache/calcite#NNN' line, to automatically close the PR. Don't add ' (MinJi Kim)' at the end of the message - we only do that for non-committers. Please mark this case resolved, and fixed in 1.9.0, after you commit.
        Hide
        minjikim MinJi Kim added a comment -

        Okay, sure. I will make the changes. I will give it a try at my first commit!

        Show
        minjikim MinJi Kim added a comment - Okay, sure. I will make the changes. I will give it a try at my first commit!
        Hide
        julianhyde Julian Hyde added a comment - - edited

        MinJi Kim, When you mark a bug resolved, can you please also add a "Fixed in ..." comment so we can tie back to the source code change. It makes it much easier for others to review. I usually paste in the commit URL from the email sent by ASF's git-hook, and add a "thank you" if it is from a non-committer. See CALCITE-1337 for an example.

        Show
        julianhyde Julian Hyde added a comment - - edited MinJi Kim , When you mark a bug resolved, can you please also add a "Fixed in ..." comment so we can tie back to the source code change. It makes it much easier for others to review. I usually paste in the commit URL from the email sent by ASF's git-hook, and add a "thank you" if it is from a non-committer. See CALCITE-1337 for an example.
        Hide
        minjikim MinJi Kim added a comment -

        Okay, will remember from now on!

        Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/cb356d8a

        Show
        minjikim MinJi Kim added a comment - Okay, will remember from now on! Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/cb356d8a
        Hide
        jcamachorodriguez Jesus Camacho Rodriguez added a comment -

        Resolved in release 1.9.0 (2016-09-22)

        Show
        jcamachorodriguez Jesus Camacho Rodriguez added a comment - Resolved in release 1.9.0 (2016-09-22)
        Hide
        julianhyde Julian Hyde added a comment -

        FYI, this commit broke QuidemTest, viz, it changed some plans in misc.iq. (I'm not sure how we missed it, but at least the plan changes were improvements!) Fixed the test in http://git-wip-us.apache.org/repos/asf/calcite/commit/b5b9e53a.

        Show
        julianhyde Julian Hyde added a comment - FYI, this commit broke QuidemTest, viz, it changed some plans in misc.iq . (I'm not sure how we missed it, but at least the plan changes were improvements!) Fixed the test in http://git-wip-us.apache.org/repos/asf/calcite/commit/b5b9e53a .
        Hide
        minjikim MinJi Kim added a comment -

        Oh, that's weird. I do a full build (mvn clean install, just to be sure), and I thought that would run all tests including Quidem. I will keep an eye out for it next time.

        Show
        minjikim MinJi Kim added a comment - Oh, that's weird. I do a full build (mvn clean install, just to be sure), and I thought that would run all tests including Quidem. I will keep an eye out for it next time.

          People

          • Assignee:
            julianhyde Julian Hyde
            Reporter:
            minjikim MinJi Kim
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development