[GitHub] drcrallen commented on a change in pull request #6219: Add optional `name` to top level of FilteredAggregatorFactory

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

drcrallen commented on a change in pull request #6219: Add optional `name` to top level of FilteredAggregatorFactory
URL: https://github.com/apache/incubator-druid/pull/6219#discussion_r212738542
 
 

 ##########
 File path: processing/src/main/java/io/druid/query/aggregation/FilteredAggregatorFactory.java
 ##########
 @@ -112,7 +127,11 @@ public Object finalizeComputation(Object object)
   @Override
   public String getName()
   {
-    return delegate.getName();
+    String name = delegate.getName();
+    if (Strings.isNullOrEmpty(name)) {
+      name = this.name;
+    }
+    return name;
 
 Review comment:
   for my particular use case it is fine to flip the default around. My concern was just that it is slightly less backwards compatible since if someone *had* defined a `name` field (even if it wasn't used), the proposed in this PR way would retain prior behavior. The only way to get the behavior in this PR is to add the `name` at the top level *AND* remove the `name` from the delegate.
   
   I'm ok with making it more clear by using the `name` at the top level first, as long as folks are ok with the slightly unexpected behavior change that probably-will-not-be-in-the-wild

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