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

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

suneet-s commented on a change in pull request #10325:
URL: https://github.com/apache/druid/pull/10325#discussion_r483290837



##########
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:
       It looks like it can if the OrderBy is null. See `GroupByEngineKeySerde#getDimensionComparators` which calls `DefaultLimitSpec#getComparatorForDimName` which can return null.
   
   Maybe it makes more sense for the default to be something faster since we can't use the indexes in this case, something like alphanumeric or strlen would be faster.
   
   I haven't seen this triggered in a real query yet, but I think I've seen the lexicographic comparator used as a default in other parts of the query stack (`BoundDimFilter` and `DimensionTopNMetricSpec`)




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