[GitHub] leventov commented on a change in pull request #6220: Fix four bugs with numeric dimension output types.

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

leventov commented on a change in pull request #6220: Fix four bugs with numeric dimension output types.
URL: https://github.com/apache/incubator-druid/pull/6220#discussion_r212519103
 
 

 ##########
 File path: processing/src/main/java/io/druid/query/topn/types/TopNColumnSelectorStrategyFactory.java
 ##########
 @@ -27,23 +28,39 @@
 
 public class TopNColumnSelectorStrategyFactory implements ColumnSelectorStrategyFactory<TopNColumnSelectorStrategy>
 {
+  private final ValueType dimensionType;
+
+  public TopNColumnSelectorStrategyFactory(final ValueType dimensionType)
+  {
+    this.dimensionType = Preconditions.checkNotNull(dimensionType, "dimensionType");
+  }
+
   @Override
   public TopNColumnSelectorStrategy makeColumnSelectorStrategy(
       ColumnCapabilities capabilities, ColumnValueSelector selector
   )
   {
-    ValueType type = capabilities.getType();
-    switch (type) {
+    final ValueType selectorType = capabilities.getType();
+
+    switch (selectorType) {
       case STRING:
-        return new StringTopNColumnSelectorStrategy();
+        // Return strategy that reads strings and outputs dimensionTypes.
+        return new StringTopNColumnSelectorStrategy(dimensionType);
       case LONG:
-        return new NumericTopNColumnSelectorStrategy.OfLong();
       case FLOAT:
-        return new NumericTopNColumnSelectorStrategy.OfFloat();
       case DOUBLE:
-        return new NumericTopNColumnSelectorStrategy.OfDouble();
+        if (ValueType.isNumeric(dimensionType)) {
+          // Return strategy that aggregates using the _output_ type, because this allows us to collapse values
+          // properly (numeric types cannot represent all values of other numeric types).
+          return NumericTopNColumnSelectorStrategy.ofType(dimensionType, dimensionType);
+        } else {
+          // Return strategy that aggregates using the _input_ type. Here we are assuming that the output type can
 
 Review comment:
   Despite those comments, it's still hard to understand what is going on here. Maybe you could try to clarify it more?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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