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

#48475 Pure disjunctions should rewrite to a MatchNoneQueryBuilder #48557

Merged
merged 9 commits into from
Nov 28, 2019

Conversation

SivagurunathanV
Copy link
Contributor

  1. Added Pure disjunctions should rewrite to a MatchNoneQueryBuilder when all instance matches MatchNoneQueryBuilder
  2. Added testcase for the same.

@SivagurunathanV
Copy link
Contributor Author

#48557

@jimczi
Copy link
Contributor

jimczi commented Oct 28, 2019

@SivagurunathanV we already rewrite bool query with a MatchNoneQueryBuilder in a filter or a must clause to a match none query so I don't think that your pr changes anything. We don't need to check that all required clauses are an instance of MatchNoneQueryBuilder, one is enough.

@jpountz
Copy link
Contributor

jpountz commented Oct 28, 2019

@SivagurunathanV The linked issue is about optional clauses while your PR handles required clauses. If you don't understand how the issue should be addressed, please raise questions before opening a pull request.

@SivagurunathanV
Copy link
Contributor Author

@jimczi Thanks for the clarification. @jpountz yeah sure. Can you please confirm, optional clauses you mean the should BoolQuery classes alone?

@jpountz
Copy link
Contributor

jpountz commented Oct 28, 2019

@SivagurunathanV Yes indeed. One important factor is that SHOULD clauses behave differently depending on whether there are any required (MUST or FILTER) clauses, so we will need to be carefull with this.

@SivagurunathanV
Copy link
Contributor Author

@jpountz Thanks for the explanation. Based on my understanding, if the required clauses are empty and the optional clause is returning MatchNone, we can safely make an early termination.

@jpountz
Copy link
Contributor

jpountz commented Oct 29, 2019

@SivagurunathanV Yes, if there are no required clauses and all SHOULD clauses rewrite to a MatchNoneQueryBuilder then the bool query can rewrite to a MatchNoneQueryBuilder.

@SivagurunathanV
Copy link
Contributor Author

@jpountz Thanks. Added my changes to the PR. Please let me know if that works.

@cbuescher cbuescher added the :Search/Search Search-related issues that do not fall into other categories label Nov 7, 2019
@elasticmachine
Copy link
Collaborator

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

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

Thanks, the change looks good to me! I'll merge this PR next week.

@SivagurunathanV
Copy link
Contributor Author

@jpountz Can you merge this PR?

@jpountz
Copy link
Contributor

jpountz commented Nov 24, 2019

Oops, thanks for the reminder, I'll merge soon.

@jpountz
Copy link
Contributor

jpountz commented Nov 26, 2019

@elasticmachine ok to test

@@ -417,12 +417,19 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws
changed |= rewriteClauses(queryRewriteContext, mustNotClauses, newBuilder::mustNot);
changed |= rewriteClauses(queryRewriteContext, filterClauses, newBuilder::filter);
changed |= rewriteClauses(queryRewriteContext, shouldClauses, newBuilder::should);
// early termination when must clause is empty and optional clauses is returning MatchNoneQueryBuilder
if(mustClauses.size() == 0 && filterClauses.size() == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Given test failures, I think the problem is that this rewrites empty boolean queries to a match_none, while this is currently treated as a match_all. We probably to do something like below, can you add a unit test for this case?

Suggested change
if(mustClauses.size() == 0 && filterClauses.size() == 0
if (mustClauses.size() == 0 && filterClauses.size() == 0 && shouldClauses.size() > 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch. I just updated the code. Let me know.

@SivagurunathanV
Copy link
Contributor Author

@jpountz I am not able to see the failed job logs in Jenkins, can you please help me here. what is the exact issue. Thanks

@jpountz
Copy link
Contributor

jpountz commented Nov 27, 2019

@elasticmachine ok to test

@jpountz
Copy link
Contributor

jpountz commented Nov 27, 2019

@SivagurunathanV The logs don't require authentication or anything, so you should be able to read them. Some tests were complaining that the matching documents were not the expected ones.

@elasticmachine
Copy link
Collaborator

user doesn't have permission to update head repository

@jpountz
Copy link
Contributor

jpountz commented Nov 27, 2019

@SivagurunathanV Would you mind merging master into your branch?

@SivagurunathanV
Copy link
Contributor Author

@jpountz Thanks for the help. I merged the master into my branch and all test passed :)

@jpountz
Copy link
Contributor

jpountz commented Nov 28, 2019

@elasticmachine ok to test

@jpountz jpountz merged commit 08bf0ae into elastic:master Nov 28, 2019
@jpountz
Copy link
Contributor

jpountz commented Nov 28, 2019

Thanks @SivagurunathanV !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search/Search Search-related issues that do not fall into other categories v7.6.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants