gianm commented on a change in pull request #10968:
URL: https://github.com/apache/druid/pull/10968#discussion_r590747307
##########
File path:
processing/src/main/java/org/apache/druid/query/groupby/GroupByQuery.java
##########
@@ -715,7 +716,11 @@ private DateTime computeUniversalTimestamp()
if (!timestampStringFromContext.isEmpty()) {
return DateTimes.utc(Long.parseLong(timestampStringFromContext));
} else if (Granularities.ALL.equals(granularity)) {
- final DateTime timeStart = getIntervals().get(0).getStart();
+ final List<Interval> intervals = getIntervals();
+ if (CollectionUtils.isNullOrEmpty(intervals)) {
+ return null;
Review comment:
This just struck my eye because it doesn't agree with the javadoc for
getUniversalTimestamp, which says:
> * If this query has a single universal timestamp, return it. Otherwise
return null.
> *
> * This method will return a nonnull timestamp in the following two cases:
> *
> * 1) CTX_KEY_FUDGE_TIMESTAMP is set (in which case this timestamp will
be returned).
> * 2) Granularity is "ALL".
Here, granularity is ALL, but null is still returned.
I guess the idea is that it doesn't matter what is returned in this case,
because there aren't going to be any result rows anyway. That's OK if so, just
please update the getUniversalTimestamp accordingly.
Btw, I don't think there's a reason to use `CollectionUtils.isNullOrEmpty`,
because I don't think `getIntervals` can return null.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
