-
Notifications
You must be signed in to change notification settings - Fork 25k
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 Multi-Valued Field Methods to Expressions #11105
Conversation
* Note that apply/reduce do not work with COUNT since count cannot be derived from | ||
* an accumulator algorithm without a consistency between which value is the accumulation. | ||
*/ | ||
COUNT { |
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.
Maybe this one would be best outside of MultiValueMode since it is quite different from the other ones in that it always produces integers no matter what the values source is?
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.
I completely agree with this, so I moved count out of MultiValueMode and created a ValueSource/FunctionValues pair for it exclusively for expressions instead.
The median impl looks fine but count looks more challenging since it doesn't work like other modes. Maybe we should only focus on exposing min/max/sum/avg/med in this PR and think about Regarding apply/reduce, I think we should nuke them: they are only used for function score and look easy to replace. We can do it in another PR though. Could you update |
|
||
class DateMethodFunctionValues extends FieldDataFunctionValues { | ||
private final int calendarType; | ||
private final Calendar calendar; | ||
|
||
DateMethodFunctionValues(ValueSource parent, AtomicNumericFieldData data, int calendarType) { | ||
super(parent, data); | ||
DateMethodFunctionValues(ValueSource parent, MultiValueMode multiValueMode, int calendarType, AtomicNumericFieldData data) { |
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.
Can we keep the order of arguments for this and super ctor consistent? So all the args that will go to the super class should come first, in the same order to the super ctor? It is confusing to see args moved around (here the order is consistent but in others it seems not)
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.
Fixed.
LGTM |
Add methods to operate on multi-valued fields in the expressions language. Note that users will still not be able to access individual values within a multi-valued field. The following methods will be included: * min * max * avg * median * count * sum Additionally, changes have been made to MultiValueMode to support the new median method. closes elastic#11105
w00t |
Add methods to operate on multi-valued fields in the expressions language.
Note that users will still not be able to access individual values
within a multi-valued field in expressions at this time.
The following methods will be included:
Additionally, changes have been made to MultiValueMode to support the
new med and count methods.