[GitHub] [druid] clintropolis commented on a change in pull request #10390: Vectorized variance aggregators

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

clintropolis commented on a change in pull request #10390:
URL: https://github.com/apache/druid/pull/10390#discussion_r488175527



##########
File path: extensions-core/stats/src/main/java/org/apache/druid/query/aggregation/variance/VarianceAggregatorFactory.java
##########
@@ -162,10 +167,36 @@ public BufferAggregator factorizeBuffered(ColumnSelectorFactory metricFactory)
     throw new IAE(
         "Incompatible type for metric[%s], expected a float, double, long, or variance, but got a %s",
         fieldName,
-        inputType
+        type
     );
   }
 
+  @Override
+  public VectorAggregator factorizeVector(VectorColumnSelectorFactory selectorFactory)
+  {
+    final String type = getTypeString(selectorFactory);
+    if (ValueType.FLOAT.name().equalsIgnoreCase(type)) {

Review comment:
       Oh, this isn't actually correct. This method is backed by `getType`, which means `VARIANCE_TYPE_NAME.equalsIgnoreCase(type)` will never be true.  I suggest instead of using `getTypeString` to get the column capabilities, and then this enum can switch on `getType` directly instead of converting them to and from strings, and if type is `ValueType.COMPLEX` then to treat it as the object aggregator factory.
   
   After #10277 goes in, then we can check `getComplexTypeName` matches `VARIANCE_TYPE_NAME`.




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