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

Avoid double counting when sorting by field for faceted query #1836

Merged
merged 1 commit into from Jul 12, 2023

Conversation

metonymic-smokey
Copy link
Contributor

@metonymic-smokey metonymic-smokey commented Jul 6, 2023

When a query sorts by the same field as the facets, the number of search results per facet doubles. This is because there is double counting of the documents by the collector on that field for both sorting and faceting.

This PR deduplicates the fields in the collector to avoid this.

Copy link
Member

@abhinavdangeti abhinavdangeti left a comment

Choose a reason for hiding this comment

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

Let's add a unit test test for this - the issue should be be easily reproduced.
Also create an MB for this and let's use it in the commit header.

@metonymic-smokey
Copy link
Contributor Author

Verification re the unit test:
Without the changes, the unit test fails with the following message:

aditiahuja@Aditis-MacBook-Pro collector % sudo go test --run=TestSetFacetsBuilder
--- FAIL: TestSetFacetsBuilder (0.00s)
    topn_test.go:768: expected fields in collector: [locations], observed: [locations locations]
FAIL
exit status 1
FAIL    github.com/blevesearch/bleve/v2/search/collector        0.558s

Copy link
Member

@abhinavdangeti abhinavdangeti left a comment

Choose a reason for hiding this comment

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

Well done.

@metonymic-smokey
Copy link
Contributor Author

Verification without change:

--- FAIL: TestSortedFacetedQuery (0.56s)
    search_test.go:114: expected 2, got 4
    search_test.go:114: expected 1, got 2
FAIL
exit status 1
FAIL    github.com/blevesearch/bleve/v2 1.799s

@metonymic-smokey metonymic-smokey merged commit b9d9c3b into blevesearch:master Jul 12, 2023
9 checks passed
abhinavdangeti pushed a commit to abhinavdangeti/bleve that referenced this pull request Jul 12, 2023
abhinavdangeti added a commit that referenced this pull request Jul 12, 2023
…ry (#1836) (#1842)

Co-authored-by: Aditi Ahuja <48997495+metonymic-smokey@users.noreply.github.com>
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

3 participants