Skip to content

Commit 187b815

Browse files
committed
HSEARCH-4950 Change how nesting/knn checks are performed
1 parent 110e309 commit 187b815

File tree

17 files changed

+180
-104
lines changed

17 files changed

+180
-104
lines changed

backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/search/aggregation/impl/AbstractElasticsearchNestableAggregation.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import org.hibernate.search.backend.elasticsearch.search.common.impl.ElasticsearchSearchIndexScope;
1616
import org.hibernate.search.backend.elasticsearch.search.common.impl.ElasticsearchSearchIndexValueFieldContext;
1717
import org.hibernate.search.backend.elasticsearch.search.predicate.impl.ElasticsearchSearchPredicate;
18+
import org.hibernate.search.backend.elasticsearch.search.predicate.impl.PredicateNestingContext;
1819
import org.hibernate.search.backend.elasticsearch.search.predicate.impl.PredicateRequestContext;
1920
import org.hibernate.search.engine.search.predicate.SearchPredicate;
2021
import org.hibernate.search.util.common.logging.impl.LoggerFactory;
@@ -132,7 +133,8 @@ public void filter(SearchPredicate filter) {
132133
throw log.cannotFilterAggregationOnRootDocumentField( field.absolutePath(), field.eventContext() );
133134
}
134135
ElasticsearchSearchPredicate elasticsearchFilter = ElasticsearchSearchPredicate.from( scope, filter );
135-
elasticsearchFilter.checkNestableWithin( nestedPathHierarchy.get( nestedPathHierarchy.size() - 1 ) );
136+
elasticsearchFilter.checkNestableWithin(
137+
PredicateNestingContext.nested( nestedPathHierarchy.get( nestedPathHierarchy.size() - 1 ) ) );
136138
this.filter = elasticsearchFilter;
137139
}
138140

backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/search/predicate/impl/AbstractElasticsearchNestablePredicate.java

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,11 @@ public abstract class AbstractElasticsearchNestablePredicate extends AbstractEla
2323
super( builder );
2424
}
2525

26+
2627
@Override
27-
public void checkNestableWithin(String expectedParentNestedPath) {
28+
public void doCheckNestableWithin(PredicateNestingContext context) {
2829
List<String> nestedPathHierarchy = getNestedPathHierarchy();
30+
String expectedParentNestedPath = context.getNestedPath();
2931

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

3638
@Override
37-
public JsonObject buildJsonQuery(PredicateRequestContext context) {
39+
public JsonObject toJsonQuery(PredicateRequestContext context) {
3840
List<String> nestedPathHierarchy = getNestedPathHierarchy();
3941
String expectedNestedPath = nestedPathHierarchy.isEmpty()
4042
? null
4143
: nestedPathHierarchy.get( nestedPathHierarchy.size() - 1 );
4244

4345
if ( Objects.equals( context.getNestedPath(), expectedNestedPath ) ) {
4446
// Implicit nesting is not necessary
45-
checkNestableWithin( context.getNestedPath() );
46-
return super.buildJsonQuery( context );
47+
return super.toJsonQuery( context );
4748
}
4849

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

56-
JsonObject result = super.buildJsonQuery( contextAfterImplicitNesting );
56+
JsonObject result = super.toJsonQuery( contextAfterImplicitNesting );
5757

5858
// traversing the nestedPathHierarchy in the inverted order
5959
int hierarchyLastIndex = nestedPathHierarchy.size() - 1;

backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/search/predicate/impl/AbstractElasticsearchPredicate.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ public Set<String> indexNames() {
3737
}
3838

3939
@Override
40-
public JsonObject buildJsonQuery(PredicateRequestContext context) {
40+
public JsonObject toJsonQuery(PredicateRequestContext context) {
4141
JsonObject outerObject = new JsonObject();
4242
JsonObject innerObject = new JsonObject();
4343

backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/search/predicate/impl/ElasticsearchBooleanPredicate.java

Lines changed: 24 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -64,11 +64,11 @@ private ElasticsearchBooleanPredicate(Builder builder) {
6464
}
6565

6666
@Override
67-
public void checkNestableWithin(String expectedParentNestedPath) {
68-
checkNestableWithin( expectedParentNestedPath, mustClauses );
69-
checkNestableWithin( expectedParentNestedPath, shouldClauses );
70-
checkNestableWithin( expectedParentNestedPath, filterClauses );
71-
checkNestableWithin( expectedParentNestedPath, mustNotClauses );
67+
public void doCheckNestableWithin(PredicateNestingContext context) {
68+
checkAcceptableWithin( context, mustClauses );
69+
checkAcceptableWithin( context, shouldClauses );
70+
checkAcceptableWithin( context, filterClauses );
71+
checkAcceptableWithin( context, mustNotClauses );
7272
}
7373

7474
@Override
@@ -111,27 +111,23 @@ private void contributeClauses(PredicateRequestContext context, JsonObject inner
111111

112112
for ( ElasticsearchSearchPredicate clause : clauses ) {
113113
JsonObject jsonQuery = clause.toJsonQuery( context );
114-
if ( jsonQuery == null ) {
115-
if ( !SHOULD_PROPERTY_NAME.equals( occurProperty ) ) {
116-
throw log.cannotAddKnnClauseAtThisStep();
117-
}
114+
if ( jsonQuery != null ) {
118115
// This is an exceptional case for a KNN search on Elasticsearch distribution.
119-
// A Knn predicate would contribute to a knn clause inside the request context itself
120-
// and we do not want to add this json as a clause to the bool predicate.
121-
// So the predicate returns null as JSON query and we ignore it.
122-
}
123-
else {
116+
// A Knn predicate would contribute to a knn clause inside the request context itself,
117+
// and we do not want to add this json as a clause to the bool predicate.
118+
// Hence, when the predicate returns null as JSON query and we ignore it.
119+
124120
GsonUtils.setOrAppendToArray( innerObject, occurProperty, jsonQuery );
125121
}
126122
}
127123
}
128124

129-
private void checkNestableWithin(String expectedParentNestedPath, List<ElasticsearchSearchPredicate> clauses) {
125+
private void checkAcceptableWithin(PredicateNestingContext context, List<ElasticsearchSearchPredicate> clauses) {
130126
if ( clauses == null ) {
131127
return;
132128
}
133129
for ( ElasticsearchSearchPredicate clause : clauses ) {
134-
clause.checkNestableWithin( expectedParentNestedPath );
130+
clause.checkNestableWithin( context );
135131
}
136132
}
137133

@@ -218,31 +214,39 @@ public void must(SearchPredicate clause) {
218214
if ( mustClauses == null ) {
219215
mustClauses = new ArrayList<>();
220216
}
221-
mustClauses.add( ElasticsearchSearchPredicate.from( scope, clause ) );
217+
ElasticsearchSearchPredicate elasticsearchClause = ElasticsearchSearchPredicate.from( scope, clause );
218+
elasticsearchClause.checkNestableWithin( PredicateNestingContext.doesNotAcceptKnn() );
219+
mustClauses.add( elasticsearchClause );
222220
}
223221

224222
@Override
225223
public void mustNot(SearchPredicate clause) {
226224
if ( mustNotClauses == null ) {
227225
mustNotClauses = new ArrayList<>();
228226
}
229-
mustNotClauses.add( ElasticsearchSearchPredicate.from( scope, clause ) );
227+
ElasticsearchSearchPredicate elasticsearchClause = ElasticsearchSearchPredicate.from( scope, clause );
228+
elasticsearchClause.checkNestableWithin( PredicateNestingContext.doesNotAcceptKnn() );
229+
mustNotClauses.add( elasticsearchClause );
230230
}
231231

232232
@Override
233233
public void should(SearchPredicate clause) {
234234
if ( shouldClauses == null ) {
235235
shouldClauses = new ArrayList<>();
236236
}
237-
shouldClauses.add( ElasticsearchSearchPredicate.from( scope, clause ) );
237+
ElasticsearchSearchPredicate elasticsearchClause = ElasticsearchSearchPredicate.from( scope, clause );
238+
elasticsearchClause.checkNestableWithin( PredicateNestingContext.acceptsKnn() );
239+
shouldClauses.add( elasticsearchClause );
238240
}
239241

240242
@Override
241243
public void filter(SearchPredicate clause) {
242244
if ( filterClauses == null ) {
243245
filterClauses = new ArrayList<>();
244246
}
245-
filterClauses.add( ElasticsearchSearchPredicate.from( scope, clause ) );
247+
ElasticsearchSearchPredicate elasticsearchClause = ElasticsearchSearchPredicate.from( scope, clause );
248+
elasticsearchClause.checkNestableWithin( PredicateNestingContext.doesNotAcceptKnn() );
249+
filterClauses.add( elasticsearchClause );
246250
}
247251

248252
@Override

backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/search/predicate/impl/ElasticsearchKnnPredicate.java

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,9 @@ public void vector(Object vector) {
119119

120120
@Override
121121
public void filter(SearchPredicate filter) {
122-
this.filter = ElasticsearchSearchPredicate.from( scope, filter );
122+
ElasticsearchSearchPredicate elasticsearchFilter = ElasticsearchSearchPredicate.from( scope, filter );
123+
elasticsearchFilter.checkNestableWithin( PredicateNestingContext.doesNotAcceptKnn() );
124+
this.filter = elasticsearchFilter;
123125
}
124126

125127
}
@@ -157,9 +159,9 @@ private ElasticsearchImpl(Builder<?> builder) {
157159
}
158160

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

183185
@Override
184-
public void checkNestableWithin(String expectedParentNestedPath) {
185-
if ( expectedParentNestedPath != null ) {
186+
public void doCheckNestableWithin(PredicateNestingContext context) {
187+
if ( context.getNestedPath() != null || !context.acceptsKnnClause() ) {
186188
throw log.cannotAddKnnClauseAtThisStep();
187189
}
188190
}

backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/search/predicate/impl/ElasticsearchMatchAllPredicate.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ private ElasticsearchMatchAllPredicate(Builder builder) {
2323
}
2424

2525
@Override
26-
public void checkNestableWithin(String expectedParentNestedPath) {
26+
public void doCheckNestableWithin(PredicateNestingContext context) {
2727
// Nothing to do
2828
}
2929

backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/search/predicate/impl/ElasticsearchMatchIdPredicate.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ private ElasticsearchMatchIdPredicate(Builder builder) {
5959
}
6060

6161
@Override
62-
public void checkNestableWithin(String expectedParentNestedPath) {
62+
public void doCheckNestableWithin(PredicateNestingContext context) {
6363
// Nothing to do
6464
}
6565

backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/search/predicate/impl/ElasticsearchMatchNonePredicate.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ class ElasticsearchMatchNonePredicate extends AbstractElasticsearchPredicate {
2424
}
2525

2626
@Override
27-
public void checkNestableWithin(String expectedParentNestedPath) {
27+
public void doCheckNestableWithin(PredicateNestingContext context) {
2828
// Nothing to do
2929
}
3030

backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/search/predicate/impl/ElasticsearchNamedPredicate.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,9 @@ private ElasticsearchNamedPredicate(Builder builder, ElasticsearchSearchPredicat
3838
}
3939

4040
@Override
41-
public void checkNestableWithin(String expectedParentNestedPath) {
42-
providedPredicate.checkNestableWithin( expectedParentNestedPath );
43-
super.checkNestableWithin( expectedParentNestedPath );
41+
public void doCheckNestableWithin(PredicateNestingContext context) {
42+
providedPredicate.checkNestableWithin( context );
43+
super.checkNestableWithin( context );
4444
}
4545

4646
@Override

backend/elasticsearch/src/main/java/org/hibernate/search/backend/elasticsearch/search/predicate/impl/ElasticsearchNestedPredicate.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ private static class Builder extends AbstractBuilder implements NestedPredicateB
7777
public void nested(SearchPredicate nestedPredicate) {
7878
ElasticsearchSearchPredicate elasticsearchPredicate = ElasticsearchSearchPredicate.from(
7979
scope, nestedPredicate );
80-
elasticsearchPredicate.checkNestableWithin( absoluteFieldPath );
80+
elasticsearchPredicate.checkNestableWithin( PredicateNestingContext.nested( absoluteFieldPath ) );
8181
this.nestedPredicate = elasticsearchPredicate;
8282
}
8383

0 commit comments

Comments
 (0)