Skip to content

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

Merged
merged 7 commits into from
Aug 17, 2020

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Aug 6, 2020

This adds boolean values runtime_script fields.

This adds `boolean` values `runtime_script` fields.
@nik9000 nik9000 added the :Search/Search Search-related issues that do not fall into other categories label Aug 6, 2020
@nik9000 nik9000 requested a review from javanna August 6, 2020 16:10
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Search)

@elasticmachine elasticmachine added the Team:Search Meta label for search team label Aug 6, 2020
}

@Override
public Query rangeQuery(
Copy link
Member Author

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!

Copy link
Member

Choose a reason for hiding this comment

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

oh boy :)

@javanna javanna mentioned this pull request Aug 6, 2020
30 tasks
Copy link
Member

@javanna javanna left a 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;
Copy link
Member

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?

Copy link
Member Author

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;
Copy link
Member

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????

Copy link
Member Author

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

Choose a reason for hiding this comment

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

Copy link
Member Author

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!

Copy link
Member

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(
Copy link
Member

Choose a reason for hiding this comment

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

oh boy :)

@nik9000
Copy link
Member Author

nik9000 commented Aug 17, 2020

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.

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.

@nik9000 nik9000 force-pushed the script_field_boolean branch from f7c099e to 21a810f Compare August 17, 2020 16:08
@nik9000
Copy link
Member Author

nik9000 commented Aug 17, 2020

run elasticsearch-ci/2

@nik9000 nik9000 merged commit 07bfa51 into elastic:feature/runtime_fields Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants