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

Empty bool {} should return match_all #7347

Closed
wants to merge 6 commits into from

Conversation

Projects
None yet
4 participants
@brwe
Copy link
Contributor

brwe commented Aug 20, 2014

This also fixes has_parent filters with a nested empty bool filter
(see test SimpleChildQuerySearchTests#test6722, the test should actually expect
either 0 results when searching for has_parent "test" or one result when
search for has_parent "foo")

closes #7240

@brwe brwe added the review label Aug 20, 2014

@rjernst

View changes

...t/java/org/elasticsearch/search/aggregations/bucket/StringTermsWithoutClusterScopeTests.java Outdated
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.instanceOf;

/**

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 21, 2014

Member

I don't think an empty javadoc comment is useful here?

@rjernst

View changes

src/test/java/org/elasticsearch/index/query/empty-bool-query.json Outdated
@@ -0,0 +1,3 @@
{

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 21, 2014

Member

Seems like test data like this can just be inlined in the test case? I would do that for most of these actually, just my opinion (it seems a little weird to me to have test data pulled in as resources for such small data).

@rjernst

This comment has been minimized.

Copy link
Member

rjernst commented Aug 21, 2014

LGTM. Just a couple minor comments.

@s1monw

View changes

...t/java/org/elasticsearch/search/aggregations/bucket/StringTermsWithoutClusterScopeTests.java Outdated
/**
*
*/
public class StringTermsWithoutClusterScopeTests extends ElasticsearchIntegrationTest {

This comment has been minimized.

Copy link
@s1monw

s1monw Aug 21, 2014

Contributor

can you maybe find a better name for this?

@s1monw s1monw removed the review label Aug 21, 2014

@brwe brwe force-pushed the brwe:issue-7240 branch Aug 21, 2014

@brwe

This comment has been minimized.

Copy link
Contributor Author

brwe commented Aug 21, 2014

Thanks for the review! Updated with commits to adress the comments.

@brwe brwe added the review label Aug 21, 2014

@rjernst

View changes

src/test/java/org/elasticsearch/index/query/SimpleIndexQueryParserTests.java Outdated

public void test6722Parser() throws ElasticsearchException, IOException {

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 22, 2014

Member

Maybe call this "testEmptyBoolSubclausesMatchAll()"? Sorry if I misunderstood what the test is doing, I just think having a github issue number in the name is unhelpful to someone if they see a failure.

@rjernst

View changes

src/test/java/org/elasticsearch/search/aggregations/bucket/DedicatedAggregationTests.java Outdated
public class DedicatedAggregationTests extends ElasticsearchIntegrationTest {

@Test
public void issue7240EmptyBoolShouldReturnMatchAll() throws IOException {

This comment has been minimized.

Copy link
@rjernst

rjernst Aug 22, 2014

Member

Same as above. I would not put an issue number in the name. I would also suggest starting the function name with "test".

@rjernst

This comment has been minimized.

Copy link
Member

rjernst commented Aug 22, 2014

LGTM. A couple more cosmetic naming comments.

@brwe

This comment has been minimized.

Copy link
Contributor Author

brwe commented Aug 26, 2014

Renamed the tests.

@rjernst

This comment has been minimized.

Copy link
Member

rjernst commented Aug 26, 2014

+1 to commit.

@brwe brwe removed the review label Aug 27, 2014

brwe added some commits Aug 19, 2014

bool query: parser should return match_all in case there are no clauses
This also fixes has_parent filters with a nested empty bool filter
(see test SimpleChildQuerySearchTests#test6722, the test should actually expect
either 0 results when searching for has_parent "test" or one result when
search for has_parent "foo")

closes #7240
fix test - this only bubbled up after d414d89
The query had the wrong parent type (which is not really wrong, just
confusing) and the type filter inside the parent_filter
is redundant.

@brwe brwe force-pushed the brwe:issue-7240 branch to 58735d7 Aug 27, 2014

@brwe

This comment has been minimized.

Copy link
Contributor Author

brwe commented Aug 27, 2014

After rebasing to master I had to fix a test, see 58735d7
The type cache is actually redundant.

brwe added a commit that referenced this pull request Aug 27, 2014

bool query: parser should return match_all in case there are no clauses
This also fixes has_parent filters with a nested empty bool filter
(see test SimpleChildQuerySearchTests#test6722, the test should actually expect
either 0 results when searching for has_parent "test" or one result when
search for has_parent "foo")

closes #7240
closes #7347

@brwe brwe closed this in 238efe5 Aug 27, 2014

brwe added a commit that referenced this pull request Aug 27, 2014

bool query: parser should return match_all in case there are no clauses
This also fixes has_parent filters with a nested empty bool filter
(see test SimpleChildQuerySearchTests#test6722, the test should actually expect
either 0 results when searching for has_parent "test" or one result when
search for has_parent "foo")

closes #7240
closes #7347

brwe added a commit that referenced this pull request Sep 8, 2014

bool query: parser should return match_all in case there are no clauses
This also fixes has_parent filters with a nested empty bool filter
(see test SimpleChildQuerySearchTests#test6722, the test should actually expect
either 0 results when searching for has_parent "test" or one result when
search for has_parent "foo")

closes #7240
closes #7347

@clintongormley clintongormley changed the title bool query: parser should return match_all in case there are no clauses Query DSL: Empty bool {} should return match_all Sep 8, 2014

@clintongormley clintongormley added the >bug label Sep 8, 2014

@clintongormley clintongormley changed the title Query DSL: Empty bool {} should return match_all Empty bool {} should return match_all Jun 7, 2015

mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015

bool query: parser should return match_all in case there are no clauses
This also fixes has_parent filters with a nested empty bool filter
(see test SimpleChildQuerySearchTests#test6722, the test should actually expect
either 0 results when searching for has_parent "test" or one result when
search for has_parent "foo")

closes elastic#7240
closes elastic#7347
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.