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

Improve how ReduceExpressionsRule handles duplicate constraints

    Details

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

      Description

      We need to improve ReduceExpressionRule to deal with multiple "equals to literal" in predicate. In ReduceExpressionRule.java, L396-397:

      final ImmutableMap<RexNode, RexLiteral> constants =
             predicateConstants(predicates);

      The query is

      select * from src where (key='12' and key is null);

      Here 'key' is a string type and it is one of the columns of 'src' table.

      Then we will have predicates

      [=($0, '12'), isnull($0)]

      which is the input of the
      predicateConstants, and the function will return "{$0='12'}"

      1. CALCITE-935.01.patch
        5 kB
        Pengcheng Xiong

        Activity

        Hide
        jcamachorodriguez Jesus Camacho Rodriguez added a comment -

        Resolved in release 1.5.0 (2015-11-10)

        Show
        jcamachorodriguez Jesus Camacho Rodriguez added a comment - Resolved in release 1.5.0 (2015-11-10)
        Show
        julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/690faa55 . Thanks for the patch, Pengcheng Xiong !
        Hide
        pxiong Pengcheng Xiong added a comment -

        Hi Julian, Thanks for your efforts. I thought about the multi map for a while and I changed it back to basic map due to 2 reasons (1) as RexLiteral is immutable too, a constant '10' is different from a constant '10'. Thus there will be two values of '10'. (2) We only need a single literal value. Storing multiple values in a multi-map is not that useful. The new patch is in https://github.com/julianhyde/incubator-calcite/commit/ee02796a4f7a1c26f040657bb58026fa03da8666 and I already sent u a pull request. Plz let me know what you think. Thanks.

        Show
        pxiong Pengcheng Xiong added a comment - Hi Julian, Thanks for your efforts. I thought about the multi map for a while and I changed it back to basic map due to 2 reasons (1) as RexLiteral is immutable too, a constant '10' is different from a constant '10'. Thus there will be two values of '10'. (2) We only need a single literal value. Storing multiple values in a multi-map is not that useful. The new patch is in https://github.com/julianhyde/incubator-calcite/commit/ee02796a4f7a1c26f040657bb58026fa03da8666 and I already sent u a pull request. Plz let me know what you think. Thanks.
        Hide
        julianhyde Julian Hyde added a comment -

        I reviewed your changes. I tidied up by gathering the code into a method, and using a multimap to defer detecting duplicates. I also added a test case. The test case shows that if you have a non-equi constraint, say 'mgr is null' then it prevents any constant reduction occurring. That is not OK. Constraints which are not 'column = constant' (and that includes constraints 'column = constant1 and column = constant2') should be ignored, but they should not prevent constant reduction.

        Can you build on the work in https://github.com/julianhyde/incubator-calcite/tree/935-reduce-multiple-constraints and make sure that testReduceConstantsDup2 reduces empno to 10 and ignores the constraints on deptno and mgr. (If you reduce mgr to null I wouldn't object.)

        Show
        julianhyde Julian Hyde added a comment - I reviewed your changes. I tidied up by gathering the code into a method, and using a multimap to defer detecting duplicates. I also added a test case. The test case shows that if you have a non-equi constraint, say 'mgr is null' then it prevents any constant reduction occurring. That is not OK. Constraints which are not 'column = constant' (and that includes constraints 'column = constant1 and column = constant2') should be ignored, but they should not prevent constant reduction. Can you build on the work in https://github.com/julianhyde/incubator-calcite/tree/935-reduce-multiple-constraints and make sure that testReduceConstantsDup2 reduces empno to 10 and ignores the constraints on deptno and mgr. (If you reduce mgr to null I wouldn't object.)
        Hide
        pxiong Pengcheng Xiong added a comment -

        Julian Hyde, could you please take a look? Thanks.

        Show
        pxiong Pengcheng Xiong added a comment - Julian Hyde , could you please take a look? Thanks.
        Hide
        pxiong Pengcheng Xiong added a comment -

        I plan to submit a patch myself soon.

        Show
        pxiong Pengcheng Xiong added a comment - I plan to submit a patch myself soon.

          People

          • Assignee:
            pxiong Pengcheng Xiong
            Reporter:
            pxiong Pengcheng Xiong
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development