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:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
