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

Don't compile script fields when size is 0 #31824

Closed
jpountz opened this issue Jul 5, 2018 · 9 comments
Closed

Don't compile script fields when size is 0 #31824

jpountz opened this issue Jul 5, 2018 · 9 comments
Labels
>enhancement help wanted adoptme :Search/Search Search-related issues that do not fall into other categories

Comments

@jpountz
Copy link
Contributor

jpountz commented Jul 5, 2018

Today Kibana sends the list of script fields that it may need for convenience, even if it doesn't need any hits. This may be confusing to users sometimes when this causes for instance the script compilation limit to break. While things could be improved on the Kibana side, this issue looks easy to fix in SearchService.parseSource by just ignoring script fields when size is 0.

@jpountz jpountz added >enhancement discuss :Search/Search Search-related issues that do not fall into other categories labels Jul 5, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@dakrone
Copy link
Member

dakrone commented Jul 5, 2018

We discussed this in fix-it-thursday and decided this sounds like a good idea. Let's do it

@dakrone dakrone added help wanted adoptme and removed discuss labels Jul 5, 2018
@Parth-Vader
Copy link
Contributor

@dakrone @jpountz

I would like to take this up. From what I understand, I would have to make changes in this condition and adding source.size() == 0.

Please correct me if I'm wrong.

@rjernst
Copy link
Member

rjernst commented Jul 9, 2018

@Parth-Vader I think it would be this line, so that script fields are not compiled or added to the search context.

Upon further thought, I also wonder whether this should be an error case? Otherwise part of the request is being silently ignored.

@jpountz
Copy link
Contributor Author

jpountz commented Jul 10, 2018

Failing the request is another option indeed. I guess ignoring the script has by preference since it avoids introducing a significant breaking change: to be consistent we'd have to do the same with stored fields, source filtering, highlighting or sort orders.

@Parth-Vader
Copy link
Contributor

@rjernst @jpountz So to get started, source.scriptFields() != null && source.size() != 0 in that line? Or do we want to throw an error too?

@jpountz
Copy link
Contributor Author

jpountz commented Jul 10, 2018

@Parth-Vader We're still having second thoughts regarding whether we want to raise an error, let's give @rjernst a chance to comment.

@rjernst
Copy link
Member

rjernst commented Jul 10, 2018

While I think erroring would be better, @Parth-Vader could certainly implement the very simple optimization proposed, and deprecation/error could come later.

@Parth-Vader
Copy link
Contributor

@rjernst @jpountz Thanks, I've made this PR #31917.

cbuescher pushed a commit that referenced this issue Aug 7, 2018
This change adds a check so that when parsing the search source, script fields are 
ignored when the requested search result size is 0. This helps with e.g. clients like 
Kibana that sends a list of script fields that they may need for convenience, but they
don't require any hits. Before this change, user sometimes ran into confusing behaviour, 
e.g. the script compilation limit to breaking although no hits were requested.

Closes #31824
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement help wanted adoptme :Search/Search Search-related issues that do not fall into other categories
Projects
None yet
Development

No branches or pull requests

5 participants