Skip to content

Commit 8d679a1

Browse files
cmarkcmark
committed
feat(index): reduce must/filter clauses to single expression...
...by using the intersection of the specified values
1 parent 8b9c489 commit 8d679a1

File tree

2 files changed

+87
-14
lines changed

2 files changed

+87
-14
lines changed

‎commons/com.b2international.index.tests/src/com/b2international/index/BoolQueryTest.java

+30-2
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ public void deepFilterOnlyBooleanQueryShouldBeFlattenedToPreventError() throws E
5353
}
5454

5555
@Test
56-
public void mergeMultipleSingleTermShouldClauses() throws Exception {
56+
public void mergeSingleTermShouldClauses() throws Exception {
5757
String id1 = UUID.randomUUID().toString();
5858
String id2 = UUID.randomUUID().toString();
5959
String id3 = UUID.randomUUID().toString();
@@ -66,7 +66,7 @@ public void mergeMultipleSingleTermShouldClauses() throws Exception {
6666
}
6767

6868
@Test
69-
public void mergeMultipleSingleAndMultiTermShouldClauses() throws Exception {
69+
public void mergeSingleAndMultiTermShouldClauses() throws Exception {
7070
String id1 = UUID.randomUUID().toString();
7171
String id2 = UUID.randomUUID().toString();
7272
String id3 = UUID.randomUUID().toString();
@@ -78,6 +78,34 @@ public void mergeMultipleSingleAndMultiTermShouldClauses() throws Exception {
7878
.build();
7979
assertEquals(Expressions.matchAny("id", Set.of(id1, id2, id3, id4)), actual);
8080
}
81+
82+
@Test
83+
public void mergeSingleAndMultiTermFilterClauses() throws Exception {
84+
String id1 = UUID.randomUUID().toString();
85+
String id2 = UUID.randomUUID().toString();
86+
String id3 = UUID.randomUUID().toString();
87+
String id4 = UUID.randomUUID().toString();
88+
Expression actual = Expressions.bool()
89+
.filter(Expressions.exactMatch("id", id1))
90+
.filter(Expressions.matchAny("id", Set.of(id1, id2)))
91+
.filter(Expressions.matchAny("id", Set.of(id1, id3, id4)))
92+
.build();
93+
assertEquals(Expressions.exactMatch("id", id1), actual);
94+
}
95+
96+
@Test
97+
public void mergeDisjunctTermFilterClauses() throws Exception {
98+
String id1 = UUID.randomUUID().toString();
99+
String id2 = UUID.randomUUID().toString();
100+
String id3 = UUID.randomUUID().toString();
101+
String id4 = UUID.randomUUID().toString();
102+
Expression actual = Expressions.bool()
103+
.filter(Expressions.exactMatch("id", id1))
104+
.filter(Expressions.matchAny("id", Set.of(id2, id3)))
105+
.filter(Expressions.matchAny("id", Set.of(id3, id4)))
106+
.build();
107+
assertEquals(Expressions.matchNone(), actual);
108+
}
81109

82110
private Expression generateDeepBooleanClause(int depth, BiFunction<ExpressionBuilder, Expression, ExpressionBuilder> boolClause) {
83111
ExpressionBuilder root = Expressions.bool();

‎commons/com.b2international.index/src/com/b2international/index/query/AbstractExpressionBuilder.java

+57-12
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,7 @@
1515
*/
1616
package com.b2international.index.query;
1717

18-
import static com.google.common.collect.Lists.newArrayList;
19-
18+
import java.util.ArrayList;
2019
import java.util.Collection;
2120
import java.util.List;
2221
import java.util.Set;
@@ -32,10 +31,10 @@
3231
*/
3332
public abstract class AbstractExpressionBuilder<B extends AbstractExpressionBuilder<B>>{
3433

35-
protected final List<Expression> mustClauses = newArrayList();
36-
protected final List<Expression> mustNotClauses = newArrayList();
37-
protected final List<Expression> shouldClauses = newArrayList();
38-
protected final List<Expression> filterClauses = newArrayList();
34+
protected final List<Expression> mustClauses = new ArrayList<>(1);
35+
protected final List<Expression> mustNotClauses = new ArrayList<>(1);
36+
protected final List<Expression> shouldClauses = new ArrayList<>(3);
37+
protected final List<Expression> filterClauses = new ArrayList<>(3);
3938
protected int minShouldMatch = 1;
4039

4140
protected AbstractExpressionBuilder() {}
@@ -102,9 +101,15 @@ public Expression build() {
102101
return shouldClauses.get(0);
103102
}
104103

105-
final BoolExpression be = new BoolExpression(mustClauses, mustNotClauses, shouldClauses, filterClauses);
106-
be.setMinShouldMatch(minShouldMatch);
107-
return be;
104+
// if after the optimization the resulting bool clauses are empty, then return a MatchNone expression
105+
if (mustClauses.isEmpty() && mustNotClauses.isEmpty() && shouldClauses.isEmpty() && filterClauses.isEmpty()) {
106+
return Expressions.matchNone();
107+
} else {
108+
// otherwise create the bool expression as usual
109+
final BoolExpression be = new BoolExpression(mustClauses, mustNotClauses, shouldClauses, filterClauses);
110+
be.setMinShouldMatch(minShouldMatch);
111+
return be;
112+
}
108113
}
109114
}
110115

@@ -133,11 +138,50 @@ protected final void flattenShoulds() {
133138

134139
protected final void mergeTermFilters() {
135140
// check each mustNot and should clause list and merge term/terms queries into a single terms query, targeting the same field
136-
// XXX merging must/filter queries will change the boolean logic from AND to OR which leads to incorrect results
137141
mergeTermFilters(mustNotClauses);
138142
mergeTermFilters(shouldClauses);
143+
// XXX merging must/filter queries will change the boolean logic from AND to OR which leads to incorrect results
144+
// instead calculate the intersection of the values and use that for the expressions
145+
reduceTermFilters(mustClauses);
146+
reduceTermFilters(filterClauses);
139147
}
140148

149+
private void reduceTermFilters(List<Expression> clauses) {
150+
Multimap<String, Expression> termExpressionsByField = HashMultimap.create();
151+
for (Expression expression : List.copyOf(clauses)) {
152+
if (expression instanceof SingleArgumentPredicate<?>) {
153+
termExpressionsByField.put(((SingleArgumentPredicate<?>) expression).getField(), expression);
154+
} else if (expression instanceof SetPredicate<?>) {
155+
termExpressionsByField.put(((SetPredicate<?>) expression).getField(), expression);
156+
}
157+
}
158+
159+
for (String field : Set.copyOf(termExpressionsByField.keySet())) {
160+
Collection<Expression> termExpressions = termExpressionsByField.removeAll(field);
161+
if (termExpressions.size() > 1) {
162+
Set<Object> values = null;
163+
for (Expression expression : termExpressions) {
164+
if (values != null && values.isEmpty()) {
165+
break;
166+
}
167+
Set<Object> expressionValues;
168+
if (expression instanceof SingleArgumentPredicate<?>) {
169+
expressionValues = Set.of(((SingleArgumentPredicate<?>) expression).getArgument());
170+
} else if (expression instanceof SetPredicate<?>) {
171+
expressionValues = Set.copyOf(((SetPredicate<?>) expression).values());
172+
} else {
173+
throw new IllegalStateException("Invalid clause detected when processing term/terms clauses: " + expression);
174+
}
175+
values = values == null ? expressionValues : Sets.intersection(values, expressionValues);
176+
}
177+
// remove all matching clauses first
178+
clauses.removeAll(termExpressions);
179+
// add the new merged expression
180+
clauses.add(Expressions.matchAnyObject(field, values));
181+
}
182+
}
183+
}
184+
141185
private void mergeTermFilters(List<Expression> clauses) {
142186
Multimap<String, Expression> termExpressionsByField = HashMultimap.create();
143187
for (Expression expression : List.copyOf(clauses)) {
@@ -159,9 +203,10 @@ private void mergeTermFilters(List<Expression> clauses) {
159203
values.addAll(((SetPredicate<?>) expression).values());
160204
}
161205
}
162-
// replace all clauses with a single expression
163-
clauses.add(Expressions.matchAnyObject(field, values));
206+
// remove all matching clauses first
164207
clauses.removeAll(termExpressions);
208+
// add the new merged expression
209+
clauses.add(Expressions.matchAnyObject(field, values));
165210
}
166211
}
167212
}

0 commit comments

Comments
 (0)