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.

Sign up for GitHub

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

Jump to bottom

Ignore script fields when size is 0 #31917

Merged
merged 10 commits into from
Aug 7, 2018
Merged

Ignore script fields when size is 0 #31917

merged 10 commits into from
Aug 7, 2018

Conversation

Parth-Vader
Copy link
Contributor

Addresses issue #31824

I have added the condition as discussed.

@Parth-Vader Parth-Vader mentioned this pull request Jul 10, 2018
Closed
cbuescher
cbuescher requested changes Jul 10, 2018
View reviewed changes
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

Hi @Parth-Vader, thanks for opening the PR. Could you also add a unit test for the change you made? I think adding a test case to SearchServiceTests would work. You can take a look at e.g the existing "testMaxScriptFieldsSearch()" which tests something slightly different but covers the same area in the code.

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

Pinging @elastic/es-search-aggs

@Parth-Vader
Copy link
Contributor Author

@cbuescher @colings86 Can you have a look at the test? Is it correctly done? Thanks.

@cbuescher cbuescher self-assigned this Jul 22, 2018
cbuescher
cbuescher requested changes Jul 23, 2018
View reviewed changes
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

Hi @Parth-Vader, the test already looks quite good in general. However, it currently also passes without your changes in SearchService.java. I think you need to add a script field to searchSourceBuilder, and later you then check that this is ignored when size=0. You can take a look at "testMaxScriptFieldsSearch" (L328) for an example of how to add a dummy script field to the search source.

server/src/test/java/org/elasticsearch/search/SearchServiceTests.java Outdated

SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder();
searchSourceBuilder.size(0);
try(SearchContext context = service.createContext(new ShardSearchLocalRequest(indexShard.shardId(), 1, SearchType.DEFAULT,
Copy link
Member

Choose a reason for hiding this comment

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

nit: whitespace after "try"

server/src/test/java/org/elasticsearch/search/SearchServiceTests.java Outdated
searchSourceBuilder.size(0);
try(SearchContext context = service.createContext(new ShardSearchLocalRequest(indexShard.shardId(), 1, SearchType.DEFAULT,
searchSourceBuilder, new String[0], false, new AliasFilter(null, Strings.EMPTY_ARRAY), 1.0f, true, null, null))) {
assertEquals(0,context.scriptFields().fields().size());
Copy link
Member

Choose a reason for hiding this comment

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

nit: whitespace after comma

@Parth-Vader
Copy link
Contributor Author

@cbuescher Hi, I added a script field, then set the size to zero. Also added the required white spaces.

cbuescher
cbuescher requested changes Aug 6, 2018
View reviewed changes
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

Hi @Parth-Vader, thanks for the changes, there are only a couple of very minor formatting changes I'd like you to make if possible. The rest looks great now.

server/src/test/java/org/elasticsearch/search/SearchServiceTests.java Outdated
@@ -345,6 +346,26 @@ public void testMaxScriptFieldsSearch() throws IOException {
}
}

public void testIgnoreScriptfieldIfSizeZero() throws IOException{
Copy link
Member

Choose a reason for hiding this comment

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

nit: please add a whitespace after IOExecption

server/src/test/java/org/elasticsearch/search/SearchServiceTests.java Outdated
final IndexShard indexShard = indexService.getShard(0);

SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder();

Copy link
Member

Choose a reason for hiding this comment

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

nit: pls remove the extra blank lines (at least the double one)

server/src/test/java/org/elasticsearch/search/SearchServiceTests.java Outdated
searchSourceBuilder.size(0);
try (SearchContext context = service.createContext(new ShardSearchLocalRequest(indexShard.shardId(), 1, SearchType.DEFAULT,
searchSourceBuilder, new String[0], false, new AliasFilter(null, Strings.EMPTY_ARRAY), 1.0f, true, null, null))) {
assertEquals(0, context.scriptFields().fields().size());
Copy link
Member

Choose a reason for hiding this comment

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

nit: please move indentation to the right so it alligns at four spaces in the try-block)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, do you mean shift assertEquals four spaces backwards?

cbuescher
cbuescher approved these changes Aug 6, 2018
View reviewed changes
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

@Parth-Vader many thanks, LGTM. I will run CI before merging.

@cbuescher
Copy link
Member

@elasticmachine test this please

cbuescher
cbuescher reviewed Aug 6, 2018
View reviewed changes
server/src/test/java/org/elasticsearch/search/SearchServiceTests.java Outdated
@@ -56,6 +56,7 @@
import org.elasticsearch.search.aggregations.support.ValueType;
import org.elasticsearch.search.builder.SearchSourceBuilder;
import org.elasticsearch.search.fetch.ShardFetchRequest;
import org.elasticsearch.search.fetch.subphase.ScriptFieldsContext;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the checkstyle runner doesn't like unused imports, could you remove this line please?

17:52:18 [ant:checkstyle] [ERROR] /var/lib/jenkins/workspace/elastic+elasticsearch+pull-request/server/src/test/java/org/elasticsearch/search/SearchServiceTests.java:59:8: Unused import - org.elasticsearch.search.fetch.subphase.ScriptFieldsContext. [UnusedImports]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad. Done!

@cbuescher
Copy link
Member

@elasticmachine test this please

@cbuescher
Copy link
Member

@Parth-Vader more CI failures, this time due to an old version of master. I took the liberty to merge in current master, hope you don't mind
@elasticmachine test this please

@cbuescher cbuescher merged commit 6fe6247 into elastic:master Aug 7, 2018
cbuescher pushed a commit that referenced this pull request Aug 7, 2018
@Parth-Vader
Ignore script fields when size is 0 (#31917)
ba5fb3b 
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
@cbuescher
Copy link
Member

@Parth-Vader thanks again, I merged this to master and the 6.5 branch.

@Parth-Vader
Copy link
Contributor Author

@cbuescher Thanks for guiding me through this.

@Parth-Vader Parth-Vader deleted the SearchService-new branch August 7, 2018 10:47
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbuescher cbuescher cbuescher approved these changes

Assignees

@cbuescher cbuescher

Labels
>enhancement :Search/Search Search-related issues that do not fall into other categories v6.5.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Parth-Vader @elasticmachine @cbuescher @colings86 @jimczi