-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Conversation
if (Strings.isNullOrEmpty(name)) { | ||
name = this.name; | ||
} | ||
return name; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nishantmonu51 fixed
@drcrallen I added the |
There was a problem hiding this 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
?
@clintropolis whoops, yep, will fix |
@nishantmonu51 Did you have more comments on this? |
The
FilteredAggregatorFactory
behavior is kind of odd in the sense that the resultingname
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 theFilteredAggregatorFactory
.This PR adds a
name
field toFilteredAggregatorFactory
which is used in preference to the delegate aggregator factory'sname
.