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

[MM-25538] Adds the termOperator and takes it into account for terms and hashtag queries #14664

Merged
merged 2 commits into from
Jun 1, 2020

Conversation

mgdelacroix
Copy link
Member

Summary

Adds the term operator for both term and hashtag queries and for normal and excluded terms

Ticket Link

https://mattermost.atlassian.net/browse/MM-25538

@mgdelacroix mgdelacroix added 2: Dev Review Requires review by a developer 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels May 26, 2020
@mgdelacroix mgdelacroix added this to the v5.24.0 milestone May 26, 2020
@amyblais amyblais added the CherryPick/Approved Meant for the quality or patch release tracked in the milestone label May 26, 2020
@ogi-m ogi-m added the Setup Cloud Test Server Setup an on-prem test server label May 27, 2020
Copy link
Member

@jespino jespino left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ethervoid ethervoid left a comment

Choose a reason for hiding this comment

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

One question

@@ -43,6 +43,11 @@ func (b *BleveEngine) SearchPosts(channels *model.ChannelList, searchParams []*m
filters = append(filters, typeQ)

for i, params := range searchParams {
var termOperator query.MatchQueryOperator = query.MatchQueryOperatorAnd
Copy link
Contributor

Choose a reason for hiding this comment

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

Question. Why don't do termOperator := query.MatchQueryOperatorAnd?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not really sure why, but the type inference system doesn't play well with that assignment particularly:

# github.com/mattermost/mattermost-server/v5/services/searchengine/bleveengine
services/searchengine/bleveengine/search.go:149:25: cannot use termOperator (type int) as type query.MatchQueryOperator in argument to hashtagQ.SetOperator
services/searchengine/bleveengine/search.go:154:25: cannot use termOperator (type int) as type query.MatchQueryOperator in argument to hashtagQ.SetOperator
services/searchengine/bleveengine/search.go:161:25: cannot use termOperator (type int) as type query.MatchQueryOperator in argument to messageQ.SetOperator
services/searchengine/bleveengine/search.go:168:25: cannot use termOperator (type int) as type query.MatchQueryOperator in argument to messageQ.SetOperator

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, is a problem with how the constant is created. The type is MatchOperator but the constant is being created as int as you can see here.

I've sent a PR in order to fix that in the upcoming versions if it's merged

Copy link
Member Author

Choose a reason for hiding this comment

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

Awesome! Thanks for taking the time to send the PR!! ❤️

Copy link

@ogi-m ogi-m left a comment

Choose a reason for hiding this comment

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

Tested and looks good 👍

@ogi-m ogi-m removed the 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review label May 29, 2020
@mgdelacroix
Copy link
Member Author

/update-branch

Copy link
Contributor

@ethervoid ethervoid left a comment

Choose a reason for hiding this comment

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

LGTM!

@mgdelacroix mgdelacroix added 4: Reviews Complete All reviewers have approved the pull request AutoMerge Used by Mattermod to merge PR automatically and removed 2: Dev Review Requires review by a developer labels Jun 1, 2020
@mattermod
Copy link
Contributor

Will try to auto merge this PR once all tests and checks are passing. This might take up to an hour.

1 similar comment
@mattermod
Copy link
Contributor

Will try to auto merge this PR once all tests and checks are passing. This might take up to an hour.

@mgdelacroix mgdelacroix removed the Setup Cloud Test Server Setup an on-prem test server label Jun 1, 2020
@mm-cloud-bot
Copy link

Test server destroyed

@mattermod
Copy link
Contributor

Will try to auto merge this PR once all tests and checks are passing. This might take up to an hour.

@mattermod
Copy link
Contributor

Trying to auto merge this PR.

@mattermod mattermod merged commit 6643a7c into master Jun 1, 2020
@mattermod
Copy link
Contributor

Pull Request successfully merged
SHA: 6643a7c

@mattermod mattermod deleted the mm-25538-add-search-terms-bleve branch June 1, 2020 18:31
@mattermod mattermod removed the AutoMerge Used by Mattermod to merge PR automatically label Jun 1, 2020
@mattermod mattermod added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Jun 1, 2020
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Jun 2, 2020
@ogi-m ogi-m added the Tests/Not Needed New release tests not required label Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone Docs/Not Needed Does not require documentation Tests/Not Needed New release tests not required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants