From 0c107c51b278ae3d147768ae76100571dd023956 Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Tue, 4 Mar 2025 13:07:52 +0200 Subject: [PATCH 1/4] Use a "must" instead of "filter" when building the pushed down filter AND when scoring is needed; wrap the request filter with a bool filter in case the query itself doesn't need scoring. --- .../physical/local/PushFiltersToSource.java | 8 +++- .../local/ReplaceSourceAttributes.java | 37 ++++++++++++------- .../xpack/esql/plan/physical/EsQueryExec.java | 5 +++ .../esql/plan/physical/EsStatsQueryExec.java | 1 - .../planner/EsPhysicalOperationProviders.java | 5 +-- .../xpack/esql/session/EsqlSession.java | 37 +++++++++++++++++-- 6 files changed, 70 insertions(+), 23 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushFiltersToSource.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushFiltersToSource.java index 2f28b1a0e41ba..70bfb0df5f1d2 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushFiltersToSource.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushFiltersToSource.java @@ -101,7 +101,13 @@ private static PhysicalPlan rewrite( if (newPushable.size() > 0) { // update the executable with pushable conditions Query queryDSL = TRANSLATOR_HANDLER.asQuery(Predicates.combineAnd(newPushable)); QueryBuilder planQuery = queryDSL.asBuilder(); - var query = Queries.combine(Queries.Clause.FILTER, asList(queryExec.query(), planQuery)); + Queries.Clause combiningQueryClauseType; + if (queryExec.hasScoring()) { + combiningQueryClauseType = Queries.Clause.MUST; + } else { + combiningQueryClauseType = Queries.Clause.FILTER; + } + var query = Queries.combine(combiningQueryClauseType, asList(queryExec.query(), planQuery)); queryExec = new EsQueryExec( queryExec.source(), queryExec.indexPattern(), diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/ReplaceSourceAttributes.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/ReplaceSourceAttributes.java index 4f3358c539b05..dd9db3c3bf834 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/ReplaceSourceAttributes.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/ReplaceSourceAttributes.java @@ -32,27 +32,38 @@ protected PhysicalPlan rule(EsSourceExec plan) { var docId = new FieldAttribute(plan.source(), EsQueryExec.DOC_ID_FIELD.getName(), EsQueryExec.DOC_ID_FIELD); final List attributes = new ArrayList<>(); attributes.add(docId); - if (plan.indexMode() == IndexMode.TIME_SERIES) { - Attribute tsid = null, timestamp = null; - for (Attribute attr : plan.output()) { - String name = attr.name(); - if (name.equals(MetadataAttribute.TSID_FIELD)) { - tsid = attr; - } else if (name.equals(MetadataAttribute.TIMESTAMP_FIELD)) { - timestamp = attr; + + var outputIterator = plan.output().iterator(); + var isTimeSeries = plan.indexMode() == IndexMode.TIME_SERIES; + var keepIterating = true; + Attribute tsid = null, timestamp = null, score = null; + + while (keepIterating && outputIterator.hasNext()) { + Attribute attr = outputIterator.next(); + if (attr instanceof MetadataAttribute ma) { + if (ma.name().equals(MetadataAttribute.SCORE)) { + score = attr; + } else if (isTimeSeries) { + if (ma.name().equals(MetadataAttribute.TSID_FIELD)) { + tsid = attr; + } else if (ma.name().equals(MetadataAttribute.TIMESTAMP_FIELD)) { + timestamp = attr; + } } } + keepIterating = score == null || (isTimeSeries && (tsid == null || timestamp == null)); + } + if (isTimeSeries) { if (tsid == null || timestamp == null) { throw new IllegalStateException("_tsid or @timestamp are missing from the time-series source"); } attributes.add(tsid); attributes.add(timestamp); } - plan.output().forEach(attr -> { - if (attr instanceof MetadataAttribute ma && ma.name().equals(MetadataAttribute.SCORE)) { - attributes.add(ma); - } - }); + if (score != null) { + attributes.add(score); + } + return new EsQueryExec(plan.source(), plan.indexPattern(), plan.indexMode(), plan.indexNameWithModes(), attributes, plan.query()); } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsQueryExec.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsQueryExec.java index a3fc62d935795..73509ffd08155 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsQueryExec.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsQueryExec.java @@ -21,6 +21,7 @@ import org.elasticsearch.xpack.esql.core.expression.Attribute; import org.elasticsearch.xpack.esql.core.expression.Expression; import org.elasticsearch.xpack.esql.core.expression.FieldAttribute; +import org.elasticsearch.xpack.esql.core.expression.MetadataAttribute; import org.elasticsearch.xpack.esql.core.tree.NodeInfo; import org.elasticsearch.xpack.esql.core.tree.NodeUtils; import org.elasticsearch.xpack.esql.core.tree.Source; @@ -204,6 +205,10 @@ public static boolean isSourceAttribute(Attribute attr) { return DOC_ID_FIELD.getName().equals(attr.name()); } + public boolean hasScoring() { + return attrs().stream().anyMatch(a -> a instanceof MetadataAttribute && a.name().equals(MetadataAttribute.SCORE)); + } + @Override protected NodeInfo info() { return NodeInfo.create( diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsStatsQueryExec.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsStatsQueryExec.java index 96214652b87cb..5519e7fbc7083 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsStatsQueryExec.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsStatsQueryExec.java @@ -37,7 +37,6 @@ public enum StatsType { } public record Stat(String name, StatsType type, QueryBuilder query) { - public QueryBuilder filter(QueryBuilder sourceQuery) { return query == null ? sourceQuery : Queries.combine(Queries.Clause.FILTER, asList(sourceQuery, query)); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/EsPhysicalOperationProviders.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/EsPhysicalOperationProviders.java index 10c380f2db56d..e1e296fc12de9 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/EsPhysicalOperationProviders.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/EsPhysicalOperationProviders.java @@ -49,7 +49,6 @@ import org.elasticsearch.xpack.esql.core.expression.Expression; import org.elasticsearch.xpack.esql.core.expression.FieldAttribute; import org.elasticsearch.xpack.esql.core.expression.FoldContext; -import org.elasticsearch.xpack.esql.core.expression.MetadataAttribute; import org.elasticsearch.xpack.esql.core.type.DataType; import org.elasticsearch.xpack.esql.core.type.KeywordEsField; import org.elasticsearch.xpack.esql.core.type.MultiTypeEsField; @@ -186,9 +185,7 @@ public final PhysicalOperation sourcePhysicalOperation(EsQueryExec esQueryExec, assert esQueryExec.estimatedRowSize() != null : "estimated row size not initialized"; int rowEstimatedSize = esQueryExec.estimatedRowSize(); int limit = esQueryExec.limit() != null ? (Integer) esQueryExec.limit().fold(context.foldCtx()) : NO_LIMIT; - boolean scoring = esQueryExec.attrs() - .stream() - .anyMatch(a -> a instanceof MetadataAttribute && a.name().equals(MetadataAttribute.SCORE)); + boolean scoring = esQueryExec.hasScoring(); if ((sorts != null && sorts.isEmpty() == false)) { List> sortBuilders = new ArrayList<>(sorts.size()); for (Sort sort : sorts) { diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java index 2cc78713f89bb..7100101668b50 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java @@ -59,6 +59,7 @@ import org.elasticsearch.xpack.esql.plan.IndexPattern; import org.elasticsearch.xpack.esql.plan.logical.Aggregate; import org.elasticsearch.xpack.esql.plan.logical.Enrich; +import org.elasticsearch.xpack.esql.plan.logical.EsRelation; import org.elasticsearch.xpack.esql.plan.logical.Fork; import org.elasticsearch.xpack.esql.plan.logical.Keep; import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; @@ -70,6 +71,7 @@ import org.elasticsearch.xpack.esql.plan.logical.join.LookupJoin; import org.elasticsearch.xpack.esql.plan.logical.local.LocalRelation; import org.elasticsearch.xpack.esql.plan.logical.local.LocalSupplier; +import org.elasticsearch.xpack.esql.plan.physical.EsQueryExec; import org.elasticsearch.xpack.esql.plan.physical.EstimatesRowSize; import org.elasticsearch.xpack.esql.plan.physical.FragmentExec; import org.elasticsearch.xpack.esql.plan.physical.LocalSourceExec; @@ -712,16 +714,43 @@ private static Set subfields(Set names) { private PhysicalPlan logicalPlanToPhysicalPlan(LogicalPlan optimizedPlan, EsqlQueryRequest request) { PhysicalPlan physicalPlan = optimizedPhysicalPlan(optimizedPlan); + final QueryBuilder filter = request.filter(); + final Holder hasScoring = new Holder<>(false); + + if (filter != null) { + physicalPlan.forEachDown(EsQueryExec.class, esQueryExec -> { + if (hasScoring.get() == false && esQueryExec.hasScoring()) { + hasScoring.set(true); + } + }); + if (hasScoring.get() == false) { + physicalPlan.forEachDown(FragmentExec.class, fragmentExec -> { + fragmentExec.fragment().forEachDown(EsRelation.class, esRelation -> { + if (esRelation.output() + .stream() + .anyMatch(a -> a instanceof MetadataAttribute && a.name().equals(MetadataAttribute.SCORE))) { + hasScoring.set(true); + } + }); + }); + } + } physicalPlan = physicalPlan.transformUp(FragmentExec.class, f -> { - QueryBuilder filter = request.filter(); if (filter != null) { var fragmentFilter = f.esFilter(); // TODO: have an ESFilter and push down to EsQueryExec / EsSource // This is an ugly hack to push the filter parameter to Lucene // TODO: filter integration testing - filter = fragmentFilter != null ? boolQuery().filter(fragmentFilter).must(filter) : filter; - LOGGER.debug("Fold filter {} to EsQueryExec", filter); - f = f.withFilter(filter); + QueryBuilder newFilter; + if (fragmentFilter != null) { + newFilter = hasScoring.get() + ? boolQuery().filter(fragmentFilter).must(filter) + : boolQuery().filter(fragmentFilter).filter(filter); + } else { + newFilter = hasScoring.get() ? filter : boolQuery().filter(filter); + } + LOGGER.debug("Fold filter {} to EsQueryExec", newFilter); + f = f.withFilter(newFilter); } return f; }); From c88e28879d0b808c143398bb7f493d9953f2f1cd Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Tue, 4 Mar 2025 18:06:56 +0200 Subject: [PATCH 2/4] Fix test and add comments --- .../rules/physical/local/ReplaceSourceAttributes.java | 10 ++++------ .../elasticsearch/xpack/esql/session/EsqlSession.java | 7 +++++-- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/ReplaceSourceAttributes.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/ReplaceSourceAttributes.java index dd9db3c3bf834..4730f561348c9 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/ReplaceSourceAttributes.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/ReplaceSourceAttributes.java @@ -43,13 +43,11 @@ protected PhysicalPlan rule(EsSourceExec plan) { if (attr instanceof MetadataAttribute ma) { if (ma.name().equals(MetadataAttribute.SCORE)) { score = attr; - } else if (isTimeSeries) { - if (ma.name().equals(MetadataAttribute.TSID_FIELD)) { - tsid = attr; - } else if (ma.name().equals(MetadataAttribute.TIMESTAMP_FIELD)) { - timestamp = attr; - } + } else if (isTimeSeries && ma.name().equals(MetadataAttribute.TSID_FIELD)) { + tsid = attr; } + } else if (attr.name().equals(MetadataAttribute.TIMESTAMP_FIELD)) { + timestamp = attr; } keepIterating = score == null || (isTimeSeries && (tsid == null || timestamp == null)); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java index 7100101668b50..e15fd9ecc3a6d 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java @@ -718,11 +718,14 @@ private PhysicalPlan logicalPlanToPhysicalPlan(LogicalPlan optimizedPlan, EsqlQu final Holder hasScoring = new Holder<>(false); if (filter != null) { + // a PhysicalPlan completely transformed (no fragments) will have an EsQueryExec in it if the data comes from ES physicalPlan.forEachDown(EsQueryExec.class, esQueryExec -> { if (hasScoring.get() == false && esQueryExec.hasScoring()) { hasScoring.set(true); } }); + // if there is no EsQueryExec and still scoring is required, search for fragments as well where EsRelations should + // know if there is scoring needed or not if (hasScoring.get() == false) { physicalPlan.forEachDown(FragmentExec.class, fragmentExec -> { fragmentExec.fragment().forEachDown(EsRelation.class, esRelation -> { @@ -744,8 +747,8 @@ private PhysicalPlan logicalPlanToPhysicalPlan(LogicalPlan optimizedPlan, EsqlQu QueryBuilder newFilter; if (fragmentFilter != null) { newFilter = hasScoring.get() - ? boolQuery().filter(fragmentFilter).must(filter) - : boolQuery().filter(fragmentFilter).filter(filter); + ? boolQuery().filter(fragmentFilter).must(filter) // a "bool" "must" does influence scoring + : boolQuery().filter(fragmentFilter).filter(filter);// no scoring? then "filter" to completely disable scoring } else { newFilter = hasScoring.get() ? filter : boolQuery().filter(filter); } From 46d37ad4d3ab14f4cbaa1e73a3232b2754556fd7 Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Tue, 4 Mar 2025 18:11:20 +0200 Subject: [PATCH 3/4] Update docs/changelog/124001.yaml --- docs/changelog/124001.yaml | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 docs/changelog/124001.yaml diff --git a/docs/changelog/124001.yaml b/docs/changelog/124001.yaml new file mode 100644 index 0000000000000..374a7ad7efb58 --- /dev/null +++ b/docs/changelog/124001.yaml @@ -0,0 +1,7 @@ +pr: 124001 +summary: Use a must boolean statement when pushing down to Lucene when scoring is + also needed +area: ES|QL +type: bug +issues: + - 123967 From ac2c900bb0a34bf63d87f8a91b1a33a303a064be Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Wed, 5 Mar 2025 17:01:33 +0200 Subject: [PATCH 4/4] Add tests and revert EsqlSession change --- .../core/expression/MetadataAttribute.java | 2 +- .../xpack/esql/plugin/MatchOperatorIT.java | 117 ++++++++++++++++++ .../physical/local/PushFiltersToSource.java | 7 +- .../xpack/esql/plan/physical/EsQueryExec.java | 7 +- .../xpack/esql/session/EsqlSession.java | 40 +----- 5 files changed, 129 insertions(+), 44 deletions(-) diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/MetadataAttribute.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/MetadataAttribute.java index dc75ac3a96248..a07e2d9589034 100644 --- a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/MetadataAttribute.java +++ b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/MetadataAttribute.java @@ -29,7 +29,7 @@ import static org.elasticsearch.core.Tuple.tuple; public class MetadataAttribute extends TypedAttribute { - public static final String TIMESTAMP_FIELD = "@timestamp"; + public static final String TIMESTAMP_FIELD = "@timestamp"; // this is not a true metadata attribute public static final String TSID_FIELD = "_tsid"; public static final String SCORE = "_score"; public static final String INDEX = "_index"; diff --git a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/plugin/MatchOperatorIT.java b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/plugin/MatchOperatorIT.java index c978dead8f4fd..216786798ad34 100644 --- a/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/plugin/MatchOperatorIT.java +++ b/x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/plugin/MatchOperatorIT.java @@ -11,12 +11,16 @@ import org.elasticsearch.action.index.IndexRequest; import org.elasticsearch.action.support.WriteRequest; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.index.query.QueryBuilder; +import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.xpack.esql.VerificationException; import org.elasticsearch.xpack.esql.action.AbstractEsqlIntegTestCase; import org.junit.Before; import java.util.List; +import static org.elasticsearch.index.query.QueryBuilders.boolQuery; +import static org.elasticsearch.index.query.QueryBuilders.matchQuery; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.hamcrest.CoreMatchers.containsString; @@ -120,6 +124,119 @@ public void testWhereMatchWithScoring() { } } + /** + * Test for https://github.com/elastic/elasticsearch/issues/123967 + */ + public void testWhereMatchWithScoring_AndRequestFilter() { + var query = """ + FROM test METADATA _score + | WHERE content:"fox" + | SORT _score DESC + | KEEP content, _score + """; + + QueryBuilder filter = boolQuery().must(matchQuery("content", "brown")); + + try (var resp = run(query, randomPragmas(), filter)) { + assertColumnNames(resp.columns(), List.of("content", "_score")); + assertColumnTypes(resp.columns(), List.of("text", "double")); + assertValues( + resp.values(), + List.of( + List.of("This is a brown fox", 1.4274532794952393), + List.of("The quick brown fox jumps over the lazy dog", 1.1248724460601807) + ) + ); + } + } + + public void testWhereMatchWithScoring_AndNoScoreRequestFilter() { + var query = """ + FROM test METADATA _score + | WHERE content:"fox" + | SORT _score DESC + | KEEP content, _score + """; + + QueryBuilder filter = boolQuery().filter(matchQuery("content", "brown")); + + try (var resp = run(query, randomPragmas(), filter)) { + assertColumnNames(resp.columns(), List.of("content", "_score")); + assertColumnTypes(resp.columns(), List.of("text", "double")); + assertValues( + resp.values(), + List.of( + List.of("This is a brown fox", 1.156558871269226), + List.of("The quick brown fox jumps over the lazy dog", 0.9114001989364624) + ) + ); + } + } + + public void testWhereMatchWithScoring_And_MatchAllRequestFilter() { + var query = """ + FROM test METADATA _score + | WHERE content:"fox" + | SORT _score DESC + | KEEP content, _score + """; + + QueryBuilder filter = QueryBuilders.matchAllQuery(); + + try (var resp = run(query, randomPragmas(), filter)) { + assertColumnNames(resp.columns(), List.of("content", "_score")); + assertColumnTypes(resp.columns(), List.of("text", "double")); + assertValues( + resp.values(), + List.of( + List.of("This is a brown fox", 2.1565589904785156), + List.of("The quick brown fox jumps over the lazy dog", 1.9114001989364624) + ) + ); + } + } + + public void testScoringOutsideQuery() { + var query = """ + FROM test METADATA _score + | SORT _score DESC + | KEEP content, _score + """; + + QueryBuilder filter = boolQuery().must(matchQuery("content", "fox")); + + try (var resp = run(query, randomPragmas(), filter)) { + assertColumnNames(resp.columns(), List.of("content", "_score")); + assertColumnTypes(resp.columns(), List.of("text", "double")); + assertValues( + resp.values(), + List.of( + List.of("This is a brown fox", 1.156558871269226), + List.of("The quick brown fox jumps over the lazy dog", 0.9114001989364624) + ) + ); + } + } + + public void testScoring_Zero_OutsideQuery() { + var query = """ + FROM test METADATA _score + | SORT _score DESC + | KEEP content, _score + """; + + QueryBuilder filter = boolQuery().filter(matchQuery("content", "fox")); + + try (var resp = run(query, randomPragmas(), filter)) { + assertColumnNames(resp.columns(), List.of("content", "_score")); + assertColumnTypes(resp.columns(), List.of("text", "double")); + assertValues( + resp.values(), + List.of(List.of("This is a brown fox", 0.0), List.of("The quick brown fox jumps over the lazy dog", 0.0)) + ); + } + } + public void testWhereMatchWithScoringDifferentSort() { var query = """ FROM test diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushFiltersToSource.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushFiltersToSource.java index 70bfb0df5f1d2..f902f261e7dc9 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushFiltersToSource.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushFiltersToSource.java @@ -101,12 +101,7 @@ private static PhysicalPlan rewrite( if (newPushable.size() > 0) { // update the executable with pushable conditions Query queryDSL = TRANSLATOR_HANDLER.asQuery(Predicates.combineAnd(newPushable)); QueryBuilder planQuery = queryDSL.asBuilder(); - Queries.Clause combiningQueryClauseType; - if (queryExec.hasScoring()) { - combiningQueryClauseType = Queries.Clause.MUST; - } else { - combiningQueryClauseType = Queries.Clause.FILTER; - } + Queries.Clause combiningQueryClauseType = queryExec.hasScoring() ? Queries.Clause.MUST : Queries.Clause.FILTER; var query = Queries.combine(combiningQueryClauseType, asList(queryExec.query(), planQuery)); queryExec = new EsQueryExec( queryExec.source(), diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsQueryExec.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsQueryExec.java index 73509ffd08155..60e7eb535f444 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsQueryExec.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsQueryExec.java @@ -206,7 +206,12 @@ public static boolean isSourceAttribute(Attribute attr) { } public boolean hasScoring() { - return attrs().stream().anyMatch(a -> a instanceof MetadataAttribute && a.name().equals(MetadataAttribute.SCORE)); + for (Attribute a : attrs()) { + if (a instanceof MetadataAttribute && a.name().equals(MetadataAttribute.SCORE)) { + return true; + } + } + return false; } @Override diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java index e15fd9ecc3a6d..2cc78713f89bb 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java @@ -59,7 +59,6 @@ import org.elasticsearch.xpack.esql.plan.IndexPattern; import org.elasticsearch.xpack.esql.plan.logical.Aggregate; import org.elasticsearch.xpack.esql.plan.logical.Enrich; -import org.elasticsearch.xpack.esql.plan.logical.EsRelation; import org.elasticsearch.xpack.esql.plan.logical.Fork; import org.elasticsearch.xpack.esql.plan.logical.Keep; import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; @@ -71,7 +70,6 @@ import org.elasticsearch.xpack.esql.plan.logical.join.LookupJoin; import org.elasticsearch.xpack.esql.plan.logical.local.LocalRelation; import org.elasticsearch.xpack.esql.plan.logical.local.LocalSupplier; -import org.elasticsearch.xpack.esql.plan.physical.EsQueryExec; import org.elasticsearch.xpack.esql.plan.physical.EstimatesRowSize; import org.elasticsearch.xpack.esql.plan.physical.FragmentExec; import org.elasticsearch.xpack.esql.plan.physical.LocalSourceExec; @@ -714,46 +712,16 @@ private static Set subfields(Set names) { private PhysicalPlan logicalPlanToPhysicalPlan(LogicalPlan optimizedPlan, EsqlQueryRequest request) { PhysicalPlan physicalPlan = optimizedPhysicalPlan(optimizedPlan); - final QueryBuilder filter = request.filter(); - final Holder hasScoring = new Holder<>(false); - - if (filter != null) { - // a PhysicalPlan completely transformed (no fragments) will have an EsQueryExec in it if the data comes from ES - physicalPlan.forEachDown(EsQueryExec.class, esQueryExec -> { - if (hasScoring.get() == false && esQueryExec.hasScoring()) { - hasScoring.set(true); - } - }); - // if there is no EsQueryExec and still scoring is required, search for fragments as well where EsRelations should - // know if there is scoring needed or not - if (hasScoring.get() == false) { - physicalPlan.forEachDown(FragmentExec.class, fragmentExec -> { - fragmentExec.fragment().forEachDown(EsRelation.class, esRelation -> { - if (esRelation.output() - .stream() - .anyMatch(a -> a instanceof MetadataAttribute && a.name().equals(MetadataAttribute.SCORE))) { - hasScoring.set(true); - } - }); - }); - } - } physicalPlan = physicalPlan.transformUp(FragmentExec.class, f -> { + QueryBuilder filter = request.filter(); if (filter != null) { var fragmentFilter = f.esFilter(); // TODO: have an ESFilter and push down to EsQueryExec / EsSource // This is an ugly hack to push the filter parameter to Lucene // TODO: filter integration testing - QueryBuilder newFilter; - if (fragmentFilter != null) { - newFilter = hasScoring.get() - ? boolQuery().filter(fragmentFilter).must(filter) // a "bool" "must" does influence scoring - : boolQuery().filter(fragmentFilter).filter(filter);// no scoring? then "filter" to completely disable scoring - } else { - newFilter = hasScoring.get() ? filter : boolQuery().filter(filter); - } - LOGGER.debug("Fold filter {} to EsQueryExec", newFilter); - f = f.withFilter(newFilter); + filter = fragmentFilter != null ? boolQuery().filter(fragmentFilter).must(filter) : filter; + LOGGER.debug("Fold filter {} to EsQueryExec", filter); + f = f.withFilter(filter); } return f; });