[GitHub] [incubator-druid] jihoonson commented on a change in pull request #7624: Fix resultLevelCache for timeseries with grandTotal

[GitBox <gi...@apache.org>]

jihoonson commented on a change in pull request #7624: Fix resultLevelCache for timeseries with grandTotal
URL: https://github.com/apache/incubator-druid/pull/7624#discussion_r282638505
 
 

 ##########
 File path: processing/src/main/java/org/apache/druid/query/Result.java
 ##########
 @@ -51,10 +54,20 @@ public Result(@JsonProperty("timestamp") DateTime timestamp, @JsonProperty("resu
   @Override
   public int compareTo(Result<T> tResult)
   {
-    return timestamp.compareTo(tResult.timestamp);
+    // timestamp is null for grandTotal which should come last.
+    if (timestamp == null && tResult.timestamp == null) {
+      return 0;
+    } else if (timestamp == null) {
+      return 1;
 
 Review comment:
   Hmm, honestly, this doesn't affect to the order of grandTotal. It's computed in brokers and added at the last of the result regardless of the `descending` flag.
   
   But, I changed this part to handle null timestamp correctly for any use case in the future. Null means unknown value and thus comparing non-nulls with nulls is not defined. Usually other systems provides some options to place nulls at first (NULLS FIRST) or at last (NULLS LAST). We don't have such options yet, and so I thought it would be better to place nulls at last in natural order because the row with null timestamp would be grandTotal. What do you think?

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org