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

Recognize Druid Timeseries and TopN queries in DruidQuery

    Details

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

      Description

      Port the work done in HIVE-14217 to recognize Timeseries and TopN queries to Calcite.

      This includes a rule to push SortLimit into DruidQuery, which can lead to creating TopN queries. This rule can help to push sorting and limit into GroupBy queries, and limit to Select queries.

        Issue Links

          Activity

          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Resolved in release 1.9.0 (2016-09-22)

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Resolved in release 1.9.0 (2016-09-22)
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/962eac5 .

          Thanks for the review Julian Hyde!

          Still thinking about your comment around user vs Druid timezone... I will update the issue with some comments shortly.

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/962eac5 . Thanks for the review Julian Hyde ! Still thinking about your comment around user vs Druid timezone... I will update the issue with some comments shortly.
          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/63a7a1a .
          Hide
          julianhyde Julian Hyde added a comment -

          That sounds about right. You could check in that fix if you have time, or I'll do it tonight.

          Show
          julianhyde Julian Hyde added a comment - That sounds about right. You could check in that fix if you have time, or I'll do it tonight.
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Yes, I agree changing timezone for the tests is not the solution, I only realized about the complete problem when I checked it into more detail.

          IMO the problem is in l253 in DateRangeRules.java. Instead of Calendar.getInstance(); which is user timezone dependent, we should use Calendar.getInstance(TimeZone.getTimeZone("UTC"));.

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Yes, I agree changing timezone for the tests is not the solution, I only realized about the complete problem when I checked it into more detail. IMO the problem is in l253 in DateRangeRules.java. Instead of Calendar.getInstance(); which is user timezone dependent, we should use Calendar.getInstance(TimeZone.getTimeZone("UTC")); .
          Hide
          julianhyde Julian Hyde added a comment -

          Regarding the failures when the tests are run in BST (and, I think, anything east of UTC... I have a nightly test that runs in Moscow time that has been failing) - these are my fault. I should fix them. I'm not sure what the fix is, but it shouldn't be to set the JVM's timezone to UTC. As a library, we can't guarantee that. It's an existing problem, so you shouldn't attempt to fix in this PR. I'll fix it shortly.

          Show
          julianhyde Julian Hyde added a comment - Regarding the failures when the tests are run in BST (and, I think, anything east of UTC... I have a nightly test that runs in Moscow time that has been failing) - these are my fault. I should fix them. I'm not sure what the fix is, but it shouldn't be to set the JVM's timezone to UTC. As a library, we can't guarantee that. It's an existing problem, so you shouldn't attempt to fix in this PR. I'll fix it shortly.
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Julian Hyde, thanks for the feedback. I updated the PR. Follow-up on timezone over there; I think new commit should address your comments.

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Julian Hyde , thanks for the feedback. I updated the PR. Follow-up on timezone over there; I think new commit should address your comments.
          Hide
          julianhyde Julian Hyde added a comment -

          I've left some review comments.

          But there's a design issue that doesn't fit a precise part of the code, so let's discuss here. In SQL (certainly in Calcite, I don't know about Hive), datetime values have no timezone. I don't know what Druid does, but I suspect that it assumes that all values are UTC.

          Using Joda terminology: SQL datetimes correspond to Joda LocalDateTime, whereas Druid is based upon Instant and Interval.

          Have you thought about what should happen if Calcite and Druid are in different time zones?

          Show
          julianhyde Julian Hyde added a comment - I've left some review comments. But there's a design issue that doesn't fit a precise part of the code, so let's discuss here. In SQL (certainly in Calcite, I don't know about Hive), datetime values have no timezone. I don't know what Druid does, but I suspect that it assumes that all values are UTC. Using Joda terminology: SQL datetimes correspond to Joda LocalDateTime , whereas Druid is based upon Instant and Interval . Have you thought about what should happen if Calcite and Druid are in different time zones?
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Julian Hyde, PR for CALCITE-1357 and CALCITE-1358 is in https://github.com/apache/calcite/pull/280 . Could you review it? Thanks

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Julian Hyde , PR for CALCITE-1357 and CALCITE-1358 is in https://github.com/apache/calcite/pull/280 . Could you review it? Thanks

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development