Skip to content
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

Merged
merged 1 commit into from May 14, 2015

Conversation

jdconrad
Copy link
Contributor

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:

  • min
  • max
  • avg
  • med
  • count
  • sum

Additionally, changes have been made to MultiValueMode to support the
new med and count methods.

@jdconrad jdconrad added v2.0.0-beta1 :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache labels May 12, 2015
* 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 {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@jpountz
Copy link
Contributor

jpountz commented May 12, 2015

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 count later.

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 docs/reference/search/request/sort.asciidoc to mention support of med when sorting?

@jpountz jpountz self-assigned this May 12, 2015

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) {
Copy link
Member

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@jdconrad
Copy link
Contributor Author

@jpountz @rjernst Ready for the next round of review. As noted above, I moved count into its own separate ValueSource/FunctionValues pair for expressions, and responded to all other requests for changes as well.

@rjernst
Copy link
Member

rjernst commented May 13, 2015

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
@jdconrad jdconrad merged commit a5c0ac0 into elastic:master May 14, 2015
@kevinkluge kevinkluge removed the review label May 14, 2015
@jdconrad
Copy link
Contributor Author

@jpountz @rjernst Thanks for the reviews!

@jdconrad jdconrad deleted the pr/mvmethods branch May 15, 2015 18:14
@clintongormley
Copy link

w00t

@clintongormley clintongormley changed the title Scripting: Add Multi-Valued Field Methods to Expressions Add Multi-Valued Field Methods to Expressions Jun 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >feature v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants