-
Notifications
You must be signed in to change notification settings - Fork 25.4k
Add boolean values script fields #60830
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 boolean values script fields #60830
Conversation
This adds `boolean` values `runtime_script` fields.
Pinging @elastic/es-search (:Search/Search) |
} | ||
|
||
@Override | ||
public Query rangeQuery( |
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'm trying to mimic what we support in core. It works, so far as I can tell. It is kind of amazing we allow this at all, but we do!
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.
oh boy :)
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.
thanks for all the tests, left a couple of comments but LGTM. i was wondering if we should have some duel tests to compare behaviour between concrete boolean fields and runtime boolean fields, given that we discovered some nuances around range queries etc.
@Override | ||
public long nextValue() throws IOException { | ||
// Emit all false values before all true values | ||
return cursor++ < script.falses() ? 0 : 1; |
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 must say that multiple values for booleans look really weird, especially trying to summarize multiple values into one, and re-sorting them. Is this behaviour documented anywhere?
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'm not sure it is, though it is what happens when you use doc values for out of the box booleans.
|
||
@Override | ||
public NumericType getNumericType() { | ||
return NumericType.DOUBLE; |
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 never realized this either: booleans are double????
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.
Ooops, copy and paste error. Fixing.
|
||
@Override | ||
public Object valueForDisplay(Object value) { | ||
throw new UnsupportedOperationException(); |
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.
why do we throw? Shouldn't we borrow the logic from https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java#L157 ?
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 can do that, but I don't think it is ever called!
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.
oh well I did not realize it was unused, in that case I am good with throwing, but add a comment?
} | ||
|
||
@Override | ||
public Query rangeQuery( |
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.
oh boy :)
I've added some dual tests. I think it'd probably be good to add them for every field to be honest. I can work on that. |
f7c099e
to
21a810f
Compare
run elasticsearch-ci/2 |
This adds
boolean
valuesruntime_script
fields.