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

Simplify "1 = 1" to true, "1 > 2" to false

    Details

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

      Description

      The RexUtil.simplify method should handle literal comparison simplification. From an email on the calcite-dev mailing list:

      Calcite doesn't seem to optimize away the literal comparison literal case
      with RexUtil.simplify. In my understanding any literal comparison literal
      results in a simple TRUE/FALSE result.

      I'm not sure this is valid in the general case, but I put together a simple
      example of doing this on the RexUtil simplifyCall.

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

        Issue Links

          Activity

          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.12.0 (2017-03-24).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.12.0 (2017-03-24).
          Hide
          risdenk Kevin Risden added a comment -

          Awesome thanks Julian Hyde

          Show
          risdenk Kevin Risden added a comment - Awesome thanks Julian Hyde
          Hide
          julianhyde Julian Hyde added a comment -

          Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/255cd96a. Kevin Risden, Thanks for your PR! I refactored your logic a little, combining it with the logic added for CALCITE-1638.

          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/255cd96a . Kevin Risden , Thanks for your PR! I refactored your logic a little, combining it with the logic added for CALCITE-1638 .
          Hide
          risdenk Kevin Risden added a comment -

          Julian Hyde - I created a new PR with the updated implementation and a test in RexProgramTest like you suggested.

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

          Show
          risdenk Kevin Risden added a comment - Julian Hyde - I created a new PR with the updated implementation and a test in RexProgramTest like you suggested. https://github.com/apache/calcite/pull/387
          Hide
          risdenk Kevin Risden added a comment -

          Ohhh now that you say that. I think I know what I did wrong previously with the PR. I thought I was falling through (not simplifying) when things didn't work/match. I need to be more careful about the comparison and check the type as well as the value before proceeding.

          Show
          risdenk Kevin Risden added a comment - Ohhh now that you say that. I think I know what I did wrong previously with the PR. I thought I was falling through (not simplifying) when things didn't work/match. I need to be more careful about the comparison and check the type as well as the value before proceeding.
          Hide
          julianhyde Julian Hyde added a comment -

          And should we also recognize '1' = 1, or '1' = true? I don't think so. There are implicit casts involved, and casts are non-trivial.

          Show
          julianhyde Julian Hyde added a comment - And should we also recognize '1' = 1 , or '1' = true ? I don't think so. There are implicit casts involved, and casts are non-trivial.
          Hide
          risdenk Kevin Risden added a comment -

          Since the list I haven't updated the PR. I ran into the following issue 1.0 = 1 should be true but it is not with the change from the existing PR. I haven't had a chance to go back and review it further.

          Show
          risdenk Kevin Risden added a comment - Since the list I haven't updated the PR. I ran into the following issue 1.0 = 1 should be true but it is not with the change from the existing PR. I haven't had a chance to go back and review it further.
          Hide
          risdenk Kevin Risden added a comment -

          Another comment from Julian Hyde

          Also, if you add a test — probably a few extra lines in RexProgramTest will suffice — I will accept your PR. I think it will be efficient enough.

          Show
          risdenk Kevin Risden added a comment - Another comment from Julian Hyde Also, if you add a test — probably a few extra lines in RexProgramTest will suffice — I will accept your PR. I think it will be efficient enough.
          Hide
          risdenk Kevin Risden added a comment -

          From Julian Hyde:

          We have support for this in planner rules – I’m pretty sure that ReduceExpressionsRule.FILTER_INSTANCE will convert ‘where 1 = 0’ to ‘where false’, then PruneEmptyRules.FILTER_INSTANCE will make the Filter disappear altogether — but arguably it could happen in RexUtil.simplify also.

          The purpose of RexUtil.simplify is to simplify (only) patterns that are commonly occurring, easy to recognize, and will produce a quick win in terms of the size of the RelNode/RexNode tree. I don’t know yet whether this passes that threshold. Can you log a JIRA case for this and we can discuss further?

          By the way, CALCITE-1638 is related. It changed the result of a test that was doing ‘where 1 = 1’.

          Show
          risdenk Kevin Risden added a comment - From Julian Hyde : We have support for this in planner rules – I’m pretty sure that ReduceExpressionsRule.FILTER_INSTANCE will convert ‘where 1 = 0’ to ‘where false’, then PruneEmptyRules.FILTER_INSTANCE will make the Filter disappear altogether — but arguably it could happen in RexUtil.simplify also. The purpose of RexUtil.simplify is to simplify (only) patterns that are commonly occurring, easy to recognize, and will produce a quick win in terms of the size of the RelNode/RexNode tree. I don’t know yet whether this passes that threshold. Can you log a JIRA case for this and we can discuss further? By the way, CALCITE-1638 is related. It changed the result of a test that was doing ‘where 1 = 1’.

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              risdenk Kevin Risden
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development