Skip to content

Commit 65ecf2d

Browse files
astefanastefan
committed
ESQL: Use a must boolean statement when pushing down to Lucene when scoring is also needed (elastic#124001)
* Use a "must" instead of "filter" when building the pushed down filter AND when scoring is needed
1 parent 6343545 commit 65ecf2d

File tree

8 files changed

+159
-19
lines changed

8 files changed

+159
-19
lines changed

‎docs/changelog/124001.yaml‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
pr: 124001
2+
summary: Use a must boolean statement when pushing down to Lucene when scoring is
3+
also needed
4+
area: ES|QL
5+
type: bug
6+
issues:
7+
- 123967

‎x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/MetadataAttribute.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
import static org.elasticsearch.core.Tuple.tuple;
3030

3131
public class MetadataAttribute extends TypedAttribute {
32-
public static final String TIMESTAMP_FIELD = "@timestamp";
32+
public static final String TIMESTAMP_FIELD = "@timestamp"; // this is not a true metadata attribute
3333
public static final String TSID_FIELD = "_tsid";
3434
public static final String SCORE = "_score";
3535
public static final String INDEX = "_index";

‎x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/plugin/MatchOperatorIT.java‎

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,17 @@
1111
import org.elasticsearch.action.index.IndexRequest;
1212
import org.elasticsearch.action.support.WriteRequest;
1313
import org.elasticsearch.common.settings.Settings;
14+
import org.elasticsearch.index.query.QueryBuilder;
15+
import org.elasticsearch.index.query.QueryBuilders;
1416
import org.elasticsearch.xpack.esql.VerificationException;
1517
import org.elasticsearch.xpack.esql.action.AbstractEsqlIntegTestCase;
1618
import org.elasticsearch.xpack.esql.action.EsqlCapabilities;
1719
import org.junit.Before;
1820

1921
import java.util.List;
2022

23+
import static org.elasticsearch.index.query.QueryBuilders.boolQuery;
24+
import static org.elasticsearch.index.query.QueryBuilders.matchQuery;
2125
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
2226
import static org.hamcrest.CoreMatchers.containsString;
2327

@@ -122,6 +126,119 @@ public void testWhereMatchWithScoring() {
122126
}
123127
}
124128

129+
/**
130+
* Test for https://github.com/elastic/elasticsearch/issues/123967
131+
*/
132+
public void testWhereMatchWithScoring_AndRequestFilter() {
133+
var query = """
134+
FROM test METADATA _score
135+
| WHERE content:"fox"
136+
| SORT _score DESC
137+
| KEEP content, _score
138+
""";
139+
140+
QueryBuilder filter = boolQuery().must(matchQuery("content", "brown"));
141+
142+
try (var resp = run(query, randomPragmas(), filter)) {
143+
assertColumnNames(resp.columns(), List.of("content", "_score"));
144+
assertColumnTypes(resp.columns(), List.of("text", "double"));
145+
assertValues(
146+
resp.values(),
147+
List.of(
148+
List.of("This is a brown fox", 1.4274532794952393),
149+
List.of("The quick brown fox jumps over the lazy dog", 1.1248724460601807)
150+
)
151+
);
152+
}
153+
}
154+
155+
public void testWhereMatchWithScoring_AndNoScoreRequestFilter() {
156+
var query = """
157+
FROM test METADATA _score
158+
| WHERE content:"fox"
159+
| SORT _score DESC
160+
| KEEP content, _score
161+
""";
162+
163+
QueryBuilder filter = boolQuery().filter(matchQuery("content", "brown"));
164+
165+
try (var resp = run(query, randomPragmas(), filter)) {
166+
assertColumnNames(resp.columns(), List.of("content", "_score"));
167+
assertColumnTypes(resp.columns(), List.of("text", "double"));
168+
assertValues(
169+
resp.values(),
170+
List.of(
171+
List.of("This is a brown fox", 1.156558871269226),
172+
List.of("The quick brown fox jumps over the lazy dog", 0.9114001989364624)
173+
)
174+
);
175+
}
176+
}
177+
178+
public void testWhereMatchWithScoring_And_MatchAllRequestFilter() {
179+
var query = """
180+
FROM test METADATA _score
181+
| WHERE content:"fox"
182+
| SORT _score DESC
183+
| KEEP content, _score
184+
""";
185+
186+
QueryBuilder filter = QueryBuilders.matchAllQuery();
187+
188+
try (var resp = run(query, randomPragmas(), filter)) {
189+
assertColumnNames(resp.columns(), List.of("content", "_score"));
190+
assertColumnTypes(resp.columns(), List.of("text", "double"));
191+
assertValues(
192+
resp.values(),
193+
List.of(
194+
List.of("This is a brown fox", 2.1565589904785156),
195+
List.of("The quick brown fox jumps over the lazy dog", 1.9114001989364624)
196+
)
197+
);
198+
}
199+
}
200+
201+
public void testScoringOutsideQuery() {
202+
var query = """
203+
FROM test METADATA _score
204+
| SORT _score DESC
205+
| KEEP content, _score
206+
""";
207+
208+
QueryBuilder filter = boolQuery().must(matchQuery("content", "fox"));
209+
210+
try (var resp = run(query, randomPragmas(), filter)) {
211+
assertColumnNames(resp.columns(), List.of("content", "_score"));
212+
assertColumnTypes(resp.columns(), List.of("text", "double"));
213+
assertValues(
214+
resp.values(),
215+
List.of(
216+
List.of("This is a brown fox", 1.156558871269226),
217+
List.of("The quick brown fox jumps over the lazy dog", 0.9114001989364624)
218+
)
219+
);
220+
}
221+
}
222+
223+
public void testScoring_Zero_OutsideQuery() {
224+
var query = """
225+
FROM test METADATA _score
226+
| SORT _score DESC
227+
| KEEP content, _score
228+
""";
229+
230+
QueryBuilder filter = boolQuery().filter(matchQuery("content", "fox"));
231+
232+
try (var resp = run(query, randomPragmas(), filter)) {
233+
assertColumnNames(resp.columns(), List.of("content", "_score"));
234+
assertColumnTypes(resp.columns(), List.of("text", "double"));
235+
assertValues(
236+
resp.values(),
237+
List.of(List.of("This is a brown fox", 0.0), List.of("The quick brown fox jumps over the lazy dog", 0.0))
238+
);
239+
}
240+
}
241+
125242
public void testWhereMatchWithScoringDifferentSort() {
126243
assumeTrue("'METADATA _score' is disabled", EsqlCapabilities.Cap.METADATA_SCORE.isEnabled());
127244
var query = """

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushFiltersToSource.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,8 @@ private static PhysicalPlan rewrite(
101101
if (newPushable.size() > 0) { // update the executable with pushable conditions
102102
Query queryDSL = TRANSLATOR_HANDLER.asQuery(Predicates.combineAnd(newPushable));
103103
QueryBuilder planQuery = queryDSL.asBuilder();
104-
var query = Queries.combine(Queries.Clause.FILTER, asList(queryExec.query(), planQuery));
104+
Queries.Clause combiningQueryClauseType = queryExec.hasScoring() ? Queries.Clause.MUST : Queries.Clause.FILTER;
105+
var query = Queries.combine(combiningQueryClauseType, asList(queryExec.query(), planQuery));
105106
queryExec = new EsQueryExec(
106107
queryExec.source(),
107108
queryExec.indexPattern(),

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/ReplaceSourceAttributes.java‎

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -32,27 +32,36 @@ protected PhysicalPlan rule(EsSourceExec plan) {
3232
var docId = new FieldAttribute(plan.source(), EsQueryExec.DOC_ID_FIELD.getName(), EsQueryExec.DOC_ID_FIELD);
3333
final List<Attribute> attributes = new ArrayList<>();
3434
attributes.add(docId);
35-
if (plan.indexMode() == IndexMode.TIME_SERIES) {
36-
Attribute tsid = null, timestamp = null;
37-
for (Attribute attr : plan.output()) {
38-
String name = attr.name();
39-
if (name.equals(MetadataAttribute.TSID_FIELD)) {
35+
36+
var outputIterator = plan.output().iterator();
37+
var isTimeSeries = plan.indexMode() == IndexMode.TIME_SERIES;
38+
var keepIterating = true;
39+
Attribute tsid = null, timestamp = null, score = null;
40+
41+
while (keepIterating && outputIterator.hasNext()) {
42+
Attribute attr = outputIterator.next();
43+
if (attr instanceof MetadataAttribute ma) {
44+
if (ma.name().equals(MetadataAttribute.SCORE)) {
45+
score = attr;
46+
} else if (isTimeSeries && ma.name().equals(MetadataAttribute.TSID_FIELD)) {
4047
tsid = attr;
41-
} else if (name.equals(MetadataAttribute.TIMESTAMP_FIELD)) {
42-
timestamp = attr;
4348
}
49+
} else if (attr.name().equals(MetadataAttribute.TIMESTAMP_FIELD)) {
50+
timestamp = attr;
4451
}
52+
keepIterating = score == null || (isTimeSeries && (tsid == null || timestamp == null));
53+
}
54+
if (isTimeSeries) {
4555
if (tsid == null || timestamp == null) {
4656
throw new IllegalStateException("_tsid or @timestamp are missing from the time-series source");
4757
}
4858
attributes.add(tsid);
4959
attributes.add(timestamp);
5060
}
51-
plan.output().forEach(attr -> {
52-
if (attr instanceof MetadataAttribute ma && ma.name().equals(MetadataAttribute.SCORE)) {
53-
attributes.add(ma);
54-
}
55-
});
61+
if (score != null) {
62+
attributes.add(score);
63+
}
64+
5665
return new EsQueryExec(plan.source(), plan.indexPattern(), plan.indexMode(), plan.indexNameWithModes(), attributes, plan.query());
5766
}
5867
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsQueryExec.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import org.elasticsearch.xpack.esql.core.expression.Attribute;
2222
import org.elasticsearch.xpack.esql.core.expression.Expression;
2323
import org.elasticsearch.xpack.esql.core.expression.FieldAttribute;
24+
import org.elasticsearch.xpack.esql.core.expression.MetadataAttribute;
2425
import org.elasticsearch.xpack.esql.core.tree.NodeInfo;
2526
import org.elasticsearch.xpack.esql.core.tree.NodeUtils;
2627
import org.elasticsearch.xpack.esql.core.tree.Source;
@@ -204,6 +205,15 @@ public static boolean isSourceAttribute(Attribute attr) {
204205
return DOC_ID_FIELD.getName().equals(attr.name());
205206
}
206207

208+
public boolean hasScoring() {
209+
for (Attribute a : attrs()) {
210+
if (a instanceof MetadataAttribute && a.name().equals(MetadataAttribute.SCORE)) {
211+
return true;
212+
}
213+
}
214+
return false;
215+
}
216+
207217
@Override
208218
protected NodeInfo<EsQueryExec> info() {
209219
return NodeInfo.create(

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsStatsQueryExec.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@ public enum StatsType {
3737
}
3838

3939
public record Stat(String name, StatsType type, QueryBuilder query) {
40-
4140
public QueryBuilder filter(QueryBuilder sourceQuery) {
4241
return query == null ? sourceQuery : Queries.combine(Queries.Clause.FILTER, asList(sourceQuery, query));
4342
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/EsPhysicalOperationProviders.java‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,6 @@
4949
import org.elasticsearch.xpack.esql.core.expression.Expression;
5050
import org.elasticsearch.xpack.esql.core.expression.FieldAttribute;
5151
import org.elasticsearch.xpack.esql.core.expression.FoldContext;
52-
import org.elasticsearch.xpack.esql.core.expression.MetadataAttribute;
5352
import org.elasticsearch.xpack.esql.core.type.DataType;
5453
import org.elasticsearch.xpack.esql.core.type.KeywordEsField;
5554
import org.elasticsearch.xpack.esql.core.type.MultiTypeEsField;
@@ -186,9 +185,7 @@ public final PhysicalOperation sourcePhysicalOperation(EsQueryExec esQueryExec,
186185
assert esQueryExec.estimatedRowSize() != null : "estimated row size not initialized";
187186
int rowEstimatedSize = esQueryExec.estimatedRowSize();
188187
int limit = esQueryExec.limit() != null ? (Integer) esQueryExec.limit().fold(context.foldCtx()) : NO_LIMIT;
189-
boolean scoring = esQueryExec.attrs()
190-
.stream()
191-
.anyMatch(a -> a instanceof MetadataAttribute && a.name().equals(MetadataAttribute.SCORE));
188+
boolean scoring = esQueryExec.hasScoring();
192189
if ((sorts != null && sorts.isEmpty() == false)) {
193190
List<SortBuilder<?>> sortBuilders = new ArrayList<>(sorts.size());
194191
for (Sort sort : sorts) {

0 commit comments

Comments
 (0)