Fix resultLevelCache for timeseries with grandTotal #7624
Conversation
| if (timestamp == null && tResult.timestamp == null) { | ||
| return 0; | ||
| } else if (timestamp == null) { | ||
| return 1; |
There was a problem hiding this comment.
should it be -1, if timestamp is null, assuming null is less than tResult.timestamp(which is supposedly non-null) ?
There was a problem hiding this 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?
There was a problem hiding this comment.
Thanks for explaining, if you are placing nulls at last, then this is fine. Although, it might be more clear if you use Comparator.nullsLast
| } else if (timestamp == null) { | ||
| return 1; | ||
| } else if (tResult == null) { | ||
| return -1; |
…l-result-level-cache
* Fix resultLevelCache for timeseries with grandTotal * Address comment * fix test
* Fix resultLevelCache for timeseries with grandTotal * Address comment * fix test
* Fix resultLevelCache for timeseries with grandTotal * Address comment * fix test
…ache#7637) * Fix resultLevelCache for timeseries with grandTotal * Address comment * fix test
* Fix resultLevelCache for timeseries with grandTotal * Address comment * fix test
Fixes #7621.