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

Use consistent types when processing ranges

Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • None
    • 1.21.0
    • None

    Description

      Range expressions like <ts> = 'literal' AND <ts> < 'literal' trigger ClassCastException as literal are implicitly casted differently between =/<> operators and other comparison operators. Apply the same casting rules for comparison to =/<> calls when processing ranges, so that all the terms have the same type for literals.

      Attachments

        Issue Links

          Activity

            Hi siddteotia, it seems you are describing a bug. Can you change the summary /description to better explain how and when it appears. If you have an SQL statement that fails it would be nice to include it also.

            zabetak Stamatis Zampetakis added a comment - Hi siddteotia , it seems you are describing a bug. Can you change the summary /description to better explain how and when it appears. If you have an SQL statement that fails it would be nice to include it also.
            Juhwan Juhwan Kim added a comment -

            Hello zabetak, I hit this error when I have filter like this:

             "WHERE d = '2018-01-01 01:23:45' AND d > '2018-01-01 01:23:45'".

            First one is converted to NlsString while the second one is converted to TimestampString. So, CastException is thrown while simplifying the range. 

            I posted a PR for this(https://github.com/apache/calcite/pull/1131). Could you review the change? 

            Juhwan Juhwan Kim added a comment - Hello  zabetak , I hit this error when I have filter like this:  "WHERE d = '2018-01-01 01:23:45' AND d > '2018-01-01 01:23:45'". First one is converted to NlsString while the second one is converted to TimestampString. So, CastException is thrown while simplifying the range.  I posted a PR for this( https://github.com/apache/calcite/pull/1131 ). Could you review the change? 

            Juhwan I understand that this issue surfaces during simplification; but I feel that the fix shouldn't reside in "simplifyComparision" as I would guess that the issue might arise elsewhere as well..
            instead; I would expect that this should be fixed where the "comparison" node is constructed (right now I would guess some RexBuilder part ; but I could be easily wrong)

            kgyrtkirk Zoltan Haindrich added a comment - Juhwan I understand that this issue surfaces during simplification; but I feel that the fix shouldn't reside in "simplifyComparision" as I would guess that the issue might arise elsewhere as well.. instead; I would expect that this should be fixed where the "comparison" node is constructed (right now I would guess some RexBuilder part ; but I could be easily wrong)

            I agree with kgyrtkirk . RexBuilder is responsible for building expressions so I would first look there, possibly adding tests in RexBuilderTest. 

            zabetak Stamatis Zampetakis added a comment - I agree with kgyrtkirk . RexBuilder is responsible for building expressions so I would first look there, possibly adding tests in RexBuilderTest. 
            Juhwan Juhwan Kim added a comment -

            Thanks for the suggestion. Initially, I enforced to give a consistent type when we create a comparison. However, since that involves simplifyCast call which calls reduce function, it seemed like doing it every time when we create a comparison gives some performance loss. "testPullUpPredicatesForExprsItr" test started failing after the change. In the recent patch, I made a new method in Comparison for that functionality and used it lazily, and it seems to be working now.

            Juhwan Juhwan Kim added a comment - Thanks for the suggestion. Initially, I enforced to give a consistent type when we create a comparison. However, since that involves simplifyCast call which calls reduce function, it seemed like doing it every time when we create a comparison gives some performance loss. "testPullUpPredicatesForExprsItr" test started failing after the change. In the recent patch, I made a new method in Comparison for that functionality and used it lazily, and it seems to be working now.
            julianhyde Julian Hyde added a comment -

            Let’s start off with a good Jira case title and commit message. 

            julianhyde Julian Hyde added a comment - Let’s start off with a good Jira case title and commit message. 
            Juhwan Juhwan Kim added a comment - - edited

            Thanks julianhyde. I accidentally closed the previous PR while resolving merge conflict. Opened a new PR (https://github.com/apache/calcite/pull/1227) and updated commit message.

            Juhwan Juhwan Kim added a comment - - edited Thanks julianhyde . I accidentally closed the previous PR while resolving merge conflict. Opened a new PR ( https://github.com/apache/calcite/pull/1227)  and updated commit message.
            danny0405 Danny Chen added a comment - - edited

            Kind of related with CALCITE-2302, maybe we should keep the implicit type coercion rules consistent and got a centralized maintenance instead of making the rules everywhere.

            danny0405 Danny Chen added a comment - - edited Kind of related with CALCITE-2302 , maybe we should keep the implicit type coercion rules consistent and got a centralized maintenance instead of making the rules everywhere.
            Juhwan Juhwan Kim added a comment -

            danny0405. I agree with your comment. Implicit type conversion would fix this issue, and I also think that it is a much better idea. I uploaded my PR with a simple test case. Do you mind adding that to your PR? Then, I think we could resolve this ticket along with CALCITE-2302.

            Juhwan Juhwan Kim added a comment - danny0405 . I agree with your comment. Implicit type conversion would fix this issue, and I also think that it is a much better idea. I uploaded my PR with a simple test case. Do you mind adding that to your PR? Then, I think we could resolve this ticket along with CALCITE-2302 .
            danny0405 Danny Chen added a comment - - edited

            Sure, the more test cases the better, i would link this issue with CALCITE-2302, thanks for your test cases.

            danny0405 Danny Chen added a comment - - edited Sure, the more test cases the better, i would link this issue with CALCITE-2302 , thanks for your test cases.
            danny0405 Danny Chen added a comment -

            Close this issue because it is contained by CALCITE-2302.

            danny0405 Danny Chen added a comment - Close this issue because it is contained by CALCITE-2302 .

            People

              danny0405 Danny Chen
              siddteotia Siddharth Teotia
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Time Tracking

                  Estimated:
                  Original Estimate - Not Specified
                  Not Specified
                  Remaining:
                  Remaining Estimate - 0h
                  0h
                  Logged:
                  Time Spent - 1h 20m
                  1h 20m