Skip to content

Commit

Permalink
HSEARCH-4950 Change how nesting/knn checks are performed
Browse files Browse the repository at this point in the history
  • Loading branch information
marko-bekhta committed Jan 12, 2024
1 parent fee3690 commit c36fb12
Show file tree
Hide file tree
Showing 17 changed files with 177 additions and 97 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import org.hibernate.search.backend.elasticsearch.search.common.impl.ElasticsearchSearchIndexScope;
import org.hibernate.search.backend.elasticsearch.search.common.impl.ElasticsearchSearchIndexValueFieldContext;
import org.hibernate.search.backend.elasticsearch.search.predicate.impl.ElasticsearchSearchPredicate;
import org.hibernate.search.backend.elasticsearch.search.predicate.impl.PredicateNestingContext;
import org.hibernate.search.backend.elasticsearch.search.predicate.impl.PredicateRequestContext;
import org.hibernate.search.engine.search.predicate.SearchPredicate;
import org.hibernate.search.util.common.logging.impl.LoggerFactory;
Expand Down Expand Up @@ -132,7 +133,8 @@ public void filter(SearchPredicate filter) {
throw log.cannotFilterAggregationOnRootDocumentField( field.absolutePath(), field.eventContext() );
}
ElasticsearchSearchPredicate elasticsearchFilter = ElasticsearchSearchPredicate.from( scope, filter );
elasticsearchFilter.checkNestableWithin( nestedPathHierarchy.get( nestedPathHierarchy.size() - 1 ) );
elasticsearchFilter.checkNestableWithin(
PredicateNestingContext.nested( nestedPathHierarchy.get( nestedPathHierarchy.size() - 1 ) ) );
this.filter = elasticsearchFilter;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,11 @@ public abstract class AbstractElasticsearchNestablePredicate extends AbstractEla
super( builder );
}


@Override
public void checkNestableWithin(String expectedParentNestedPath) {
public void doCheckNestableWithin(PredicateNestingContext context) {
List<String> nestedPathHierarchy = getNestedPathHierarchy();
String expectedParentNestedPath = context.getNestedPath();

if ( expectedParentNestedPath != null && !nestedPathHierarchy.contains( expectedParentNestedPath ) ) {
throw log.invalidNestedObjectPathForPredicate( this, expectedParentNestedPath,
Expand All @@ -34,26 +36,24 @@ public void checkNestableWithin(String expectedParentNestedPath) {
}

@Override
public JsonObject buildJsonQuery(PredicateRequestContext context) {
public JsonObject toJsonQuery(PredicateRequestContext context) {
List<String> nestedPathHierarchy = getNestedPathHierarchy();
String expectedNestedPath = nestedPathHierarchy.isEmpty()
? null
: nestedPathHierarchy.get( nestedPathHierarchy.size() - 1 );

if ( Objects.equals( context.getNestedPath(), expectedNestedPath ) ) {
// Implicit nesting is not necessary
checkNestableWithin( context.getNestedPath() );
return super.buildJsonQuery( context );
return super.toJsonQuery( context );
}

// The context we expect this predicate to be built in.
// We'll make sure to wrap it in nested predicates as appropriate in the next few lines,
// so that the predicate is actually executed in this context.
PredicateRequestContext contextAfterImplicitNesting =
context.withNestedPath( expectedNestedPath );
checkNestableWithin( expectedNestedPath );

JsonObject result = super.buildJsonQuery( contextAfterImplicitNesting );
JsonObject result = super.toJsonQuery( contextAfterImplicitNesting );

// traversing the nestedPathHierarchy in the inverted order
int hierarchyLastIndex = nestedPathHierarchy.size() - 1;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public Set<String> indexNames() {
}

@Override
public JsonObject buildJsonQuery(PredicateRequestContext context) {
public JsonObject toJsonQuery(PredicateRequestContext context) {
JsonObject outerObject = new JsonObject();
JsonObject innerObject = new JsonObject();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,11 @@ private ElasticsearchBooleanPredicate(Builder builder) {
}

@Override
public void checkNestableWithin(String expectedParentNestedPath) {
checkNestableWithin( expectedParentNestedPath, mustClauses );
checkNestableWithin( expectedParentNestedPath, shouldClauses );
checkNestableWithin( expectedParentNestedPath, filterClauses );
checkNestableWithin( expectedParentNestedPath, mustNotClauses );
public void doCheckNestableWithin(PredicateNestingContext context) {
checkAcceptableWithin( context, mustClauses );
checkAcceptableWithin( context, shouldClauses );
checkAcceptableWithin( context, filterClauses );
checkAcceptableWithin( context, mustNotClauses );
}

@Override
Expand Down Expand Up @@ -126,12 +126,12 @@ private void contributeClauses(PredicateRequestContext context, JsonObject inner
}
}

private void checkNestableWithin(String expectedParentNestedPath, List<ElasticsearchSearchPredicate> clauses) {
private void checkAcceptableWithin(PredicateNestingContext context, List<ElasticsearchSearchPredicate> clauses) {
if ( clauses == null ) {
return;
}
for ( ElasticsearchSearchPredicate clause : clauses ) {
clause.checkNestableWithin( expectedParentNestedPath );
clause.checkNestableWithin( context );
}
}

Expand Down Expand Up @@ -218,31 +218,39 @@ public void must(SearchPredicate clause) {
if ( mustClauses == null ) {
mustClauses = new ArrayList<>();
}
mustClauses.add( ElasticsearchSearchPredicate.from( scope, clause ) );
ElasticsearchSearchPredicate elasticsearchClause = ElasticsearchSearchPredicate.from( scope, clause );
elasticsearchClause.checkNestableWithin( PredicateNestingContext.doesNotAcceptKnn() );
mustClauses.add( elasticsearchClause );
}

@Override
public void mustNot(SearchPredicate clause) {
if ( mustNotClauses == null ) {
mustNotClauses = new ArrayList<>();
}
mustNotClauses.add( ElasticsearchSearchPredicate.from( scope, clause ) );
ElasticsearchSearchPredicate elasticsearchClause = ElasticsearchSearchPredicate.from( scope, clause );
elasticsearchClause.checkNestableWithin( PredicateNestingContext.doesNotAcceptKnn() );
mustNotClauses.add( elasticsearchClause );
}

@Override
public void should(SearchPredicate clause) {
if ( shouldClauses == null ) {
shouldClauses = new ArrayList<>();
}
shouldClauses.add( ElasticsearchSearchPredicate.from( scope, clause ) );
ElasticsearchSearchPredicate elasticsearchClause = ElasticsearchSearchPredicate.from( scope, clause );
elasticsearchClause.checkNestableWithin( PredicateNestingContext.acceptsKnn() );
shouldClauses.add( elasticsearchClause );
}

@Override
public void filter(SearchPredicate clause) {
if ( filterClauses == null ) {
filterClauses = new ArrayList<>();
}
filterClauses.add( ElasticsearchSearchPredicate.from( scope, clause ) );
ElasticsearchSearchPredicate elasticsearchClause = ElasticsearchSearchPredicate.from( scope, clause );
elasticsearchClause.checkNestableWithin( PredicateNestingContext.doesNotAcceptKnn() );
filterClauses.add( elasticsearchClause );
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,9 @@ public void vector(Object vector) {

@Override
public void filter(SearchPredicate filter) {
this.filter = ElasticsearchSearchPredicate.from( scope, filter );
ElasticsearchSearchPredicate elasticsearchFilter = ElasticsearchSearchPredicate.from( scope, filter );
elasticsearchFilter.checkNestableWithin( PredicateNestingContext.doesNotAcceptKnn() );
this.filter = elasticsearchFilter;
}

}
Expand Down Expand Up @@ -157,9 +159,9 @@ private ElasticsearchImpl(Builder<?> builder) {
}

@Override
public JsonObject buildJsonQuery(PredicateRequestContext context) {
public JsonObject toJsonQuery(PredicateRequestContext context) {
// we want the query to get created and passed to the request context
context.contributeKnnClause( ( super.buildJsonQuery( context ) ) );
context.contributeKnnClause( ( super.toJsonQuery( context ) ) );
// but we don't want it to be an actual query so we return `null`:
return null;
}
Expand All @@ -181,8 +183,8 @@ protected JsonObject doToJsonQuery(PredicateRequestContext context, JsonObject o
}

@Override
public void checkNestableWithin(String expectedParentNestedPath) {
if ( expectedParentNestedPath != null ) {
public void doCheckNestableWithin(PredicateNestingContext context) {
if ( context.getNestedPath() != null || !context.acceptsKnnClause() ) {
throw log.cannotAddKnnClauseAtThisStep();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ private ElasticsearchMatchAllPredicate(Builder builder) {
}

@Override
public void checkNestableWithin(String expectedParentNestedPath) {
public void doCheckNestableWithin(PredicateNestingContext context) {
// Nothing to do
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ private ElasticsearchMatchIdPredicate(Builder builder) {
}

@Override
public void checkNestableWithin(String expectedParentNestedPath) {
public void doCheckNestableWithin(PredicateNestingContext context) {
// Nothing to do
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class ElasticsearchMatchNonePredicate extends AbstractElasticsearchPredicate {
}

@Override
public void checkNestableWithin(String expectedParentNestedPath) {
public void doCheckNestableWithin(PredicateNestingContext context) {
// Nothing to do
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ private ElasticsearchNamedPredicate(Builder builder, ElasticsearchSearchPredicat
}

@Override
public void checkNestableWithin(String expectedParentNestedPath) {
providedPredicate.checkNestableWithin( expectedParentNestedPath );
super.checkNestableWithin( expectedParentNestedPath );
public void doCheckNestableWithin(PredicateNestingContext context) {
providedPredicate.checkNestableWithin( context );
super.checkNestableWithin( context );
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ private static class Builder extends AbstractBuilder implements NestedPredicateB
public void nested(SearchPredicate nestedPredicate) {
ElasticsearchSearchPredicate elasticsearchPredicate = ElasticsearchSearchPredicate.from(
scope, nestedPredicate );
elasticsearchPredicate.checkNestableWithin( absoluteFieldPath );
elasticsearchPredicate.checkNestableWithin( PredicateNestingContext.nested( absoluteFieldPath ) );
this.nestedPredicate = elasticsearchPredicate;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@ public abstract class ElasticsearchSearchPredicate implements SearchPredicate {

public abstract Set<String> indexNames();

public abstract void checkNestableWithin(String expectedParentNestedPath);

public final JsonObject toJsonQuery(PredicateRequestContext context) {
return buildJsonQuery( context.predicateContext( this ) );
public final void checkNestableWithin(PredicateNestingContext context) {
doCheckNestableWithin( context.wrap( this ) );
}

public abstract JsonObject buildJsonQuery(PredicateRequestContext context);
protected abstract void doCheckNestableWithin(PredicateNestingContext context);

public abstract JsonObject toJsonQuery(PredicateRequestContext context);

public static ElasticsearchSearchPredicate from(ElasticsearchSearchIndexScope<?> scope, SearchPredicate predicate) {
if ( !( predicate instanceof ElasticsearchSearchPredicate ) ) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,12 @@ public Set<String> indexNames() {
}

@Override
public void checkNestableWithin(String expectedParentNestedPath) {
public void doCheckNestableWithin(PredicateNestingContext context) {
// Nothing to do: we'll assume the user knows what they are doing.
}

@Override
public JsonObject buildJsonQuery(PredicateRequestContext context) {
public JsonObject toJsonQuery(PredicateRequestContext context) {
return json;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
* Hibernate Search, full-text search for your domain model
*
* License: GNU Lesser General Public License (LGPL), version 2.1 or later
* See the lgpl.txt file in the root directory or <http://www.gnu.org/licenses/lgpl-2.1.html>.
*/
package org.hibernate.search.backend.elasticsearch.search.predicate.impl;

public class PredicateNestingContext {
private static final PredicateNestingContext ACCEPTS_KNN = new PredicateNestingContext( true );
private static final PredicateNestingContext DOES_NOT_ACCEPT_KNN = new PredicateNestingContext( false );
private final String nestedPath;
private final boolean acceptsKnnClause;

private final Class<? extends ElasticsearchSearchPredicate> predicateType;

public static PredicateNestingContext acceptsKnn() {
return ACCEPTS_KNN;
}

public static PredicateNestingContext doesNotAcceptKnn() {
return DOES_NOT_ACCEPT_KNN;
}

public static PredicateNestingContext nested(String nestedPath) {
return new PredicateNestingContext( nestedPath );
}

private PredicateNestingContext(String nestedPath, boolean acceptsKnnClause) {
this( nestedPath, acceptsKnnClause, null );
}

private PredicateNestingContext(String nestedPath, boolean acceptsKnnClause,
Class<? extends ElasticsearchSearchPredicate> predicateType) {
this.nestedPath = nestedPath;
this.acceptsKnnClause = acceptsKnnClause;
this.predicateType = predicateType;
}

private PredicateNestingContext(String nestedPath) {
this( nestedPath, false );
}

private PredicateNestingContext(boolean acceptsKnnClause) {
this( null, acceptsKnnClause );
}

public String getNestedPath() {
return nestedPath;
}

public boolean acceptsKnnClause() {
return acceptsKnnClause;
}

public PredicateNestingContext wrap(ElasticsearchSearchPredicate elasticsearchSearchPredicate) {
return new PredicateNestingContext(
nestedPath,
acceptsKnnClause && this.predicateType == null,
elasticsearchSearchPredicate.getClass()
);
}
}
Loading

0 comments on commit c36fb12

Please sign in to comment.