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

feat(query): Make BooleanQuery supports minimum_number_should_match #2405

Merged
merged 7 commits into from
Jul 1, 2024

Conversation

LebranceBW
Copy link
Contributor

see issue #2398

In this commit, a novel scorer named DisjunctionScorer is introduced, which performs the union of inverted chains with the minimal required elements. BTW, it's implemented via a min-heap. Necessary modifications on BooleanQuery and BooleanWeight are performed as well.

LebranceBW and others added 2 commits May 20, 2024 11:50
…h`. see issue quickwit-oss#2398

In this commit, a novel scorer named DisjunctionScorer is introduced, which performs the union of inverted chains with the minimal required elements. BTW, it's implemented via a min-heap. Necessary modifications on `BooleanQuery` and `BooleanWeight` are performed as well.
Copy link
Collaborator

@fulmicoton fulmicoton left a comment

Choose a reason for hiding this comment

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

Very solid first PR! Good job. Please have a look at the comments.

@fulmicoton
Copy link
Collaborator

@LebranceBW don't forget to reapply for review once you have added the requested changes

1. More meaningful names.
2. Add Cache for `Disjunction`'s scorers, and fix bug.
3. Optimize `BooleanWeight::complex_scorer`

Thanks
 Paul Masurel <paul@quickwit.io>
@LebranceBW
Copy link
Contributor Author

@fulmicoton I've done above requested changes, and BooleanWeight::complex_scorer has some heavy modification, and I'd say it's a bit messy. Could you help me on it?

@fulmicoton fulmicoton requested review from PSeitz and removed request for fulmicoton May 22, 2024 09:37
@fulmicoton
Copy link
Collaborator

@PSeitz can you help @LebranceBW and take over the code review?

@LebranceBW
Copy link
Contributor Author

@PSeitz Could you help take a review for this PR? Thanks a lot.

src/query/disjunction.rs Outdated Show resolved Hide resolved
src/query/disjunction.rs Outdated Show resolved Hide resolved
@LebranceBW LebranceBW requested a review from PSeitz May 28, 2024 11:11
src/query/disjunction.rs Outdated Show resolved Hide resolved
src/query/disjunction.rs Outdated Show resolved Hide resolved
src/query/disjunction.rs Outdated Show resolved Hide resolved
@LebranceBW LebranceBW requested a review from PSeitz May 29, 2024 01:07
@LebranceBW
Copy link
Contributor Author

@PSeitz
Sorry to interrupt your work, but I wanna know this PR could be push forward again?

1 similar comment
@LebranceBW
Copy link
Contributor Author

@PSeitz
Sorry to interrupt your work, but I wanna know this PR could be push forward again?

src/query/disjunction.rs Outdated Show resolved Hide resolved
@PSeitz
Copy link
Contributor

PSeitz commented Jun 11, 2024

@PSeitz Sorry to interrupt your work, but I wanna know this PR could be push forward again?

@LebranceBW I left a comment to simplify the code regarding FullIntersection

@LebranceBW
Copy link
Contributor Author

@PSeitz Sorry to interrupt your work, but I wanna know this PR could be push forward again?

@LebranceBW I left a comment to simplify the code regarding FullIntersection

@PSeitz FullIntersection already removed.

@LebranceBW LebranceBW requested a review from PSeitz June 14, 2024 08:52
@LebranceBW
Copy link
Contributor Author

@PSeitz
May this PR could be push forward again?

Comment on lines +176 to +182
must_scorers = match must_scorers.take() {
Some(mut must_scorers) => {
must_scorers.append(&mut should_scorers);
Some(must_scorers)
}
None => Some(should_scorers),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

you can just create or append and not reassign the variable

@PSeitz
Copy link
Contributor

PSeitz commented Jul 1, 2024

Thanks @LebranceBW for the PR!

@PSeitz PSeitz merged commit f9ae295 into quickwit-oss:main Jul 1, 2024
3 checks passed
@lnx
Copy link

lnx commented Jul 1, 2024

👍🏻

@LebranceBW LebranceBW deleted the extend_bq branch July 2, 2024 00:27
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.

None yet

4 participants