[ML-DataFrame] add support for max, value_count, cardinality and sum#38569
Conversation
|
Pinging @elastic/ml-core |
There was a problem hiding this comment.
this change is due to type removal
dimitris-athanasiou
left a comment
There was a problem hiding this comment.
Looks good. Left a couple of comments.
There was a problem hiding this comment.
The aggregation names are redundant here. getName() could return name().toLowerCase(Locale.ROOT).
There was a problem hiding this comment.
yeah, thought the same. But did not change it, because that's not part of this PR.
I think I keep this for a later re-factoring, while this is redundant right now, we likely need more meta information for other purposes. One argument to keep this: if we want to have "median" we need to map to the "percentiles" aggregation.
There was a problem hiding this comment.
Shouldn't those 3 get mapped as double?
There was a problem hiding this comment.
both are counts and therefore not floating point types
There was a problem hiding this comment.
I meant about max, min & sum.
There was a problem hiding this comment.
max, min and sum are mapped according to the mapping in the source, not visible in the diff as this hasn't been changed, if you expand the diff you will see:
targetMapping - the field type for the output, if null, the source type should be used
With other words: null is a magic value, telling map deduction to use the source mapping.
There was a problem hiding this comment.
Ah! Cool, thanks for clarifying :-)
f2a7fc4 to
c883d2a
Compare
c883d2a to
d0d0856
Compare
|
run elasticsearch-ci/2 |
add support for max, value_count, cardinality and sum