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

HSEARCH-3498 Move optional predicate DSL parameters to the terminal contexts #1917

Merged
merged 8 commits into from Mar 19, 2019

Conversation

@yrodiere
Copy link
Member

yrodiere commented Mar 18, 2019

https://hibernate.atlassian.net//browse/HSEARCH-3498

@gsmet Any opinion on this?

The motivations and some examples are in the ticket descriptions.

@fax4ever @gsmet I'd be interested in your opinion on HSEARCH-3522, too?

@yrodiere yrodiere force-pushed the yrodiere:HSEARCH-3498 branch 2 times, most recently from da94df3 to 0e89043 Mar 18, 2019
@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 18, 2019

Pull Request Test Coverage Report for Build 6

  • 48 of 48 (100.0%) changed or added relevant lines in 10 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 88.854%

Totals Coverage Status
Change from base Build 167: -0.02%
Covered Lines: 16175
Relevant Lines: 18204

💛 - Coveralls
@yrodiere yrodiere force-pushed the yrodiere:HSEARCH-3498 branch 2 times, most recently from dfd9cbb to 5610522 Mar 19, 2019
@gsmet
gsmet approved these changes Mar 19, 2019
Copy link
Member

gsmet left a comment

I haven't checked the code but I agree with the general idea.

Copy link
Contributor

fax4ever left a comment

I think that changes here are OK. I've spent some time seeking if it would be possible to avoid the new generic on AbstractBooleanMultiFieldPredicateCommonState.java, but I've not found a good alternative to do that.

The issue mentions (see Pros#3) that we would be able to do more validations, such as validate fuzzy is not applied to an Integer field. Do we have some tests for these cases?

Moreover, I was thinking that maybe it would be nice change MultiFieldPredicateFieldSetContext#boostedTo method name in something different, like boostField, in order to avoid confusion between predicate and field level boosts.

@@ -74,8 +74,8 @@ default MatchPredicateFieldSetContext orRawField(String absoluteFieldPath) {
* The signature of this method defines this parameter as an {@link Object},
* but a specific type is expected depending on the targeted field.
* See <a href="SearchPredicateFactoryContext.html#commonconcepts-parametertype">there</a> for more information.
* @return A context allowing to get the resulting predicate.
* @return A context allowing to set options or get the resulting predicate.

This comment has been minimized.

Copy link
@fax4ever

fax4ever Mar 19, 2019

Contributor

Small change suggested here. What you think about to change: /set options/set match predicate options/?

static class CommonState<B> extends AbstractBooleanMultiFieldPredicateCommonState<B, RangePredicateFieldSetContextImpl<B>>
implements RangePredicateTerminalContext {
static class CommonState<B> extends AbstractBooleanMultiFieldPredicateCommonState<CommonState<B>, B, RangePredicateFieldSetContextImpl<B>>
implements RangePredicateTerminalContext, RangePredicateLimitTerminalContext {

This comment has been minimized.

Copy link
@fax4ever

fax4ever Mar 19, 2019

Contributor

Maybe we could remove the first interface after implements here.

This comment has been minimized.

Copy link
@yrodiere

yrodiere Mar 19, 2019

Author Member

Done

@yrodiere

This comment has been minimized.

Copy link
Member Author

yrodiere commented Mar 19, 2019

I addressed some comments. Here are my answers:

The issue mentions (see Pros#3) that we would be able to do more validations, such as validate fuzzy is not applied to an Integer field. Do we have some tests for these cases?

It's not about making more validations, since we already had this validation. It's about throwing the exception at the right time, i.e. when fuzzy() is called instead of when matching is called. Yes we have a test in org.hibernate.search.integrationtest.backend.tck.search.predicate.MatchSearchPredicateIT#error_fuzzy_unsupportedFieldType.

Moreover, I was thinking that maybe it would be nice change MultiFieldPredicateFieldSetContext#boostedTo method name in something different, like boostField, in order to avoid confusion between predicate and field level boosts.

Not sure it's a great name, but feel free to create a ticket so that we think about it.

@yrodiere yrodiere force-pushed the yrodiere:HSEARCH-3498 branch from 5610522 to c80659c Mar 19, 2019
@yrodiere yrodiere merged commit f8cf6ba into hibernate:master Mar 19, 2019
@yrodiere

This comment has been minimized.

Copy link
Member Author

yrodiere commented Mar 19, 2019

Merged, thanks!

@yrodiere yrodiere deleted the yrodiere:HSEARCH-3498 branch Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.