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

Add TIME/TIMESTAMP/DATE datatype handling to RexImplicationChecker

    Details

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

      Description

      In RexImplicationChecker, we support a lot of operators and types, but it looks like time/timestamp/date datatypes are not supported. For example:

      select hiredate from emp where hiredate IN [TIMESTAMP '2017-01-01 10:11:12', TIMESTAMP '2015-01-01 10:11:12']
      

      should imply

      select hiredate from emp where hiredate <= TIMESTAMP '2017-12-01 01:02:03'
      

      since both timestamps in the first query's filter condition is stronger than the second query's filter condition.

      In RexImplicationCheckerTest, there are tests for each of the aforementioned datatypes, but are disabled.

      RexImplicationCheckerTest.testSimpleDate
      RexImplicationCheckerTest.testSimpleTime
      RexImplicationCheckerTest.testSimpleTimestamp
      

      Running these ignored tests, I get the following error messages.

      2017-06-02 17:13:10,028 [main] WARN  - Exception thrown while checking if => >=($8, 2017-06-03): java.sql.Date cannot be cast to java.lang.Integer
      2017-06-02 17:13:10,075 [main] WARN  - Exception thrown while checking if => <=($9, 00:13:10): java.util.GregorianCalendar cannot be cast to java.lang.Long
      2017-06-02 17:13:10,100 [main] WARN  - Exception thrown while checking if => <=($9, 2017-06-03 00:13:10): java.util.GregorianCalendar cannot be cast to java.lang.Long
      

        Activity

        Hide
        julianhyde Julian Hyde added a comment -

        Am I correct that this feature would affect what SQL expressions are valid? If so, I think you should explain it in SQL terms.

        Show
        julianhyde Julian Hyde added a comment - Am I correct that this feature would affect what SQL expressions are valid? If so, I think you should explain it in SQL terms.
        Hide
        minjikim MinJi Kim added a comment -

        Okay, let me go find an example and update the description to be more meaningful. Thanks for the quick response, Julian Hyde!

        Show
        minjikim MinJi Kim added a comment - Okay, let me go find an example and update the description to be more meaningful. Thanks for the quick response, Julian Hyde !
        Hide
        julianhyde Julian Hyde added a comment -

        By the way, I am going to give you a hard time if you assume that 1970-01-01 = 0. The way SQL date/time support is constructed, there is no preferred date that would serve as "zero".

        Show
        julianhyde Julian Hyde added a comment - By the way, I am going to give you a hard time if you assume that 1970-01-01 = 0. The way SQL date/time support is constructed, there is no preferred date that would serve as "zero".
        Hide
        minjikim MinJi Kim added a comment -

        Julian Hyde Let me know if this looks okay. Thanks!

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

        Show
        minjikim MinJi Kim added a comment - Julian Hyde Let me know if this looks okay. Thanks! https://github.com/apache/calcite/pull/467
        Hide
        julianhyde Julian Hyde added a comment -

        Basically good, a couple of comments:

        • What's the purpose of the SqlToRelConverterTest case you added? It doesn't call the logic you added, AFAICT.
        • The flow control in VisitorDataContext was awful, and you made it a bit better by adding 'break' statements (thanks!). But you can make it much better still by using the new method RexLiteral.getValueAs throughout. Each case will become a one-liner:
            case DATE:
              return Pair.of(index, ((RexLiteral) literal).getValueAs(DateString.class).getDaysSinceEpoch());
          
        Show
        julianhyde Julian Hyde added a comment - Basically good, a couple of comments: What's the purpose of the SqlToRelConverterTest case you added? It doesn't call the logic you added, AFAICT. The flow control in VisitorDataContext was awful, and you made it a bit better by adding 'break' statements (thanks!). But you can make it much better still by using the new method RexLiteral.getValueAs throughout. Each case will become a one-liner: case DATE: return Pair.of(index, ((RexLiteral) literal).getValueAs(DateString.class).getDaysSinceEpoch());
        Hide
        minjikim MinJi Kim added a comment - - edited

        Thanks for the review, Julian Hyde. I will fix the VisitorDataContext as you suggested and update the pull request later today. I didn't know about the RexLiteral.getValueAs(), that's really nice!

        The test for SqlToRelConverterTest was added while I was just testing the changes in VisitorDataContext, and I wanted to see if subquery conversion will go through this code for timestamps. As you said it doesn't go through the VisitorDataContext since we generate ISNULL operator which has special handling. I will remove it.

        Show
        minjikim MinJi Kim added a comment - - edited Thanks for the review, Julian Hyde . I will fix the VisitorDataContext as you suggested and update the pull request later today. I didn't know about the RexLiteral.getValueAs(), that's really nice! The test for SqlToRelConverterTest was added while I was just testing the changes in VisitorDataContext, and I wanted to see if subquery conversion will go through this code for timestamps. As you said it doesn't go through the VisitorDataContext since we generate ISNULL operator which has special handling. I will remove it.
        Hide
        minjikim MinJi Kim added a comment -

        Julian Hyde I updated the PR with the changes. If you can take a look and let me know of any comments/feedback, it would be great! Thanks!

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

        Show
        minjikim MinJi Kim added a comment - Julian Hyde I updated the PR with the changes. If you can take a look and let me know of any comments/feedback, it would be great! Thanks! https://github.com/apache/calcite/pull/467
        Hide
        julianhyde Julian Hyde added a comment -

        Is there a valid reason to catch AssertionError in getValue? If an AssertionError is thrown, there is a bug in Calcite code – someone has tried to create an invalid RexLiteral, and we let them – and we should not mask that.

        Show
        julianhyde Julian Hyde added a comment - Is there a valid reason to catch AssertionError in getValue? If an AssertionError is thrown, there is a bug in Calcite code – someone has tried to create an invalid RexLiteral, and we let them – and we should not mask that.
        Hide
        minjikim MinJi Kim added a comment -

        No, not really. I was worried that there might be cases that VisitorDataContext.getValue() or RexLiteral.getValueAs() weren't handling. It might be better to fail and make sure that the RexLiteral is valid and/or we are properly handling all data type in VisitorDataContext, rather than silently return null. I will fix that. Thanks!

        Show
        minjikim MinJi Kim added a comment - No, not really. I was worried that there might be cases that VisitorDataContext.getValue() or RexLiteral.getValueAs() weren't handling. It might be better to fail and make sure that the RexLiteral is valid and/or we are properly handling all data type in VisitorDataContext, rather than silently return null. I will fix that. Thanks!
        Hide
        minjikim MinJi Kim added a comment -

        I updated the PR. Please take a look, thanks again!

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

        Show
        minjikim MinJi Kim added a comment - I updated the PR. Please take a look, thanks again! https://github.com/apache/calcite/pull/467
        Show
        julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/419b810f .
        Hide
        jcamachorodriguez Jesus Camacho Rodriguez added a comment -

        Resolved in release 1.13.0 (2017-06-26).

        Show
        jcamachorodriguez Jesus Camacho Rodriguez added a comment - Resolved in release 1.13.0 (2017-06-26).

          People

          • Assignee:
            julianhyde Julian Hyde
            Reporter:
            minjikim MinJi Kim
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development