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

RelOptUtil.splitJoinCondition incorrectly splits a join condition

    Details

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

      Description

      For a RelNode such as the following:

      LogicalJoin(condition=[<>(CAST($0):INTEGER NOT NULL, $9)], joinType=[inner]): rowcount = 1.0, cumulative cost = 1.0, id = 7
          LogicalTableScan(table=[[CATALOG, SALES, EMP]]): rowcount = 1.0, cumulative cost = 0.0, id = 5
          LogicalTableScan(table=[[CATALOG, SALES, DEPT]]): rowcount = 1.0, cumulative cost = 0.0, id = 6
      

      If RelOptUtil.splitJoinCondition is fed with this RelNode, the output will be:
      1. Remaining Condition: true (i.e., no remaining condition)
      2. LeftJoinKey: List of (<>(CAST($0):INTEGER NOT NULL, $0), true)
      3. RightJoinKey: Empty List
      (Note the index in the $ which is marked in red)

      However, isn't the expected output supposed to be ?
      1. Remaining Condition: <>(CAST($0):INTEGER NOT NULL, $9)
      2. LeftJoinKey: Empty List
      3. RightJoinKey: Empty List

        Issue Links

          Activity

          Hide
          seanhychu Sean Hsuan-Yi Chu added a comment - - edited

          This branch contains the solution and unit test for this issue.
          https://github.com/hsuanyi/incubator-calcite/tree/CALCITE-833

          Jesus Camacho Rodriguez If possible, can you also help take a look at my fix?

          Show
          seanhychu Sean Hsuan-Yi Chu added a comment - - edited This branch contains the solution and unit test for this issue. https://github.com/hsuanyi/incubator-calcite/tree/CALCITE-833 Jesus Camacho Rodriguez If possible, can you also help take a look at my fix?
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Sean Hsuan-Yi Chu, you are right, it does not work as expected. But I think the new condition should be:

          ...
          if (projRefs.nextSetBit(lowerLimit) < upperLimit
                       && projRefs.nextSetBit(upperLimit) < 0) {
          ...
          

          Consider that the method should work with multi-input joins too. What do you think?

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Sean Hsuan-Yi Chu , you are right, it does not work as expected. But I think the new condition should be: ... if (projRefs.nextSetBit(lowerLimit) < upperLimit && projRefs.nextSetBit(upperLimit) < 0) { ... Consider that the method should work with multi-input joins too. What do you think?
          Hide
          seanhychu Sean Hsuan-Yi Chu added a comment - - edited

          Jesus Camacho Rodriguez, but if we use this condition, the output for my input stated above will be:

          1. Remaining Condition: true
          2. LeftJoinKey: Empty List
          3. RightJoinKey: (<>(CAST($0):INTEGER NOT NULL, $0), true)

          I assume it is supposed to be:

          1. Remaining Condition: <>(CAST($0):INTEGER NOT NULL, $9)
          2. LeftJoinKey: Empty List
          3. RightJoinKey: Empty List

          Is my understanding correct?

          Show
          seanhychu Sean Hsuan-Yi Chu added a comment - - edited Jesus Camacho Rodriguez , but if we use this condition, the output for my input stated above will be: 1. Remaining Condition: true 2. LeftJoinKey: Empty List 3. RightJoinKey: (<>(CAST($0):INTEGER NOT NULL, $0), true) I assume it is supposed to be: 1. Remaining Condition: <>(CAST($0):INTEGER NOT NULL, $9) 2. LeftJoinKey: Empty List 3. RightJoinKey: Empty List Is my understanding correct?
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Sean Hsuan-Yi Chu, sorry about that, you are right; fix looks fine to me, thanks! Julian Hyde/Jacques Nadeau, is this fix landing in 1.4?

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Sean Hsuan-Yi Chu , sorry about that, you are right; fix looks fine to me, thanks! Julian Hyde / Jacques Nadeau , is this fix landing in 1.4?
          Hide
          seanhychu Sean Hsuan-Yi Chu added a comment -

          Honestly, I am not sure. Let's ping Julian.

          Show
          seanhychu Sean Hsuan-Yi Chu added a comment - Honestly, I am not sure. Let's ping Julian.
          Hide
          seanhychu Sean Hsuan-Yi Chu added a comment -

          Julian Hyde Can you help review this patch ? And also, because the method this JIRA issue is talking about does not have a unit test, I added one (in a hack way though...). BTW, Jesus Camacho Rodriguez has already helped reviewed and left some comments above.

          My branch:
          https://github.com/hsuanyi/incubator-calcite/tree/CALCITE-833

          Also, Jesus Camacho Rodriguez would like to know if this fix landing in 1.4?

          Show
          seanhychu Sean Hsuan-Yi Chu added a comment - Julian Hyde Can you help review this patch ? And also, because the method this JIRA issue is talking about does not have a unit test, I added one (in a hack way though...). BTW, Jesus Camacho Rodriguez has already helped reviewed and left some comments above. My branch: https://github.com/hsuanyi/incubator-calcite/tree/CALCITE-833 Also, Jesus Camacho Rodriguez would like to know if this fix landing in 1.4?
          Hide
          julianhyde Julian Hyde added a comment -

          If this is not a regression I am not inclined to put it into 1.4. We need to stabilize 1.4.

          Show
          julianhyde Julian Hyde added a comment - If this is not a regression I am not inclined to put it into 1.4. We need to stabilize 1.4.
          Hide
          julianhyde Julian Hyde added a comment -

          I'll make the same comments as I made to CALCITE-688, which is a very similar issue:

          The logic would be easier to understand and maintain if you could express it in terms of set-theory operations on bit sets, rather than the low-level nextSetBit operations. You could create a bit set that represents all of the fields of a particular input, using ImmutableBitSet.range(int, int), then intersect it with the bit sets of the conditions.

          Show
          julianhyde Julian Hyde added a comment - I'll make the same comments as I made to CALCITE-688 , which is a very similar issue: The logic would be easier to understand and maintain if you could express it in terms of set-theory operations on bit sets, rather than the low-level nextSetBit operations. You could create a bit set that represents all of the fields of a particular input, using ImmutableBitSet.range(int, int), then intersect it with the bit sets of the conditions.
          Hide
          julianhyde Julian Hyde added a comment -

          I think RexTransformTest would be the best place for this test case. That is where we are testing miscellaneous utilities that work on RelNodes and RexNodes. Frankly we need better unit tests for all of the methods in RelOptUtil.

          Since you need to convert SQL to RelNodes, you could do

          new SqlToRelTestBase() {}.createTester().convertSqlToRel()

          . Not pretty, but we can refactor later.

          Show
          julianhyde Julian Hyde added a comment - I think RexTransformTest would be the best place for this test case. That is where we are testing miscellaneous utilities that work on RelNodes and RexNodes. Frankly we need better unit tests for all of the methods in RelOptUtil. Since you need to convert SQL to RelNodes, you could do new SqlToRelTestBase() {}.createTester().convertSqlToRel() . Not pretty, but we can refactor later.
          Hide
          julianhyde Julian Hyde added a comment -

          According to Jinfeng Ni this is a regression. Let's put it into 1.4.

          Show
          julianhyde Julian Hyde added a comment - According to Jinfeng Ni this is a regression. Let's put it into 1.4.
          Hide
          seanhychu Sean Hsuan-Yi Chu added a comment - - edited

          Just sent out a pull request. Thanks.

          https://github.com/apache/incubator-calcite/pull/119

          Show
          seanhychu Sean Hsuan-Yi Chu added a comment - - edited Just sent out a pull request. Thanks. https://github.com/apache/incubator-calcite/pull/119
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/a61be0db . Thanks for the patch, Sean Hsuan-Yi Chu !
          Hide
          julianhyde Julian Hyde added a comment -

          And I've pushed a new snapshot based on this commit.

          Show
          julianhyde Julian Hyde added a comment - And I've pushed a new snapshot based on this commit.
          Hide
          jnadeau Jacques Nadeau added a comment -

          Resolved in release 1.4.0-incubating (2015-08-23)

          Show
          jnadeau Jacques Nadeau added a comment - Resolved in release 1.4.0-incubating (2015-08-23)

            People

            • Assignee:
              seanhychu Sean Hsuan-Yi Chu
              Reporter:
              seanhychu Sean Hsuan-Yi Chu
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development