Adds a rewrite phase to queries on the shard level #16870

colings86 · 2016-02-29T18:40:38Z

This change adds a rewrite phase to the queries on the shard before they are assessed for caching or executed. This allows the opportunity to rewrite queries as faster running simpler queries based on attributes known to only the shard itself. The first query to implement this is the RangeQueryBuilder which will rewrite to a MatchAllQueryBuilder if the range of terms on the shard is a subset of the query and rewrites to a MatchNoneQueryBuilder if the range of terms on the shard is completely outside the query.

Closes #9526

nik9000 · 2016-02-29T18:43:59Z

core/src/main/java/org/elasticsearch/action/fieldstats/FieldStats.java

+            if (optionalFormat != null) {
+                throw new UnsupportedOperationException("custom format isn't supported");
+            }
+            if (value instanceof java.lang.Long) {


Why not just Long?

I think we can just do this:

if (value instanceof Number) { return Long.toString(((Number)value).longValue()); }

s1monw · 2016-02-29T19:30:15Z

@colings86 I left some comments, looks great so far

polyfractal · 2016-02-29T19:46:03Z

Just a note, and absolutely not a blocker, but I think this rewrite will "escape" the profiler. Rewrite timings are only recorded when they go through the ContextIndexSearcher, so this sidesteps that ability. Also, we don't know if we should even profile until a little later when the source is parsed.

Perhaps slap a "TODO: escapes profiling" in there somewhere so that I can revisit this in the future? I imagine the profiler will need a bit of massaging to accommodate this new style of rewrites (which will only become more common in the future).

s1monw · 2016-02-29T19:50:45Z

Just a note, and absolutely not a blocker, but I think this rewrite will "escape" the profiler. Rewrite timings are only recorded when they go through the ContextIndexSearcher, so this sidesteps that ability. Also, we don't know if we should even profile until a little later when the source is parsed.

this is a different rewrite than lucene rewrite I think we are fine here?

polyfractal · 2016-02-29T20:02:43Z

this is a different rewrite than lucene rewrite I think we are fine here?

Yeah, which is why it doesn't get timed :) I assumed we would want any rewrite (ours or Lucene) to be included in the final rewrite timing, since it's still work that must be done and adds to latency?

s1monw · 2016-02-29T20:04:16Z

Yeah, which is why it doesn't get timed :) I assumed we would want any rewrite (ours or Lucene) to be included in the final rewrite timing, since it's still work that must be done and adds to latency?

compared to lucene I think we can ignore this rewriting here the one that is costly is MTQ rewrite this one is rather cheap.

jpountz · 2016-02-29T22:23:07Z

+1 to ignore

polyfractal · 2016-02-29T22:45:42Z

Works for me! :)

colings86 · 2016-03-02T12:25:52Z

Currently fails with seed -Dtests.seed=B4AEDE8445BFF1A5

s1monw · 2016-03-02T14:10:29Z

core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java

@@ -734,6 +734,53 @@ public BytesReference ext() {
        return ext;
    }

+    public SearchSourceBuilder rewrite(QueryShardContext context) throws IOException {


can we get some basic javadocs here too?

s1monw · 2016-03-02T14:11:50Z

this looks pretty awesome I left one minor comment... I love the simplification to the RangeQueryBuilderTests but I think we now need a dedicated unittest for FieldStatsProvider can you do that? Other than that LGTM

jpountz · 2016-03-02T14:44:50Z

core/src/main/java/org/elasticsearch/index/query/RangeQueryBuilder.java

+                if (fieldStatsProvider.isDenseField(fieldName)) {
+                    return new MatchAllQueryBuilder();
+                } else {
+                    return new ExistsQueryBuilder(fieldName);


I'm happy we made those exist queries fast. :)

s1monw · 2016-03-02T21:24:23Z

@colings86 I think this issue would close #9526

s1monw · 2016-03-16T09:47:17Z

core/src/main/java/org/elasticsearch/action/fieldstats/FieldStats.java

+                throw new UnsupportedOperationException("custom format isn't supported");
+            }
+            if (value instanceof Number) {
+                return java.lang.Float.toString(((Number) value).longValue());


hmm this should be ((Number) value).floatValue() no? I think we need better dedicated tests for all of those?

s1monw · 2016-03-16T09:47:46Z

core/src/main/java/org/elasticsearch/action/fieldstats/FieldStats.java

+                throw new UnsupportedOperationException("custom format isn't supported");
+            }
+            if (value instanceof Number) {
+                return java.lang.Double.toString(((Number) value).longValue());


same here doubleValue()

s1monw · 2016-03-16T10:04:36Z

@colings86 this looks awesome - pretty close, I left some comments

colings86 · 2016-03-16T11:08:19Z

@s1monw I pushed another commit addressing most of your comments

s1monw · 2016-03-16T11:32:36Z

LGTM

colings86 · 2016-03-16T19:01:00Z

@s1monw had to push one more commit to fix SimpleSearchIT.testSimpleDateRange().

s1monw · 2016-03-17T08:23:58Z

LGTM

This change adds a rewrite phase to the queries on the shard before they are assessed for caching or executed. This allows the opportunity to rewrite queries as faster running simpler queries based on attributes known to only the shard itself. The first query to implement this is the RangeQueryBuilder which will rewrite to a MatchAllQueryBuilder if the range of terms on the shard is a subset of the query and rewrites to a MatchNoneQueryBuilder if the range of terms on the shard is completely outside the query.

agirbal · 2016-07-19T05:27:11Z

quick note that based on a few production search use cases I've seen, supporting regular queries with hits would also be highly valuable. A typical example is if a dataset both has a "created time" (which drives the index name) and a "last updated time". Ideally querying on a window of "last updated time" that does not match anything in a given older index would make use of this optimization.

s1monw · 2016-07-19T07:58:45Z

@agirbal see #19472

clintongormley · 2016-07-19T13:43:51Z

also @agirbal regardless of caching, all range queries will make use of the optimization of checking whether the range intersects with the values in the shard.

agirbal · 2016-07-19T20:07:51Z

@clintongormley right I don't see what caching has to do with this. I think the query cache will be off like default. Great to hear there will be a "fail-fast" for shards that don't have the range.

asafm · 2016-07-27T16:31:53Z

Is it possible to backport this to 2.x?

colings86 · 2016-07-27T16:42:05Z

@asafm Unfortunately not. This PR is relatively small but there was a huge amount of work that lead up to this change which involved refactoring every part of the search request (every query type, aggregation, highlighter, suggester, etc.) so that it could be parsed into an internal Elasticsearch QueryBuilder object on the coordinating node to allow it to be in a form where it can be rewritten. Unfortunately the changes are just too extensive to be able to backport.

asafm · 2016-07-27T17:10:57Z

@colings86 Ok, thanks. When you do estimate Elastic 5.x is production worthy?

colings86 · 2016-07-28T08:54:05Z

5.x will be production worthy with the release of the 5.0.0 GA. When that will be is not easy to estimate as it depends on feedback we get from the alpha and beta versions.

colings86 added >feature review v5.0.0-alpha1 labels Feb 29, 2016

nik9000 reviewed Feb 29, 2016
View reviewed changes

s1monw reviewed Mar 2, 2016
View reviewed changes

jpountz reviewed Mar 2, 2016
View reviewed changes

s1monw reviewed Mar 16, 2016
View reviewed changes

s1monw mentioned this pull request Mar 16, 2016

Enable RequestCache by default if shard is inactive #17134

Closed

colings86 merged commit 0f6b8dd into elastic:master Mar 17, 2016

colings86 deleted the feature/queryRewrite branch March 17, 2016 09:12

clintongormley added the release highlight label Mar 17, 2016

colings86 mentioned this pull request Mar 17, 2016

Enable the indices request cache by default #17162

Merged

s1monw removed the review label Jul 19, 2016

colings86 mentioned this pull request Oct 18, 2016

Kibana 4 unable to utilize query cache #9526

Closed

clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query DSL labels Feb 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds a rewrite phase to queries on the shard level #16870

Adds a rewrite phase to queries on the shard level #16870

colings86 commented Feb 29, 2016

nik9000 Feb 29, 2016

s1monw Feb 29, 2016

s1monw commented Feb 29, 2016

polyfractal commented Feb 29, 2016

s1monw commented Feb 29, 2016

polyfractal commented Feb 29, 2016

s1monw commented Feb 29, 2016

jpountz commented Feb 29, 2016

polyfractal commented Feb 29, 2016

colings86 commented Mar 2, 2016

s1monw Mar 2, 2016

s1monw commented Mar 2, 2016

jpountz Mar 2, 2016

s1monw commented Mar 2, 2016

s1monw Mar 16, 2016

s1monw Mar 16, 2016

s1monw commented Mar 16, 2016

colings86 commented Mar 16, 2016

s1monw commented Mar 16, 2016

colings86 commented Mar 16, 2016

s1monw commented Mar 17, 2016

agirbal commented Jul 19, 2016

s1monw commented Jul 19, 2016

clintongormley commented Jul 19, 2016

agirbal commented Jul 19, 2016

asafm commented Jul 27, 2016

colings86 commented Jul 27, 2016

asafm commented Jul 27, 2016

colings86 commented Jul 28, 2016

Adds a rewrite phase to queries on the shard level #16870

Adds a rewrite phase to queries on the shard level #16870

Conversation

colings86 commented Feb 29, 2016

nik9000 Feb 29, 2016

Choose a reason for hiding this comment

s1monw Feb 29, 2016

Choose a reason for hiding this comment

s1monw commented Feb 29, 2016

polyfractal commented Feb 29, 2016

s1monw commented Feb 29, 2016

polyfractal commented Feb 29, 2016

s1monw commented Feb 29, 2016

jpountz commented Feb 29, 2016

polyfractal commented Feb 29, 2016

colings86 commented Mar 2, 2016

s1monw Mar 2, 2016

Choose a reason for hiding this comment

s1monw commented Mar 2, 2016

jpountz Mar 2, 2016

Choose a reason for hiding this comment

s1monw commented Mar 2, 2016

s1monw Mar 16, 2016

Choose a reason for hiding this comment

s1monw Mar 16, 2016

Choose a reason for hiding this comment

s1monw commented Mar 16, 2016

colings86 commented Mar 16, 2016

s1monw commented Mar 16, 2016

colings86 commented Mar 16, 2016

s1monw commented Mar 17, 2016

agirbal commented Jul 19, 2016

s1monw commented Jul 19, 2016

clintongormley commented Jul 19, 2016

agirbal commented Jul 19, 2016

asafm commented Jul 27, 2016

colings86 commented Jul 27, 2016

asafm commented Jul 27, 2016

colings86 commented Jul 28, 2016