re-factor top-metrics to use a generic multi value output interface#71903
re-factor top-metrics to use a generic multi value output interface#71903hendrikmuhs merged 9 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
nik9000
left a comment
There was a problem hiding this comment.
I think this is better than continuing to implement NumericMetricsAggregation.MultiValue. I have some comments around specific bits but I really like it.
server/src/main/java/org/elasticsearch/search/aggregations/metrics/MultiValueAggregation.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Do you need this one? It feels fairly top-metrics specific.
There was a problem hiding this comment.
yes, I will need this for transform later. I basically want to know the size before iterating over the values. I could instead make getValuesAsStrings returning a List, but I like Iterable more, although it is a List.
There was a problem hiding this comment.
There is something against size: it basically says that all values have that size. That might not be true and/or limit us in future.
WDYT about removing and changing getValuesAsStrings to return List?
There was a problem hiding this comment.
There is something against
size: it basically says that all values have that size. That might not be true and/or limit us in future.
Yeah, that is the problem I had with size. We could make a maxSize if that's something that you'd need. But getting the results as a list seems sensible to me.
There was a problem hiding this comment.
This "InternalX" class and "X" interface has existed for a long time for the transport client. But with it deprecated I think we can do away with the "X" interface.
There was a problem hiding this comment.
Thinking out loud, as much as I'd like to remove these now we probably should keep making them until we're ready to change all of them....
...c/main/java/org/elasticsearch/search/aggregations/metrics/InternalMultiValueAggregation.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/elasticsearch/search/aggregations/metrics/InternalMultiValueAggregation.java
Outdated
Show resolved
Hide resolved
aggregation results
b217d96 to
9c8d13c
Compare
…71903) re-factor top-metrics to return generic(non-numeric) multi value aggregation results
re-factor top-metrics to return generic(non-numeric) multi value
aggregation results
relates #71850
Motivation: top metrics implements
InternalNumericMetricsAggregation.MultiValue, however is not limited to returning numeric, but it can return an arbitrary type.However, I kept the the numeric dependency on the builder side, which seems to be valid as sorting is based on it.
A big benefit of this change: The former multi-value inteface was limited to return the top 1, this interface will make it possible to return top-N.
/CC @nik9000