Added max_expansion param to span_multi #30913

nirmalc · 2018-05-28T21:51:32Z

Causes span query to throw exception if term expansions with in
multi_term inner query exceeds max_expansion param

Example query:

{"span_multi" : {"match" : {"prefix" : {"user" : {"value" : "ki","boost" : 1.08}}},"max_expansions" : 22 ,"boost" :1.0}}

I was trying to patch one older version of ES to avoid heap crash, but noticed that its logged as an issue here - #27432

elasticmachine · 2018-05-28T22:29:33Z

Pinging @elastic/es-search-aggs

mayya-sharipova · 2018-05-28T22:56:34Z

@nirmalc Thank you for submitting the PR. It nicely addresses #27432

So this PR, although submitted against master, will be for 6.4 version.
For 7.x version, we will need another PR, slightly different.

So, for 6.x version, we want to do following:

Add max_expansions into span_multi query as an optional parameter that defaults to 1024
Issue a deprecation warning if a user submits span_multi query without max_expansions parameter
throw an error if max_expansions is greater than 1024
add max_expansions as an override of the rewrite in 6.x
update documentation for span_multi query

Unchecked items still need to be implemented in your PR

mayya-sharipova · 2018-05-28T23:02:04Z

server/src/main/java/org/elasticsearch/index/query/SpanMultiTermQueryBuilder.java

+                SpanTermQuery q = new SpanTermQuery(term, states);
+                topLevel.add(q);
+            }
+        };


I would not throw an error, I would just ensure that we collect only up to maxExpansions queries. @jimczi what do you think?

Thanks , I will look at checklist . I thought the plan was to throw exception if term expansion count is above max_expansions ( which is defaulted to 1024 ) . The other pattern can be done already by adding rewrite param to inner multi query

I think it's better to throw an exception, at least in master. I don't think we need a max_expansion option though, we can just honor BooleanQuery#getMaxClauseCount() and fails the query if there are more terms. The span scoring rewrite does not allow to limit the number of expansions, it is all or nothing and I think it's a good property since it will not confuse the users by returning partial responses on prefix queries that match more than 1024 terms. This is the main reason why I opened the issue in the first place, we should never build a disjunction on more than BooleanQuery.getMaxClauseCount().

jimczi

Thanks @nirmalc. The change looks good but I wonder if we really need a max_expansion option or if we should only honor BooleanQuery.getMaxClauseCount() ? MultiTermQuery that uses a TopTermRewrite strategy have the same limit so it would be consistent and wouldn't require the addition of another option that could be confusing since the rewrite method can be overridden in the multi term query directly.

jimczi · 2018-05-29T18:50:18Z

server/src/main/java/org/elasticsearch/index/query/SpanMultiTermQueryBuilder.java

+    }
+
+    public class TopTermSpanBooleanQueryRewriteWithMaxClause extends SpanMultiTermQueryWrapper.SpanRewriteMethod{
+        MultiTermQuery multiTermQuery;


Can you create a constructor to assign this variable and make it final ?

👍 , will do - I thought it will be nice to have more dynamic way to override instead of boolean.max count ( I believe its a static property and requires yml file change ) ; Dynamically changing is very useful in case of cluster with multiple indices/data shapes. But I do agree that it can be confusing .

Or should we use BooleanQuery.getMaxClauseCount() as default and provide an override ?

Dynamically changing is very useful in case of cluster with multiple indices/data shapes.

Which multi_term query are you using ? Is it to increase the default value of 1024 ? Any multi term query that don't use top terms rewriting is not a good fit for the span query in general. If you need to perform prefix queries within span queries it is preferable to index the prefix using the edge_ngram filter. This way you can transform any prefix query into a span term query on a single term.

Thanks for feedback @jimczi , Multi queries could be prefix , wildcard or worst case -regex. Im using span queries now for exact use case mentioned in ES documentation of span queries - legal discovery. We considered edge ngrams which takes care of part of that use case. data volume( text) and unpredictable cardinality is another issue ( often few thousands for many multi searches)

My request is to consider it as advanced param , but if elastic thinks it is confusing for users - i can change PR to use boolean query limit. Please let me know ! Thanks again

My original intent was to add a max_expansion to force the rewrite of all multi terms to use a top terms rewrite when used inside a span query but in this case the max_expansion param can be interpreted as the number of top terms to keep during the rewrite. However I prefer your approach which throws an exception on multi_term queries that don't use top terms rewrite and matches more than 1024 terms. In this case we can fail the query with a nice message explaining that a top term rewrite should be used on the multi_term query if applicable. max_expansion is confusing in this scenario and doesn't bring much so I think we should simply honor the BooleanQuery max clause limit.

👍 i will update the PR in a day or two

thanks @nirmalc !

jimczi · 2018-05-29T18:53:05Z

server/src/main/java/org/elasticsearch/index/query/SpanMultiTermQueryBuilder.java

+        return new SpanMultiTermQueryBuilder(subQuery).queryName(queryName).boost(boost).maxExpansions(maxExpansionValue);
+    }
+
+    public class TopTermSpanBooleanQueryRewriteWithMaxClause extends SpanMultiTermQueryWrapper.SpanRewriteMethod{


Why does it need to be public ? I'd prefer a private static class since it's not used outside of this class.

just used in one of unit tests..

nirmalc · 2018-06-01T02:39:50Z

@jimczi , please let me know if changes look good , I'll squash and rebase with master

jimczi

I left more comments but I like it overall.
Can you also merge with latest master, I'll run the tests afterward.

jimczi · 2018-06-01T07:29:56Z

docs/reference/query-dsl/span-multi-term-query.asciidoc

+
+span multi query will hit too many clauses failure if it exceeds the boolean
+query limit (defaults  to `1024`) unless inner multi query uses top_terms_N
+rewrite method.


Maybe add a link to the documentation: <<query-dsl-multi-term-rewrite,rewrite>>

jimczi · 2018-06-01T07:31:17Z

server/src/test/java/org/elasticsearch/search/query/SpanMultiQueryIT.java

+
+public class SpanMultiQueryIT extends ESIntegTestCase {
+    public void testTermExpansionExceptionOnSpanFailure() throws ExecutionException, InterruptedException {
+        org.elasticsearch.common.settings.Settings.Builder builder = org.elasticsearch.common.settings.Settings.builder();


You can import Settings and remove the package name ?

jimczi · 2018-06-01T07:33:16Z

server/src/test/java/org/elasticsearch/search/query/SpanMultiQueryIT.java

+
+        createIndex("test", builder.build());
+        ArrayList<IndexRequestBuilder> reqs = new ArrayList<>();
+        for (int i = 0; i < BooleanQuery.getMaxClauseCount() + 1; i++) {


That's a big test to check for an exception. I'd something like:

final int origValue = BooleanQuery.getMaxClauseCount(); BooleanQuery.setMaxClauseCount(5); try { // test goes here } finally { BooleanQuery.setMaxClauseCount(origValue); }

jimczi · 2018-06-01T07:35:30Z

server/src/test/java/org/elasticsearch/search/query/SpanMultiQueryIT.java

+import java.util.ArrayList;
+import java.util.concurrent.ExecutionException;
+
+public class SpanMultiQueryIT extends ESIntegTestCase {


Can you move these tests to org.elasticsearch.search.query.SearchQueryIT, we don't need another IT test to check this.

jimczi · 2018-06-01T07:36:20Z

server/src/main/java/org/elasticsearch/index/query/SpanMultiTermQueryBuilder.java

+            protected void checkMaxClauseCount(int count) {
+                if (count > maxExpansions) {
+                    throw new ElasticsearchException("[" + multiTermQuery.toString() + " ] " +
+                        "exceeds maxExpansions [maxExpansions is set to " + maxExpansions + "]");


Maybe mention boolean query max clause and top terms rewrite ?

nirmalc · 2018-06-04T16:49:58Z

@jimczi new commit should address all code review comments .

jimczi

It looks good @nirmalc ! I left some minor comments but I think it's getting close.

jimczi · 2018-06-05T07:12:55Z

server/src/test/java/org/elasticsearch/search/query/SearchQueryIT.java

@@ -1819,4 +1826,52 @@ public void testRangeQueryRangeFields_24744() throws Exception {
        assertHitCount(searchResponse, 1);
    }

+    public void testTermExpansionExceptionOnSpanFailure() throws ExecutionException, InterruptedException {
+        org.elasticsearch.common.settings.Settings.Builder builder = org.elasticsearch.common.settings.Settings.builder();


nit: add an import for Settings

jimczi · 2018-06-05T07:13:04Z

server/src/test/java/org/elasticsearch/search/query/SearchQueryIT.java

+
+            expectThrows(ElasticsearchException.class, () ->
+                client().prepareSearch().setIndices("test").setQuery(queryBuilder).get());
+        }finally{


nit: missing space

jimczi · 2018-06-05T07:14:34Z

server/src/test/java/org/elasticsearch/index/query/SpanMultiTermQueryBuilderTests.java

+
+    }
+
+    public void testDefaultMaxRewriteWithDefaultValue() throws Exception {


This is a duplicate of the previous test ?

jimczi · 2018-06-05T07:15:34Z

server/src/test/java/org/elasticsearch/index/query/SpanMultiTermQueryBuilderTests.java

+            "}," +
+            "\"boost\" :1.0" +
+            "}" +
+            "}";


The xcontent parsing is tested in the parent class, can you modify this test to use the query builders instead ?

jimczi · 2018-06-05T07:20:28Z

@nirmalc can you merge with master and resolve the conflicts ? I'll trigger the tests when it's done.

nirmalc · 2018-06-05T21:36:31Z

Done, sorry - my fork was bit old and i had to get rid of some tests since we decided not to do max_expansion params.

jimczi

Thanks @nirmalc , it's really close however I left some comments that need to be addressed before we run the tests.

jimczi · 2018-06-06T13:29:52Z

docs/reference/query-dsl/span-multi-term-query.asciidoc

+query limit (defaults  to `1024`) unless inner multi query uses top_terms_N
+<<query-dsl-multi-term-rewrite,rewrite>> parameter.
+


Please keep the WARNING here and maybe reword this to:

WARNING: `span_multi` queries will hit too many clauses failure if the number of terms that match the query exceeds the boolean query limit (defaults to 1024). To avoid an unbounded expansion you can set the <<query-dsl-multi-term-rewrite,rewrite method>> of the multi term query to `top_terms_*` rewrite. Or, if you use `span_multi` on `prefix` query only, you can activate the <<index-prefix-config,`index_prefixes`>> field option of the `text` field instead. This will rewrite any prefix query on the field to a a single term query that matches the indexed prefix.

jimczi · 2018-06-06T13:36:18Z

server/src/test/java/org/elasticsearch/search/query/SearchQueryIT.java

+    }
+
+    public void testTermExpansionExceptionOnSpan() throws ExecutionException, InterruptedException {
+        Settings.Builder builder = Settings.builder();


I don't think this test is needed. testSpanMultiTermQuery does the same thing.

By default span_multi query will limit term expansions = boolean max clause. This will limit high heap usage in case of high cardinality term expansions.This applies only if top_terms_N is not used in inner multi query

nirmalc · 2018-06-06T15:28:40Z

updated doc + removed test you suggested as already there is one testing that scenario.

jimczi

LGTM thanks @nirmalc , I'll merge if the tests pass

jimczi · 2018-06-06T16:34:17Z

test this please

By default span_multi query will limit term expansions = boolean max clause. This will limit high heap usage in case of high cardinality term expansions. This applies only if top_terms_N is not used in inner multi query.

mayya-sharipova added the :Search/Search Search-related issues that do not fall into other categories label May 28, 2018

mayya-sharipova added v7.0.0 v6.4.0 labels May 28, 2018

mayya-sharipova self-requested a review May 28, 2018 22:31

mayya-sharipova reviewed May 28, 2018

View reviewed changes

jimczi requested changes May 29, 2018

View reviewed changes

jimczi reviewed Jun 1, 2018

View reviewed changes

nirmalc force-pushed the spanMultiRewriteParam branch from 33c5e4b to de00e85 Compare June 1, 2018 13:53

jimczi reviewed Jun 5, 2018

View reviewed changes

nirmalc force-pushed the spanMultiRewriteParam branch from de00e85 to 68e2b13 Compare June 5, 2018 21:35

jimczi requested changes Jun 6, 2018

View reviewed changes

nirmalc force-pushed the spanMultiRewriteParam branch from 68e2b13 to 816e37f Compare June 6, 2018 15:28

jimczi approved these changes Jun 6, 2018

View reviewed changes

jimczi merged commit 75a676c into elastic:master Jun 7, 2018

jimczi mentioned this pull request Jun 7, 2018

Fail span_multi queries that don't use top_terms_* rewriting #27432

Closed

lcawl added the >enhancement label Aug 20, 2018

Mpdreamz mentioned this pull request Sep 25, 2018

[meta] 6.4.0 release elastic/elasticsearch-net#3397

Closed

89 tasks

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019


		}

		public void testDefaultMaxRewriteWithDefaultValue() throws Exception {

		query limit (defaults to `1024`) unless inner multi query uses top_terms_N
		<<query-dsl-multi-term-rewrite,rewrite>> parameter.

Added max_expansion param to span_multi #30913

Added max_expansion param to span_multi #30913

Conversation

nirmalc commented May 28, 2018 • edited Loading

elasticmachine commented May 28, 2018

mayya-sharipova commented May 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirmalc commented Jun 1, 2018

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirmalc commented Jun 4, 2018

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimczi commented Jun 5, 2018

nirmalc commented Jun 5, 2018

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirmalc commented Jun 6, 2018

jimczi left a comment

Choose a reason for hiding this comment

jimczi commented Jun 6, 2018

nirmalc commented May 28, 2018 •

edited

Loading