Details

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

      Description

      Create a rule that pulls up constants through a Union operator.

        Issue Links

          Activity

          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Julian Hyde, I have created a PR with the new rule in : https://github.com/apache/calcite/pull/275

          Could you review it? Thanks

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Julian Hyde , I have created a PR with the new rule in : https://github.com/apache/calcite/pull/275 Could you review it? Thanks
          Hide
          julianhyde Julian Hyde added a comment -

          I think count == 1 is wrong. What if there were two constant columns, select 1, 2 ... union all select 1, 2 .... When they are removed you have an empty union.

          Can you add a test case SELECT ... FROM (SELECT 2 AS x ... UNION ALL SELECT 3 ...) WHERE x > 5? It's a negative test case, of course, but it there's a real danger the logic would get confused by two columns that are constant but are different constants, so it's worth testing.

          Just curious, why did you pull up constants (creating a Project) rather than just pulling up predicates? Predicates are more powerful. For instance, in the above case you could easily pull pull up the predicate x = 2 or x = 3 in the above case. Then select ... from v where x > 5 would benefit.

          You rely on RexNode.hashCode and .equals. RexInputRef has .hashCode and .equals but RexCall does not. You only use RexInputRef, so I think you're OK, but be careful. (Yes, we should probably add hashNode and equals for all RexNode sub-classes.)

          Show
          julianhyde Julian Hyde added a comment - I think count == 1 is wrong. What if there were two constant columns, select 1, 2 ... union all select 1, 2 ... . When they are removed you have an empty union. Can you add a test case SELECT ... FROM (SELECT 2 AS x ... UNION ALL SELECT 3 ...) WHERE x > 5 ? It's a negative test case, of course, but it there's a real danger the logic would get confused by two columns that are constant but are different constants, so it's worth testing. Just curious, why did you pull up constants (creating a Project) rather than just pulling up predicates? Predicates are more powerful. For instance, in the above case you could easily pull pull up the predicate x = 2 or x = 3 in the above case. Then select ... from v where x > 5 would benefit. You rely on RexNode.hashCode and .equals. RexInputRef has .hashCode and .equals but RexCall does not. You only use RexInputRef, so I think you're OK, but be careful. (Yes, we should probably add hashNode and equals for all RexNode sub-classes.)
          Hide
          julianhyde Julian Hyde added a comment -

          Re "Can you add a test case". Oops, I see you have that already.

          Show
          julianhyde Julian Hyde added a comment - Re "Can you add a test case". Oops, I see you have that already.
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - - edited

          Thanks for the careful review Julian Hyde.

          I have added a test case for select 1, 2 ... union all select 1, 2 ..... count==1 was an early bail out; in principle, I thought it could indeed be removed, but we need it so the rule is not triggered indefinitely. In the new commit, I also addressed your comment about the iterable on the BitSet.

          Edit: nevermind, I have just rewritten the rule so we can bail out later, and hence we can pull up a single constant too.

          Edit2: I thought I had figured out a way to detect this loop, but it does not seem straightforward, and we still end up in the firing the rule indefinitely. Thus, I think we need to leave the guard code for count=1 as it is.

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - - edited Thanks for the careful review Julian Hyde . I have added a test case for select 1, 2 ... union all select 1, 2 .... . count==1 was an early bail out; in principle, I thought it could indeed be removed, but we need it so the rule is not triggered indefinitely. In the new commit, I also addressed your comment about the iterable on the BitSet. Edit: nevermind, I have just rewritten the rule so we can bail out later, and hence we can pull up a single constant too. Edit2: I thought I had figured out a way to detect this loop, but it does not seem straightforward, and we still end up in the firing the rule indefinitely. Thus, I think we need to leave the guard code for count=1 as it is.
          Hide
          julianhyde Julian Hyde added a comment -

          Can you add this rule to RelOptUtil.registerAbstractRels? I think it would be useful to have it in the standard rule set (and it would get more testing).

          Show
          julianhyde Julian Hyde added a comment - Can you add this rule to RelOptUtil.registerAbstractRels ? I think it would be useful to have it in the standard rule set (and it would get more testing).
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Sure.

          Wrt considering predicates instead of constants, I think it could be done indeed. I did not do it originally because the sample cases in Hive were only targeting constant values, thus no special reason to not implement it.

          We could create a follow-up issue and implement it later on.

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Sure. Wrt considering predicates instead of constants, I think it could be done indeed. I did not do it originally because the sample cases in Hive were only targeting constant values, thus no special reason to not implement it. We could create a follow-up issue and implement it later on.
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Julian Hyde, I have addressed the review comments, I think the patch is now ready to go in. Is it OK?

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Julian Hyde , I have addressed the review comments, I think the patch is now ready to go in. Is it OK?
          Hide
          julianhyde Julian Hyde added a comment -

          Looks good, but running final tests; I will commit when they pass.

          Show
          julianhyde Julian Hyde added a comment - Looks good, but running final tests; I will commit when they pass.
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/9b1624a8 .
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          There was a wrong method call in the md provider : it should have been isAlwaysTrue, which is the case when we do not need to add the pred to the inferred list (it is that way in Hive, I think it ended up changed when I copied/pasted the code).

          Fix up in : http://git-wip-us.apache.org/repos/asf/calcite/commit/317802d

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - There was a wrong method call in the md provider : it should have been isAlwaysTrue , which is the case when we do not need to add the pred to the inferred list (it is that way in Hive, I think it ended up changed when I copied/pasted the code). Fix up in : http://git-wip-us.apache.org/repos/asf/calcite/commit/317802d
          Hide
          julianhyde Julian Hyde added a comment -

          Fix up now committed as part of CALCITE-1366.

          Show
          julianhyde Julian Hyde added a comment - Fix up now committed as part of CALCITE-1366 .
          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)

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              jcamachorodriguez Jesus Camacho Rodriguez
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development