Add support for booleans in scripts #20950
Conversation
Since 2.0, booleans have been represented as numeric fields (longs). However, in scripts, this is odd, since you expect doing a comparison against a boolean to work. While languages like groovy will auto convert between booleans and longs, painless does not. This changes the doc values accessor for boolean fields in scripts to return Boolean objects instead of Long objects. closes elastic#20949
rjernst
added
>enhancement
:Core/Infra/Scripting
|
This is technically breaking, since anyone who has written a painless script already will need to change from comparing a 0/1 constant to true/false. But we have said that painless will change. The question is if this is considered small enough (and enough of a pain for people to change) that it should be moved into 5.0 (I lean towards just putting in 5.1, and noting it in the release notes). /cc @clintongormley |
|
LGTM, thanks! |
| } | ||
|
|
||
| @Override | ||
| public List<Boolean> getValues() { |
There was a problem hiding this comment.
does it make sense to assign Collections.unmodifiableList(this); to a member then we don't create a new object each time it's pulled?
There was a problem hiding this comment.
actually we could even skip the unmodifiableList wrapper since the wrapped list is already unmodifiable
| @@ -291,4 +291,38 @@ public double geohashDistanceWithDefault(String geohash, double defaultValue) { | |||
| return geohashDistance(geohash); | |||
| } | |||
| } | |||
|
|
|||
| class Booleans extends AbstractList<Boolean> implements ScriptDocValues<Boolean> { | |||
There was a problem hiding this comment.
should this class be static and final?
There was a problem hiding this comment.
static is not necessary since it is inside an interface, but good catch on the final.
| } | ||
|
|
||
| @Override | ||
| public List<Boolean> getValues() { |
There was a problem hiding this comment.
actually we could even skip the unmodifiableList wrapper since the wrapped list is already unmodifiable
I think I'd be tempted to put this one into 5.0. Since it will be breaking anyway, I'd rather require a change between RC and GA than between two minor releases. |
clintongormley
changed the title
I agree |
++ this is really a bug IMO that we don't have this |
* Scripting: Add support for booleans in scripts Since 2.0, booleans have been represented as numeric fields (longs). However, in scripts, this is odd, since you expect doing a comparison against a boolean to work. While languages like groovy will auto convert between booleans and longs, painless does not. This changes the doc values accessor for boolean fields in scripts to return Boolean objects instead of Long objects. closes #20949 * Make Booleans final and remove wrapping of `this` for getValues()
* Scripting: Add support for booleans in scripts Since 2.0, booleans have been represented as numeric fields (longs). However, in scripts, this is odd, since you expect doing a comparison against a boolean to work. While languages like groovy will auto convert between booleans and longs, painless does not. This changes the doc values accessor for boolean fields in scripts to return Boolean objects instead of Long objects. closes #20949 * Make Booleans final and remove wrapping of `this` for getValues()
Since 2.0, booleans have been represented as numeric fields (longs).
However, in scripts, this is odd, since you expect doing a comparison
against a boolean to work. While languages like groovy will auto convert
between booleans and longs, painless does not.
This changes the doc values accessor for boolean fields in scripts to
return Boolean objects instead of Long objects.
closes #20949