You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/08/14 02:22:15 UTC
[GitHub] [druid] gianm opened a new pull request #10279: Add SQL "OFFSET" clause.
gianm opened a new pull request #10279:
URL: https://github.com/apache/druid/pull/10279
Under the hood, this uses the new offset features from #10233 (Scan)
and #10235 (GroupBy). Since Timeseries and TopN queries do not currently
have an offset feature, SQL planning will switch from one of those to
Scan or GroupBy if users add an OFFSET.
Includes a refactoring to harmonize offset and limit planning using an
OffsetLimit wrapper class. This is useful because it ensures that the
various places that need to deal with offset and limit collapsing all
behave the same way, using its "andThen" method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10279: Add SQL "OFFSET" clause.
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10279:
URL: https://github.com/apache/druid/pull/10279#discussion_r473832613
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java
##########
@@ -762,9 +756,20 @@ public TimeseriesQuery toTimeseriesQuery()
Iterables.getOnlyElement(grouping.getDimensions()).toDimensionSpec().getOutputName()
);
if (sorting != null) {
- // If there is sorting, set timeseriesLimit to given value if less than Integer.Max_VALUE
- if (sorting.isLimited()) {
- timeseriesLimit = Ints.checkedCast(sorting.getLimit());
+ if (sorting.getOffsetLimit().hasOffset()) {
+ // Timeseries cannot handle offsets.
+ return null;
+ }
+
+ if (sorting.getOffsetLimit().hasLimit()) {
+ final long limit = sorting.getOffsetLimit().getLimit();
+
+ if (limit == 0) {
Review comment:
Would it be worth pushing a method into `OffsetLimit` to check that limit is greater than 0 to use for timeseries/group by/scan to use instead of `hasLimit`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #10279: Add SQL "OFFSET" clause.
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #10279:
URL: https://github.com/apache/druid/pull/10279#issuecomment-677854285
@clintropolis Any other comments or should we merge this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #10279: Add SQL "OFFSET" clause.
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10279:
URL: https://github.com/apache/druid/pull/10279#discussion_r474217036
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java
##########
@@ -762,9 +756,20 @@ public TimeseriesQuery toTimeseriesQuery()
Iterables.getOnlyElement(grouping.getDimensions()).toDimensionSpec().getOutputName()
);
if (sorting != null) {
- // If there is sorting, set timeseriesLimit to given value if less than Integer.Max_VALUE
- if (sorting.isLimited()) {
- timeseriesLimit = Ints.checkedCast(sorting.getLimit());
+ if (sorting.getOffsetLimit().hasOffset()) {
+ // Timeseries cannot handle offsets.
+ return null;
+ }
+
+ if (sorting.getOffsetLimit().hasLimit()) {
+ final long limit = sorting.getOffsetLimit().getLimit();
+
+ if (limit == 0) {
Review comment:
That's true; at this point, a zero should be treated as an explicit zero. The native timeseries query treats `0` and `null` equivalently and both result in an unlimited query. So we have to bail out if the limit is an explicit zero.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #10279: Add SQL "OFFSET" clause.
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10279:
URL: https://github.com/apache/druid/pull/10279#discussion_r474232782
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java
##########
@@ -897,6 +906,11 @@ public GroupByQuery toGroupByQuery()
return null;
}
+ if (sorting != null && sorting.getOffsetLimit().hasLimit() && sorting.getOffsetLimit().getLimit() <= 0) {
Review comment:
It can't, but I guess it doesn't hurt to overcheck.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10279: Add SQL "OFFSET" clause.
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10279:
URL: https://github.com/apache/druid/pull/10279#discussion_r473853234
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java
##########
@@ -762,9 +756,20 @@ public TimeseriesQuery toTimeseriesQuery()
Iterables.getOnlyElement(grouping.getDimensions()).toDimensionSpec().getOutputName()
);
if (sorting != null) {
- // If there is sorting, set timeseriesLimit to given value if less than Integer.Max_VALUE
- if (sorting.isLimited()) {
- timeseriesLimit = Ints.checkedCast(sorting.getLimit());
+ if (sorting.getOffsetLimit().hasOffset()) {
+ // Timeseries cannot handle offsets.
+ return null;
+ }
+
+ if (sorting.getOffsetLimit().hasLimit()) {
+ final long limit = sorting.getOffsetLimit().getLimit();
+
+ if (limit == 0) {
Review comment:
Ah i wasn't thinking it through, I guess you still need to check `hasLimit` for the non-zero case.
For timeseries though, it looks like `timeseriesLimit` is initialized to 0, is returning null here changing behavior? I guess a zero here is an explicit zero, meaning unlimited wasn't probably the expected response?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10279: Add SQL "OFFSET" clause.
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10279:
URL: https://github.com/apache/druid/pull/10279#discussion_r473853234
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java
##########
@@ -762,9 +756,20 @@ public TimeseriesQuery toTimeseriesQuery()
Iterables.getOnlyElement(grouping.getDimensions()).toDimensionSpec().getOutputName()
);
if (sorting != null) {
- // If there is sorting, set timeseriesLimit to given value if less than Integer.Max_VALUE
- if (sorting.isLimited()) {
- timeseriesLimit = Ints.checkedCast(sorting.getLimit());
+ if (sorting.getOffsetLimit().hasOffset()) {
+ // Timeseries cannot handle offsets.
+ return null;
+ }
+
+ if (sorting.getOffsetLimit().hasLimit()) {
+ final long limit = sorting.getOffsetLimit().getLimit();
+
+ if (limit == 0) {
Review comment:
Ah i wasn't thinking it through, I guess you still need to check `hasLimit` for the non-zero case.
For timeseries though, it looks like `timeseriesLimit` is initialized to 0, is returning null here changing behavior?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm merged pull request #10279: Add SQL "OFFSET" clause.
Posted by GitBox <gi...@apache.org>.
gianm merged pull request #10279:
URL: https://github.com/apache/druid/pull/10279
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10279: Add SQL "OFFSET" clause.
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10279:
URL: https://github.com/apache/druid/pull/10279#discussion_r474231095
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java
##########
@@ -897,6 +906,11 @@ public GroupByQuery toGroupByQuery()
return null;
}
+ if (sorting != null && sorting.getOffsetLimit().hasLimit() && sorting.getOffsetLimit().getLimit() <= 0) {
Review comment:
can limit be less than 0 here because of the preconditions in `OffsetLimit`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org