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

Predicate should be simplified when we create a Filter operator

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.5.0
    • Fix Version/s: 1.6.0
    • Component/s: core
    • Labels:
      None

      Description

      When we create a Filter operator, there is a chance we end up with duplicate predicate factors (for instance, this has been seen in Hive when we use FilterMergeRule). It seems appropriate to simplify the predicate to eliminate those factors before we create the filter.

        Issue Links

          Activity

          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.6.0 (2016-01-22).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.6.0 (2016-01-22).
          Show
          julianhyde Julian Hyde added a comment - Revised fix http://git-wip-us.apache.org/repos/asf/calcite/commit/a67b4a97 .
          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/9f6f23d .
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Julian Hyde, Vladimir Sitnikov, catching up with this one.

          I updated the pull request and added an additional test in RelBuilder.

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

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Julian Hyde , Vladimir Sitnikov , catching up with this one. I updated the pull request and added an additional test in RelBuilder. https://github.com/apache/calcite/pull/171
          Hide
          julianhyde Julian Hyde added a comment -

          I too am usually worried about overhead. However, the overhead is a quick switch (node.getKind()) for each conjunction, followed by simplifyAnd applied to the list of conjunctions, which, if the expression has already been simplified and has no NOTs, is hardly slower than the composeConjunction that filter calls anyway.

          Show
          julianhyde Julian Hyde added a comment - I too am usually worried about overhead. However, the overhead is a quick switch (node.getKind()) for each conjunction, followed by simplifyAnd applied to the list of conjunctions, which, if the expression has already been simplified and has no NOTs, is hardly slower than the composeConjunction that filter calls anyway.
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Julian Hyde, Vladimir Sitnikov, thanks for the comments. I agree with you, we need a test case; I'll add it so you can see clearly the purpose of the improvement and check its correctness.

          I could delegate the check to RelBuilder.filter indeed, though I am afraid we might be introducing an unnecessary overhead in many cases; that is why I think it is better to keep it in FilterMergeRule.

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Julian Hyde , Vladimir Sitnikov , thanks for the comments. I agree with you, we need a test case; I'll add it so you can see clearly the purpose of the improvement and check its correctness. I could delegate the check to RelBuilder.filter indeed, though I am afraid we might be introducing an unnecessary overhead in many cases; that is why I think it is better to keep it in FilterMergeRule.
          Hide
          julianhyde Julian Hyde added a comment -

          I concur with Vladimir Sitnikov's review comment that we need a test case. It would help me understand why you want this. I had thought that RelBuilder.filter was already simplifying - but I now see that it is not. Why not make RelBuilder.filter always simplify?

          If you modify RelBuilder.filter, then a test case in RelBuilderTest will be sufficient.

          Show
          julianhyde Julian Hyde added a comment - I concur with Vladimir Sitnikov 's review comment that we need a test case. It would help me understand why you want this. I had thought that RelBuilder.filter was already simplifying - but I now see that it is not. Why not make RelBuilder.filter always simplify? If you modify RelBuilder.filter, then a test case in RelBuilderTest will be sufficient.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development