[GitHub] [druid] gianm commented on a change in pull request #9638: refactor internal type system

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

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



##########
File path: processing/src/main/java/org/apache/druid/query/aggregation/AggregatorFactory.java
##########
@@ -214,19 +216,42 @@ public AggregatorFactory getMergingFactory(AggregatorFactory other) throws Aggre
    * {@link #deserialize} and the type accepted by {@link #combine}. However, it is *not* necessarily the same type
    * returned by {@link #finalizeComputation}.
    *
-   * If the type is complex (i.e. not a simple, numeric {@link org.apache.druid.segment.column.ValueType}) then there
+   * If the type is complex (i.e. not a simple, numeric {@link ValueType}) then there
    * must be a corresponding {@link org.apache.druid.segment.serde.ComplexMetricSerde} which was registered with
    * {@link org.apache.druid.segment.serde.ComplexMetrics#registerSerde} using this type name.
    *
-   * If you need a ValueType enum corresponding to this aggregator, a good way to do that is:
-   *
-   * <pre>
-   *   Optional.ofNullable(GuavaUtils.getEnumIfPresent(ValueType.class, aggregator.getTypeName()))
-   *           .orElse(ValueType.COMPLEX);
-   * </pre>
+   * If you need a ValueType enum corresponding to this aggregator, use {@link #getTypeName} instead.
    */
   public abstract String getTypeName();
 
+  /**
+   * Get the type name for the 'finalized' type for this aggregator, i.e. the type of the value returned by
+   * {@link #finalizeComputation}. This may be the same as or different than the types expected in {@link #deserialize}
+   * and {@link #combine}.
+   *
+   * If you need a ValueType enum corresponding to this aggregator, use {@link #getFinalizedType} instead.
+   */
+  public String getFinalizedTypeName()
+  {
+    return getTypeName();

Review comment:
       Fwiw, I'm in favor of requiring aggregators to explicitly implement these functions — especially if 90% of people would be fine with the default! That means it would be really easy for the 10% that need to override it to forget to do so. In general it's good to make it easy to do things right, not easy to forget important stuff.




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