-
-
Notifications
You must be signed in to change notification settings - Fork 246
HSEARCH-4489 Add matchNone() predicate #2964
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
Conversation
yrodiere
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well this is a nice surprise, thanks!
I added a few comments below.
| include::{sourcedir}/org/hibernate/search/documentation/search/predicate/PredicateDslIT.java[tags=matchNone] | ||
| ---- | ||
| ==== | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't include the original use case in the ticket; here it is: https://discourse.hibernate.org/t/fail-fast-predicate/6062?u=yrodiere
The best solution to avoid running a search query is simply to not call .fetch() at all, but in some cases people will have methods dedicated to the creation of inner predicates, and will not be able to prevent the query to run; in that case, they can just create a matchNone predicate and the end result will be the same (though it will unfortunately involve some pointless I/O).
So, as you can see, it's a very specific use case, for advanced users. I'm not sure we need further explanation than what you did here, but if you think you can add something clear and concise, feel free to give it a shot :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that's an interesting case... Knew of match_all usage within dynamic filters but not the match_none. nice 😃
As for the text, I gave it a try ... aaaand the text becomes too cumbersome 😄 but as you've said - it's for advanced users. So those who need it would know its meaning, and others are probably safer without using it 😃
| * Match none of the documents. | ||
| * | ||
| * @return The initial step of a DSL where the "match none" predicate can be defined. | ||
| * @see MatchAllPredicateOptionsStep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * @see MatchAllPredicateOptionsStep | |
| * @see MatchNonePredicateFinalStep |
| @Test | ||
| public void matchNone() { | ||
| assertThatQuery( index.query() | ||
| .where( SearchPredicateFactory::matchNone ) ) | ||
| .hasNoHits(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just add another test with a bool predicate containing one predicate that does match a document, and also the matchNone predicate, to check that the resulting bool predicate won't match anything?
adc0f68 to
3aa8e5f
Compare
- add matchNone() to DSL - update documentation to mention new predicate
3aa8e5f to
c15c92d
Compare
|
haha, well I thought that the links update is not a significant enough change to add, so found this ticket 😄 applied the suggested changes and rebased the branch. |
|
Kudos, SonarCloud Quality Gate passed! |
yrodiere
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Merging. Thanks again!








https://hibernate.atlassian.net/browse/HSEARCH-4489
I wasn't able to think of some simple yet, at the same time, meaningful use case for this new predicate, hence if anyone has some suggestions would be happy to add more tests around it.