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

SOLR-16496: provide option for Query Elevation Component to bypass filters #1154

Merged
merged 20 commits into from
Nov 10, 2022

Conversation

rseitz
Copy link
Contributor

@rseitz rseitz commented Oct 31, 2022

https://issues.apache.org/jira/browse/SOLR-16496

Description

QueryElevationComponent now allows individual filter queries to be tagged for exclusion:

q=mainquery&fq=status:public&fq={!tag=dt}doctype:pdf&elevate.excludeTags=dt

When filters are not tagged, elevation respects filters. That is to say, an elevated document will only be included in the result set if it matches all the provided filter queries. When individual filters are tagged for exclusion, however, an elevated document will still be included in the result set even if it does not match those particular filters. This supports a use case where an elevated document should always be returned, regardless of filtering criteria that should still apply to non-elevated documents.

Solution

The setQuery method in QueryElevationComponent has been updated so that if a filter is tagged for exclusion, it is transformed into a boolean OR of the original filter query with a query that matches all of the elevated documents. This is similar to the way the QueryElevationComponent modifies the primary query. When tags are not provided, the behavior is the same as it has been previously (that is to say, filters take precedence over elevation).

Tests

A testFQ() method has been added in QueryElevationComponentTest. The test first establishes that by default, the QueryElevationComponent respects the fq parameter. However, when specific filters are tagged for exclusion, an elevated document will still be included in the result set even if it doesn't match those filters. Of course, a document that is not elevated should still be subject to the filter conditions.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

when elevateFilteredDocs is set to true, the QueryElevationComponent updates the provided filter queries so that elevated documents are not filtered out
@risdenk risdenk changed the title Solr 16496 SOLR-16496: provide option for Query Elevation Component to bypass filters Oct 31, 2022
Copy link
Contributor

@risdenk risdenk left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'm not really familiar with this code, but I know that this is helpful in some ecommerce cases.

@rseitz
Copy link
Contributor Author

rseitz commented Nov 2, 2022

Thanks for reviewing, @risdenk and thanks to @dsmiley for feedback on the JIRA ticket. I have reworked this PR in accordance with that feedback.

Comment on lines 625 to 626
wrappedUpdatedFilter.setCache(eq.getCache());
wrappedUpdatedFilter.setCost(eq.getCost());
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI @risdenk RE immutability of all Query classes. Maybe this slips past us for now but it'll have to change.

Copy link
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

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

Nice; well done. Just some comment EOL reflow to do.
Please add a CHANGES.txt entry under 9.2 new feature, bottom of the list.

Comment on lines 67 to 68
// always return
// an empty set
Copy link
Contributor

Choose a reason for hiding this comment

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

some comment end of line reflow to do here (and elsewhere in your PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been running gradlew tidy and it sometimes breaks my comments into very short likes as you pointed out there. Is there any secret to getting it not to do this, or should I just manually fix the comments and avoid running tidy one last time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed the short comment lines that tidy would "let" me fix... seems like it introduces these short lines in some scenarios and then if I fix them manually and run tidy again, it doesn't always reintroduce them. LMK if there's something else I should be aware of re: using tidy. Also, I added the CHANGES.txt entry.

Copy link
Contributor

Choose a reason for hiding this comment

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

make the comment a single line. then run ./gradlew tidy it will automatically break the comment across lines. you can do this for any longer length comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @risdenk. Did that and it worked.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice technique Kevin. For me -- I use IntelliJ configured with the Google Java Format plugin, so I can easily use my IDE to reformat lines using standard hotkeys etc. Often "tidy" has nothing left to do.

@rseitz
Copy link
Contributor Author

rseitz commented Nov 9, 2022

Hmm... I see the precommit failed because of the (deliberate) use of reference equality in QueryUtilsTest... (assertTrue(queries[0] != queries[1]);). Got a clean run locally though... probably because I didn't use -Pvalidation.errorprone=true. Looking to see if there's a way to suppress that particular rule... (advice welcome, ofc)

@rseitz
Copy link
Contributor Author

rseitz commented Nov 9, 2022

Precommit issue should be fixed now. Added @SuppressWarnings("ReferenceEquality") in QueryUtilsTest.

@dsmiley
Copy link
Contributor

dsmiley commented Nov 9, 2022

Use assertSame for reference equality

@dsmiley dsmiley merged commit 5606d9b into apache:main Nov 10, 2022
dsmiley pushed a commit that referenced this pull request Nov 10, 2022
new param: elevate.excludeTags
Refactored out a QueryUtils.getTaggedQueries used by this and FacetProcessor
@rseitz
Copy link
Contributor Author

rseitz commented Nov 11, 2022

Thanks to @dsmiley for all the support and feedback on this PR that kept making it better, and thanks to @risdenk for having an early look and then helping with the last step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants