Browse Source

SQL: Set track_total_hits to false when not needed (#89106)

Luigi Dell'Aquila 3 years ago
parent
commit
e9ea4630bb

+ 6 - 0
docs/changelog/89106.yaml

@@ -0,0 +1,6 @@
+pr: 89106
+summary: Set `track_total_hits` to false when not needed
+area: SQL
+type: enhancement
+issues:
+ - 88764

+ 2 - 1
docs/reference/sql/endpoints/translate.asciidoc

@@ -45,7 +45,8 @@ Which returns:
         "unmapped_type": "short"
       }
     }
-  ]
+  ],
+  "track_total_hits": -1
 }
 --------------------------------------------------
 

+ 1 - 0
x-pack/plugin/build.gradle

@@ -142,6 +142,7 @@ tasks.named("yamlRestTestV7CompatTransform").configure { task ->
   task.skipTest("indices.freeze/10_basic/Basic", "#70192 -- the freeze index API is removed from 8.0")
   task.skipTest("indices.freeze/10_basic/Test index options", "#70192 -- the freeze index API is removed from 8.0")
   task.skipTest("sql/sql/Paging through results", "scrolling through search hit queries no longer produces empty last page in 8.2")
+  task.skipTest("sql/translate/Translate SQL", "query folding changed in v 8.5, added track_total_hits: -1")
   task.skipTest("service_accounts/10_basic/Test get service accounts", "new service accounts are added")
   task.skipTest("spatial/70_script_doc_values/diagonal length", "precision changed in 8.4.0")
 

+ 54 - 0
x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/execution/search/extractor/TotalHitsExtractor.java

@@ -0,0 +1,54 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+
+package org.elasticsearch.xpack.ql.execution.search.extractor;
+
+import org.elasticsearch.common.io.stream.StreamInput;
+import org.elasticsearch.search.SearchHit;
+import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation;
+import org.elasticsearch.xpack.ql.QlIllegalArgumentException;
+
+import java.io.IOException;
+
+public class TotalHitsExtractor extends ConstantExtractor {
+
+    public TotalHitsExtractor(Long constant) {
+        super(constant);
+    }
+
+    TotalHitsExtractor(StreamInput in) throws IOException {
+        super(in);
+    }
+
+    @Override
+    public Object extract(MultiBucketsAggregation.Bucket bucket) {
+        return validate(super.extract(bucket));
+    }
+
+    @Override
+    public Object extract(SearchHit hit) {
+        return validate(super.extract(hit));
+    }
+
+    private Object validate(Object value) {
+        if (Number.class.isInstance(value) == false) {
+            throw new QlIllegalArgumentException(
+                "Inconsistent total hits count handling, expected a numeric value but found a {}: {}",
+                value == null ? null : value.getClass().getSimpleName(),
+                value
+            );
+        }
+        if (((Number) value).longValue() < 0) {
+            throw new QlIllegalArgumentException(
+                "Inconsistent total hits count handling, expected a non-negative value but found {}",
+                value
+            );
+        }
+        return value;
+    }
+
+}

+ 4 - 2
x-pack/plugin/sql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/sql/qa/single_node/CliExplainIT.java

@@ -52,7 +52,8 @@ public class CliExplainIT extends CliIntegrationTestCase {
         assertThat(readLine(), startsWith("        \"order\" : \"asc\""));
         assertThat(readLine(), startsWith("      }"));
         assertThat(readLine(), startsWith("    }"));
-        assertThat(readLine(), startsWith("  ]"));
+        assertThat(readLine(), startsWith("  ],"));
+        assertThat(readLine(), startsWith("  \"track_total_hits\" : -1"));
         assertThat(readLine(), startsWith("}]"));
         assertEquals("", readLine());
     }
@@ -111,7 +112,8 @@ public class CliExplainIT extends CliIntegrationTestCase {
         assertThat(readLine(), startsWith("        \"order\" : \"asc\""));
         assertThat(readLine(), startsWith("      }"));
         assertThat(readLine(), startsWith("    }"));
-        assertThat(readLine(), startsWith("  ]"));
+        assertThat(readLine(), startsWith("  ],"));
+        assertThat(readLine(), startsWith("  \"track_total_hits\" : -1"));
         assertThat(readLine(), startsWith("}]"));
         assertEquals("", readLine());
     }

+ 5 - 2
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/Querier.java

@@ -8,6 +8,7 @@ package org.elasticsearch.xpack.sql.execution.search;
 
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+import org.apache.lucene.search.TotalHits;
 import org.apache.lucene.util.PriorityQueue;
 import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.action.search.ClosePointInTimeAction;
@@ -42,6 +43,7 @@ import org.elasticsearch.xpack.ql.execution.search.extractor.BucketExtractor;
 import org.elasticsearch.xpack.ql.execution.search.extractor.ComputingExtractor;
 import org.elasticsearch.xpack.ql.execution.search.extractor.ConstantExtractor;
 import org.elasticsearch.xpack.ql.execution.search.extractor.HitExtractor;
+import org.elasticsearch.xpack.ql.execution.search.extractor.TotalHitsExtractor;
 import org.elasticsearch.xpack.ql.expression.Attribute;
 import org.elasticsearch.xpack.ql.expression.gen.pipeline.AggExtractorInput;
 import org.elasticsearch.xpack.ql.expression.gen.pipeline.AggPathInput;
@@ -535,14 +537,15 @@ public class Querier {
             List<QueryContainer.FieldInfo> refs = query.fields();
 
             List<BucketExtractor> exts = new ArrayList<>(refs.size());
-            ConstantExtractor totalCount = new ConstantExtractor(response.getHits().getTotalHits().value);
+            TotalHits totalHits = response.getHits().getTotalHits();
+            ConstantExtractor totalCount = new TotalHitsExtractor(totalHits == null ? -1L : totalHits.value);
             for (QueryContainer.FieldInfo ref : refs) {
                 exts.add(createExtractor(ref.extraction(), totalCount));
             }
             return exts;
         }
 
-        private BucketExtractor createExtractor(FieldExtraction ref, BucketExtractor totalCount) {
+        private BucketExtractor createExtractor(FieldExtraction ref, ConstantExtractor totalCount) {
             if (ref instanceof GroupByRef r) {
                 return new CompositeKeyExtractor(r.key(), r.property(), cfg.zoneId(), r.dataType());
             }

+ 2 - 0
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/SourceGenerator.java

@@ -161,6 +161,8 @@ public abstract class SourceGenerator {
         }
         if (query.shouldTrackHits()) {
             builder.trackTotalHits(true);
+        } else {
+            builder.trackTotalHits(false);
         }
         builder.fetchSource(FetchSourceContext.DO_NOT_FETCH_SOURCE);
     }

+ 10 - 7
x-pack/plugin/sql/src/test/resources/org/elasticsearch/xpack/sql/planner/querytranslator_tests.txt

@@ -36,16 +36,19 @@ SELECT some.string FROM test WHERE some.string = 'value';
 TermEqualityNotAnalyzed
 SELECT some.string FROM test WHERE int = 5;
 CONTAINS "term":{"int":{"value":5
+CONTAINS "track_total_hits":-1
 ;
 
 TermEqualityForDate
 SELECT some.string FROM test WHERE date = 5;
 "term":{"date":{"value":5
+CONTAINS "track_total_hits":-1
 ;
 
 EqualsAndInOnTheSameField
 SELECT int FROM test WHERE int in (1, 2) OR int = 3 OR int = 2;
 "terms":{"int":[1,2,3]
+CONTAINS "track_total_hits":-1
 ;
 
 
@@ -150,31 +153,31 @@ REGEX ^\{"size":0,.*"track_total_hits":\d+.*$
 
 GlobalCountInSpecificGroupByDoesNotForceTrackHits
 SELECT COUNT(*) FROM test GROUP BY int;
-REGEX ^((?!"track_total_hits":).)*$
+CONTAINS "track_total_hits":-1
 CONTAINS {"terms":{"field":"int","missing_bucket":true,"order":"asc"}}
 ;
 
 FieldAllCountDoesNotTrackHits
 SELECT COUNT(ALL int) FROM test;
-REGEX ^((?!"track_total_hits":).)*$
+CONTAINS "track_total_hits":-1
 REGEX "aggregations":\{"[a-zA-Z0-9]+":\{"filter":\{"exists":\{"field":"int","boost":1.0\}\}\}\}
 ;
 
 FieldCountDoesNotTrackHits
 SELECT COUNT(int) FROM test;
-REGEX ^((?!"track_total_hits":).)*$
+CONTAINS "track_total_hits":-1
 REGEX "aggregations":\{"[a-zA-Z0-9]+":\{"filter":\{"exists":\{"field":"int","boost":1.0\}\}\}\}\}\}\}$
 ;
 
 DistinctCountDoesNotTrackHits
 SELECT COUNT(DISTINCT int) FROM test;
-REGEX ^((?!"track_total_hits":).)*$
+CONTAINS "track_total_hits":-1
 REGEX "aggregations":\{"[a-zA-Z0-9]+":\{"cardinality":\{"field":"int"\}\}\}
 ;
 
 NoCountDoesNotTrackHits
 SELECT int FROM test;
-REGEX ^((?!"track_total_hits":).)*$
+CONTAINS "track_total_hits":-1
 ;
 
 // Aggregate Functions
@@ -337,7 +340,7 @@ OrderByYear
 SELECT YEAR(date) FROM test ORDER BY 1;
 "sort":[{"_script":{"script":{"source":"InternalQlScriptUtils.nullSafeSortNumeric(InternalSqlScriptUtils.dateTimeExtract(InternalQlScriptUtils.docValue(doc,params.v0),params.v1,params.v2))"
 "params":{"v0":"date","v1":"Z","v2":"YEAR"}},
-"type":"number","order":"asc"}}]}
+"type":"number","order":"asc"}}],"track_total_hits":-1}
 ;
 
 // LIKE/RLIKE
@@ -413,7 +416,7 @@ SELECT keyword FROM test ORDER BY date::TIME, int LIMIT 5;
 "sort":[{"_script":{"script":{"source":"InternalQlScriptUtils.nullSafeSortString(InternalSqlScriptUtils.cast(InternalQlScriptUtils.docValue(doc,params.v0),params.v1))
 "params":{"v0":"date","v1":"TIME"}
 "type":"string",
-"order":"asc"}},{"int":{"order":"asc","missing":"_last","unmapped_type":"integer"}}]}
+"order":"asc"}},{"int":{"order":"asc","missing":"_last","unmapped_type":"integer"}}],"track_total_hits":-1}
 ;
 
 

+ 1 - 0
x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/sql/translate.yml

@@ -24,3 +24,4 @@
               order: asc
               missing: _last
               unmapped_type: long
+        track_total_hits: -1