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

Support minimumShouldMatch on BoolQuery #184

Merged
merged 6 commits into from
Apr 25, 2023

Conversation

drmarjanovic
Copy link
Member

Description:

  • Added minimumShouldMatch support for BoolQuery.
  • Made the paramsToJson and toJson methods as package private.
  • Formatted code.
  • Adapted tests to check whether minimumShouldMatch is successfully set.

Copy link
Contributor

@markaya markaya left a comment

Choose a reason for hiding this comment

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

Maybe we should add test to check if it is written in file successfully?

Also it would be nice to have one integration test.

Comment on lines +68 to +70
def minimumShouldMatch(value: Int): BoolQuery[S] =
self.copy(minimumShouldMatch = Some(value))

Copy link
Contributor

Choose a reason for hiding this comment

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

Move this after mustNot?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorted alphabetically.

@@ -120,6 +120,21 @@ package object query {
def innerHits(innerHits: InnerHits): Q
}

private[elasticsearch] trait HasMinimumShouldMatch[Q <: HasBoost[Q]] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private[elasticsearch] trait HasMinimumShouldMatch[Q <: HasBoost[Q]] {
private[elasticsearch] trait HasMinimumShouldMatch[Q <: HasMinimumShouldMatch[Q]] {

* @param value
* a number to set `minimumShouldMatch` parameter to
* @return
* a new instance of the [[ElasticQuery]] with the `minimumShouldMatch` value set.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this applies only on BoolQuery, maybe write it here instead of ElasticQuery.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if there are any other places to be supported.

Copy link
Member Author

Choose a reason for hiding this comment

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

At least match bool prefix query supports this. So, let's have it in a separate trait.

@drmarjanovic drmarjanovic merged commit 06dafa3 into main Apr 25, 2023
@drmarjanovic drmarjanovic deleted the task-support-minimum-should-match branch April 25, 2023 12:52
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.

4 participants