Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add optional `name` to top level of FilteredAggregatorFactory #6219

Merged
merged 9 commits into from Oct 11, 2018

Conversation

@drcrallen
Copy link
Contributor

drcrallen commented Aug 22, 2018

The FilteredAggregatorFactory behavior is kind of odd in the sense that the resulting name for the aggregation is not set at the top level, but rather is set by the delegate aggregation. This makes any query planning or auto-generation items have a need to special case the FilteredAggregatorFactory.

This PR adds a name field to FilteredAggregatorFactory which is used in preference to the delegate aggregator factory's name.

if (Strings.isNullOrEmpty(name)) {
name = this.name;
}
return name;

This comment has been minimized.

Copy link
@nishantmonu51

nishantmonu51 Aug 23, 2018

Member

It would be wierd to ignore the name set in FilteredAggregator when user explicitly sets it.

IMO, the default should be - If user has set a name for FilteredAggregatorFactory - use that, otherwise use the name of the delegate. For backwards compatible case user would not have specified name at filtered aggregator level and will get the old behavior of getting delegate name.

This comment has been minimized.

Copy link
@himanshug

himanshug Aug 23, 2018

Contributor

seems like unintentional because this defeats the purpose in PR description :)

This comment has been minimized.

Copy link
@drcrallen

drcrallen Aug 24, 2018

Author Contributor

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 comment has been minimized.

Copy link
@drcrallen

drcrallen Aug 27, 2018

Author Contributor
@himanshug

This comment has been minimized.

Copy link
Contributor

himanshug commented Aug 28, 2018

@drcrallen I added the compatibility label to this PR for the catch that you mentioned. It should be covered in the release notes.

@clintropolis
Copy link
Member

clintropolis left a comment

Should name be in toString/equals/hashCode?

@drcrallen

This comment has been minimized.

Copy link
Contributor Author

drcrallen commented Aug 30, 2018

@clintropolis whoops, yep, will fix

@jon-wei

This comment has been minimized.

Copy link
Contributor

jon-wei commented Sep 14, 2018

@nishantmonu51 Did you have more comments on this?

@jon-wei jon-wei added this to the 0.13.0 milestone Oct 11, 2018

@jon-wei jon-wei merged commit c55b37d into apache:master Oct 11, 2018

2 checks passed

Inspections: pull requests (Druid) TeamCity build finished
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@drcrallen drcrallen deleted the drcrallen:filtered/name branch Oct 30, 2018

@jon-wei jon-wei removed their assignment Nov 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.