Details
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
- blocks
-
CALCITE-1356
Release Calcite 1.9.0
-
- Closed
-
- breaks
-
CALCITE-1577
Druid adapter: Incorrect result - limit on timestamp disappears
-
- Closed
-
- is related to
-
HIVE-14466
Extend Calcite capabilities to transform plan into Druid query
-
- Resolved
-
- relates to
-
CALCITE-1121
Druid adapter
-
- Closed
-
Activity
- All
- Comments
- Work Log
- History
- Activity
- Transitions
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?
Julian Hyde, thanks for the feedback. I updated the PR. Follow-up on timezone over there; I think new commit should address your comments.
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.
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"));.
That sounds about right. You could check in that fix if you have time, or I'll do it tonight.
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.
Resolved in release 1.9.0 (2016-09-22)
Julian Hyde, PR for
CALCITE-1357andCALCITE-1358is in https://github.com/apache/calcite/pull/280 . Could you review it? Thanks