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 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@himanshug
Copy link
Contributor

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

Copy link
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should name be in toString/equals/hashCode?

@drcrallen
Copy link
Contributor Author

@clintropolis whoops, yep, will fix

@jon-wei
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
@drcrallen drcrallen deleted the filtered/name branch October 30, 2018 17:40
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants