SQL: Preserve scoring in bool queries (#30730)

Make all bool constructs use match/should (that is a query context) as that is controlled and changed to a filter context by ES automatically based on the sort order (_doc, field vs _sort) and trackScores. Fix #29685
elastic · May 21, 2018 · dcf0f9f · dcf0f9f
1 parent be8c0f2
commit dcf0f9f
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 31 deletions.
diff --git a/...lugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/SourceGenerator.java b/...lugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/SourceGenerator.java
@@ -5,8 +5,6 @@
  */
 package org.elasticsearch.xpack.sql.execution.search;
 
-import org.elasticsearch.index.query.BoolQueryBuilder;
-import org.elasticsearch.index.query.ConstantScoreQueryBuilder;
 import org.elasticsearch.index.query.QueryBuilder;
 import org.elasticsearch.search.aggregations.AggregationBuilder;
 import org.elasticsearch.search.aggregations.bucket.composite.CompositeAggregationBuilder;
@@ -28,6 +26,7 @@
 import java.util.List;
 
 import static java.util.Collections.singletonList;
+import static org.elasticsearch.index.query.QueryBuilders.boolQuery;
 import static org.elasticsearch.search.sort.SortBuilders.fieldSort;
 import static org.elasticsearch.search.sort.SortBuilders.scoreSort;
 import static org.elasticsearch.search.sort.SortBuilders.scriptSort;
@@ -37,20 +36,23 @@ public abstract class SourceGenerator {
     private static final List<String> NO_STORED_FIELD = singletonList(StoredFieldsContext._NONE_);
 
     public static SearchSourceBuilder sourceBuilder(QueryContainer container, QueryBuilder filter, Integer size) {
-        final SearchSourceBuilder source = new SearchSourceBuilder();
+        QueryBuilder finalQuery = null;
         // add the source
-        if (container.query() == null) {
+        if (container.query() != null) {
             if (filter != null) {
-                source.query(new ConstantScoreQueryBuilder(filter));
+                finalQuery = boolQuery().must(container.query().asBuilder()).filter(filter);
+            } else {
+                finalQuery = container.query().asBuilder();
             }
         } else {
             if (filter != null) {
-                source.query(new BoolQueryBuilder().must(container.query().asBuilder()).filter(filter));
-            } else {
-                source.query(container.query().asBuilder());
+                finalQuery = boolQuery().filter(filter);
             }
         }
 
+        final SearchSourceBuilder source = new SearchSourceBuilder();
+        source.query(finalQuery);
+
         SqlSourceBuilder sortBuilder = new SqlSourceBuilder();
         // Iterate through all the columns requested, collecting the fields that
         // need to be retrieved from the result documents

diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/BoolQuery.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/query/BoolQuery.java
@@ -5,13 +5,13 @@
  */
 package org.elasticsearch.xpack.sql.querydsl.query;
 
-import java.util.Objects;
-
 import org.elasticsearch.index.query.BoolQueryBuilder;
 import org.elasticsearch.index.query.QueryBuilder;
 import org.elasticsearch.search.sort.NestedSortBuilder;
 import org.elasticsearch.xpack.sql.tree.Location;
 
+import java.util.Objects;
+
 import static org.elasticsearch.index.query.QueryBuilders.boolQuery;
 
 /**
@@ -63,9 +63,8 @@ public void enrichNestedSort(NestedSortBuilder sort) {
     public QueryBuilder asBuilder() {
         BoolQueryBuilder boolQuery = boolQuery();
         if (isAnd) {
-            // TODO are we throwing out score by using filter?
-            boolQuery.filter(left.asBuilder());
-            boolQuery.filter(right.asBuilder());
+            boolQuery.must(left.asBuilder());
+            boolQuery.must(right.asBuilder());
         } else {
             boolQuery.should(left.asBuilder());
             boolQuery.should(right.asBuilder());

diff --git a/.../sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java b/.../sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java
@@ -5,9 +5,6 @@
  */
 package org.elasticsearch.xpack.sql.execution.search;
 
-import org.elasticsearch.index.query.BoolQueryBuilder;
-import org.elasticsearch.index.query.ConstantScoreQueryBuilder;
-import org.elasticsearch.index.query.MatchQueryBuilder;
 import org.elasticsearch.index.query.Operator;
 import org.elasticsearch.index.query.QueryBuilder;
 import org.elasticsearch.search.aggregations.AggregatorFactories.Builder;
@@ -28,6 +25,8 @@
 import org.elasticsearch.xpack.sql.type.KeywordEsField;
 
 import static java.util.Collections.singletonList;
+import static org.elasticsearch.index.query.QueryBuilders.boolQuery;
+import static org.elasticsearch.index.query.QueryBuilders.matchQuery;
 import static org.elasticsearch.search.sort.SortBuilders.fieldSort;
 import static org.elasticsearch.search.sort.SortBuilders.scoreSort;
 
@@ -42,22 +41,22 @@ public void testNoQueryNoFilter() {
     public void testQueryNoFilter() {
         QueryContainer container = new QueryContainer().with(new MatchQuery(Location.EMPTY, "foo", "bar"));
         SearchSourceBuilder sourceBuilder = SourceGenerator.sourceBuilder(container, null, randomIntBetween(1, 10));
-        assertEquals(new MatchQueryBuilder("foo", "bar").operator(Operator.OR), sourceBuilder.query());
+        assertEquals(matchQuery("foo", "bar").operator(Operator.OR), sourceBuilder.query());
     }
 
     public void testNoQueryFilter() {
         QueryContainer container = new QueryContainer();
-        QueryBuilder filter = new MatchQueryBuilder("bar", "baz");
+        QueryBuilder filter = matchQuery("bar", "baz");
         SearchSourceBuilder sourceBuilder = SourceGenerator.sourceBuilder(container, filter, randomIntBetween(1, 10));
-        assertEquals(new ConstantScoreQueryBuilder(new MatchQueryBuilder("bar", "baz")), sourceBuilder.query());
+        assertEquals(boolQuery().filter(matchQuery("bar", "baz")), sourceBuilder.query());
     }
 
     public void testQueryFilter() {
         QueryContainer container = new QueryContainer().with(new MatchQuery(Location.EMPTY, "foo", "bar"));
-        QueryBuilder filter = new MatchQueryBuilder("bar", "baz");
+        QueryBuilder filter = matchQuery("bar", "baz");
         SearchSourceBuilder sourceBuilder = SourceGenerator.sourceBuilder(container, filter, randomIntBetween(1, 10));
-        assertEquals(new BoolQueryBuilder().must(new MatchQueryBuilder("foo", "bar").operator(Operator.OR))
-                .filter(new MatchQueryBuilder("bar", "baz")), sourceBuilder.query());
+        assertEquals(boolQuery().must(matchQuery("foo", "bar").operator(Operator.OR)).filter(matchQuery("bar", "baz")),
+                sourceBuilder.query());
     }
 
     public void testLimit() {

diff --git a/x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java b/x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java
@@ -16,7 +16,6 @@
 import org.elasticsearch.client.Response;
 import org.elasticsearch.client.ResponseException;
 import org.elasticsearch.common.CheckedSupplier;
-import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.collect.Tuple;
 import org.elasticsearch.common.io.Streams;
 import org.elasticsearch.common.xcontent.XContentHelper;
@@ -33,12 +32,11 @@
 import java.sql.JDBCType;
 import java.util.Arrays;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Locale;
 import java.util.Map;
-import java.util.TreeMap;
 
 import static java.util.Collections.emptyList;
-import static java.util.Collections.emptyMap;
 import static java.util.Collections.singletonList;
 import static java.util.Collections.singletonMap;
 import static java.util.Collections.unmodifiableMap;
@@ -396,19 +394,23 @@ public void testBasicTranslateQueryWithFilter() throws IOException {
         assertNotNull(query);
 
         @SuppressWarnings("unchecked")
-        Map<String, Object> constantScore = (Map<String, Object>) query.get("constant_score");
-        assertNotNull(constantScore);
+        Map<String, Object> bool = (Map<String, Object>) query.get("bool");
+        assertNotNull(bool);
 
         @SuppressWarnings("unchecked")
-        Map<String, Object> filter = (Map<String, Object>) constantScore.get("filter");
+        List<Object> filter = (List<Object>) bool.get("filter");
         assertNotNull(filter);
 
         @SuppressWarnings("unchecked")
-        Map<String, Object> match = (Map<String, Object>) filter.get("match");
-        assertNotNull(match);
+        Map<String, Object> map = (Map<String, Object>) filter.get(0);
+        assertNotNull(map);
 
         @SuppressWarnings("unchecked")
-        Map<String, Object> matchQuery = (Map<String, Object>) match.get("test");
+        Map<String, Object> matchQ = (Map<String, Object>) map.get("match");
+
+        @SuppressWarnings("unchecked")
+        Map<String, Object> matchQuery = (Map<String, Object>) matchQ.get("test");
+
         assertNotNull(matchQuery);
         assertEquals("foo", matchQuery.get("query"));
     }