Skip to content

Commit ac2c900

Browse files
astefanastefan
committed
Add tests and revert EsqlSession change
1 parent 24bcede commit ac2c900

File tree

5 files changed

+129
-44
lines changed

5 files changed

+129
-44
lines changed

‎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,12 +11,16 @@
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.junit.Before;
1719

1820
import java.util.List;
1921

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

@@ -120,6 +124,119 @@ public void testWhereMatchWithScoring() {
120124
}
121125
}
122126

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

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

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -101,12 +101,7 @@ 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-
Queries.Clause combiningQueryClauseType;
105-
if (queryExec.hasScoring()) {
106-
combiningQueryClauseType = Queries.Clause.MUST;
107-
} else {
108-
combiningQueryClauseType = Queries.Clause.FILTER;
109-
}
104+
Queries.Clause combiningQueryClauseType = queryExec.hasScoring() ? Queries.Clause.MUST : Queries.Clause.FILTER;
110105
var query = Queries.combine(combiningQueryClauseType, asList(queryExec.query(), planQuery));
111106
queryExec = new EsQueryExec(
112107
queryExec.source(),

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,12 @@ public static boolean isSourceAttribute(Attribute attr) {
206206
}
207207

208208
public boolean hasScoring() {
209-
return attrs().stream().anyMatch(a -> a instanceof MetadataAttribute && a.name().equals(MetadataAttribute.SCORE));
209+
for (Attribute a : attrs()) {
210+
if (a instanceof MetadataAttribute && a.name().equals(MetadataAttribute.SCORE)) {
211+
return true;
212+
}
213+
}
214+
return false;
210215
}
211216

212217
@Override

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java‎

Lines changed: 4 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,6 @@
5959
import org.elasticsearch.xpack.esql.plan.IndexPattern;
6060
import org.elasticsearch.xpack.esql.plan.logical.Aggregate;
6161
import org.elasticsearch.xpack.esql.plan.logical.Enrich;
62-
import org.elasticsearch.xpack.esql.plan.logical.EsRelation;
6362
import org.elasticsearch.xpack.esql.plan.logical.Fork;
6463
import org.elasticsearch.xpack.esql.plan.logical.Keep;
6564
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
@@ -71,7 +70,6 @@
7170
import org.elasticsearch.xpack.esql.plan.logical.join.LookupJoin;
7271
import org.elasticsearch.xpack.esql.plan.logical.local.LocalRelation;
7372
import org.elasticsearch.xpack.esql.plan.logical.local.LocalSupplier;
74-
import org.elasticsearch.xpack.esql.plan.physical.EsQueryExec;
7573
import org.elasticsearch.xpack.esql.plan.physical.EstimatesRowSize;
7674
import org.elasticsearch.xpack.esql.plan.physical.FragmentExec;
7775
import org.elasticsearch.xpack.esql.plan.physical.LocalSourceExec;
@@ -714,46 +712,16 @@ private static Set<String> subfields(Set<String> names) {
714712

715713
private PhysicalPlan logicalPlanToPhysicalPlan(LogicalPlan optimizedPlan, EsqlQueryRequest request) {
716714
PhysicalPlan physicalPlan = optimizedPhysicalPlan(optimizedPlan);
717-
final QueryBuilder filter = request.filter();
718-
final Holder<Boolean> hasScoring = new Holder<>(false);
719-
720-
if (filter != null) {
721-
// a PhysicalPlan completely transformed (no fragments) will have an EsQueryExec in it if the data comes from ES
722-
physicalPlan.forEachDown(EsQueryExec.class, esQueryExec -> {
723-
if (hasScoring.get() == false && esQueryExec.hasScoring()) {
724-
hasScoring.set(true);
725-
}
726-
});
727-
// if there is no EsQueryExec and still scoring is required, search for fragments as well where EsRelations should
728-
// know if there is scoring needed or not
729-
if (hasScoring.get() == false) {
730-
physicalPlan.forEachDown(FragmentExec.class, fragmentExec -> {
731-
fragmentExec.fragment().forEachDown(EsRelation.class, esRelation -> {
732-
if (esRelation.output()
733-
.stream()
734-
.anyMatch(a -> a instanceof MetadataAttribute && a.name().equals(MetadataAttribute.SCORE))) {
735-
hasScoring.set(true);
736-
}
737-
});
738-
});
739-
}
740-
}
741715
physicalPlan = physicalPlan.transformUp(FragmentExec.class, f -> {
716+
QueryBuilder filter = request.filter();
742717
if (filter != null) {
743718
var fragmentFilter = f.esFilter();
744719
// TODO: have an ESFilter and push down to EsQueryExec / EsSource
745720
// This is an ugly hack to push the filter parameter to Lucene
746721
// TODO: filter integration testing
747-
QueryBuilder newFilter;
748-
if (fragmentFilter != null) {
749-
newFilter = hasScoring.get()
750-
? boolQuery().filter(fragmentFilter).must(filter) // a "bool" "must" does influence scoring
751-
: boolQuery().filter(fragmentFilter).filter(filter);// no scoring? then "filter" to completely disable scoring
752-
} else {
753-
newFilter = hasScoring.get() ? filter : boolQuery().filter(filter);
754-
}
755-
LOGGER.debug("Fold filter {} to EsQueryExec", newFilter);
756-
f = f.withFilter(newFilter);
722+
filter = fragmentFilter != null ? boolQuery().filter(fragmentFilter).must(filter) : filter;
723+
LOGGER.debug("Fold filter {} to EsQueryExec", filter);
724+
f = f.withFilter(filter);
757725
}
758726
return f;
759727
});

0 commit comments

Comments
 (0)