[GitHub] [druid] gianm commented on a change in pull request #10325: fix NPE in StringGroupByColumnSelectorStrategy#bufferComparator

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

gianm commented on a change in pull request #10325:
URL: https://github.com/apache/druid/pull/10325#discussion_r483100071



##########
File path: processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/column/StringGroupByColumnSelectorStrategy.java
##########
@@ -157,8 +157,8 @@ private void initializeGroupingKeyV2Dimension(
         capabilities != null &&
         capabilities.hasBitmapIndexes() &&
         capabilities.areDictionaryValuesSorted().and(capabilities.areDictionaryValuesUnique()).isTrue();
-
-    if (canCompareInts && (stringComparator == null || StringComparators.LEXICOGRAPHIC.equals(stringComparator))) {
+    final StringComparator comparator = stringComparator == null ? StringComparators.LEXICOGRAPHIC : stringComparator;

Review comment:
       Could you add some javadocs to this method explaining what it does in general, and also specifically what it does when `stringComparator` is null?
   
   Also, do you think this NPE could have got triggered by a real query? If so, is defaulting to lexicographic the right behavior for that query?




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