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

Handle more cases in BooleanWeight#count. #961

Merged
merged 4 commits into from
Jun 21, 2022

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented Jun 15, 2022

As suggested by @zhaih on #950, we could support more cases in
BooleanWeight#count. This PR adds support for these cases specifically:

  • Pure disjunctions where only one clause has a non-zero count.
  • Pure disjunctions where one clause matches all docs.
  • Negations where positive clauses match all docs (pure negation).
  • Negations where positive clauses match no docs.
  • Negations where negative clauses match no docs.
  • Negations where negative clauses match all docs.

As suggested by @zhaih on apache#950, we could support more cases in
`BooleanWeight#count`. This PR adds support for these cases specifically:
 - Pure disjunctions where only one clause has a non-zero count.
 - Pure disjunctions where one clause matches all docs.
 - Negations where positive clauses match all docs (pure negation).
 - Negations where positive clauses match no docs.
 - Negations where negative clauses match no docs.
 - Negations where negative clauses match all docs.
@jpountz jpountz requested a review from zhaih June 15, 2022 16:57
Copy link
Contributor

@zhaih zhaih left a comment

Choose a reason for hiding this comment

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

Overall LGTM, thank you!

There're some rare cases seems not handled, but probably we could left a comment and ignore them for now? Up to you!

}

/**
* Return the number of matches of required clauses, or -1 if unknown, or 0 if there are no
Copy link
Contributor

Choose a reason for hiding this comment

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

number of matches of optional (instead of "required") clauses?

} else if (query.getClauses(Occur.FILTER).isEmpty() == false
|| query.getClauses(Occur.MUST).isEmpty() == false) {
if (query.getMinimumNumberShouldMatch() > 0) {
positiveCount = -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

There'll be still some rare cases here? When minShouldMatch > 0 and no should clause match; when minShouldMatch == 1 and should clauses matches all?

@jpountz
Copy link
Contributor Author

jpountz commented Jun 16, 2022

Thanks for the review, I pushed a comment to clarify that more cases could be handled.

@jpountz jpountz merged commit 68f77db into apache:main Jun 21, 2022
@jpountz jpountz deleted the boolean_query_counts branch June 21, 2022 15:29
jpountz added a commit that referenced this pull request Jun 21, 2022
As suggested by @zhaih on #950, we could support more cases in
`BooleanWeight#count`. This PR adds support for these cases specifically:
 - Pure disjunctions where only one clause has a non-zero count.
 - Pure disjunctions where one clause matches all docs.
 - Negations where positive clauses match all docs (pure negation).
 - Negations where positive clauses match no docs.
 - Negations where negative clauses match no docs.
 - Negations where negative clauses match all docs.
shaie pushed a commit to mdmarshmallow/lucene that referenced this pull request Jun 22, 2022
As suggested by @zhaih on apache#950, we could support more cases in
`BooleanWeight#count`. This PR adds support for these cases specifically:
 - Pure disjunctions where only one clause has a non-zero count.
 - Pure disjunctions where one clause matches all docs.
 - Negations where positive clauses match all docs (pure negation).
 - Negations where positive clauses match no docs.
 - Negations where negative clauses match no docs.
 - Negations where negative clauses match all docs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants