Clean up BoxplotAggregator to reuse AbstractTDigestPercentilesAggregator #53365
Conversation
Refactors BoxplotAggregator to reuse parts that are already available in AbstractTDigestPercentilesAggregator Follow up for elastic#51948
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
imotov
requested review from
nik9000 and
polyfractal
and removed request for
a team
nik9000
left a comment
nik9000
left a comment
There was a problem hiding this comment.
Neat! I haven't given it a super in depth look but as long as you aren't trying to sneak something tricky in I think I get it and I'm all for it.
|
I was just about to LGTM as well, but remembered the histo field in VS branch (and thus soon master) will be changing: #53298 E.g. the Histo VSType and related are moving to XPack where they belong and the histo-specific aggregating logic is getting pulled out of Core. Which means I think if we merge we'll end up undoing this in a few weeks. There still might be a way to share code though, haven't looked super closely at how it will mesh up with the VS stuff. |
|
Interesting. So, should we close this and then revisit again when #53298 lands? |
|
Yeah that might be for the best. Sorry, when I suggested code sharing in the last PR I didn't think about us also moving histo around too :( |
|
Well, technically, we didn't have histo support in the PR where reuse was suggested :) I added it later. So, your suggestions was correct at a time and I didn't revisit it later on. Good catch! |
5 participants
Refactors BoxplotAggregator to reuse parts that are already available in
AbstractTDigestPercentilesAggregator
Follow up for #51948