Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make limit on number of expanded fields configurable #35284

Merged
merged 4 commits into from
Nov 8, 2018
Merged

Make limit on number of expanded fields configurable #35284

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
96e915e
Make limit on number of expanded fields configurable
Nov 5, 2018
6a060dd
Merge branch 'master' into fix-34778
Nov 6, 2018
a33ef5a
iter
Nov 6, 2018
a154905
Fix test
Nov 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions docs/reference/query-dsl/query-string-query.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,9 @@ The `query_string` top level parameters include:
specified. Defaults to the `index.query.default_field` index settings, which in
turn defaults to `*`. `*` extracts all fields in the mapping that are eligible
to term queries and filters the metadata fields. All extracted fields are then
combined to build a query when no prefix field is provided. There is a limit of
no more than 1024 fields being queried at once.
combined to build a query when no prefix field is provided. There is a limit on
the number of fields that can be queried at once which is defined by the
`indices.query.bool.max_clause_count` <<search-settings>> which defaults to 1024.

|`default_operator` |The default operator used if no explicit operator
is specified. For example, with a default operator of `OR`, the query
Expand Down
5 changes: 3 additions & 2 deletions docs/reference/query-dsl/simple-query-string-query.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,9 @@ The `simple_query_string` top level parameters include:
|`fields` |The fields to perform the parsed query against. Defaults to the
`index.query.default_field` index settings, which in turn defaults to `*`. `*`
extracts all fields in the mapping that are eligible to term queries and filters
the metadata fields. There is a limit of no more than 1024 fields being queried
at once.
the metadata fields. There is a limit on the number of fields that can be queried
at once which is defined by the `indices.query.bool.max_clause_count` <<search-settings>>
which defaults to 1024.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a note in the multi_match query ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

np


|`default_operator` |The default operator used if no explicit operator
is specified. For example, with a default operator of `OR`, the query
Expand Down
14 changes: 8 additions & 6 deletions server/src/main/java/org/elasticsearch/index/search/QueryParserHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.elasticsearch.index.mapper.MappedFieldType;
import org.elasticsearch.index.query.QueryShardContext;
import org.elasticsearch.index.query.QueryShardException;
import org.elasticsearch.search.SearchModule;

import java.util.Collection;
import java.util.HashMap;
Expand Down Expand Up @@ -85,7 +86,7 @@ public static Map<String, Float> resolveMappingFields(QueryShardContext context,
!multiField, !allField, fieldSuffix);
resolvedFields.putAll(fieldMap);
}
checkForTooManyFields(resolvedFields);
checkForTooManyFields(resolvedFields, context);
return resolvedFields;
}

Expand Down Expand Up @@ -141,7 +142,7 @@ public static Map<String, Float> resolveMappingField(QueryShardContext context,
if (acceptAllTypes == false) {
try {
fieldType.termQuery("", context);
} catch (QueryShardException |UnsupportedOperationException e) {
} catch (QueryShardException | UnsupportedOperationException e) {
// field type is never searchable with term queries (eg. geo point): ignore
continue;
} catch (IllegalArgumentException |ElasticsearchParseException e) {
Expand All @@ -150,13 +151,14 @@ public static Map<String, Float> resolveMappingField(QueryShardContext context,
}
fields.put(fieldName, weight);
}
checkForTooManyFields(fields);
checkForTooManyFields(fields, context);
return fields;
}

private static void checkForTooManyFields(Map<String, Float> fields) {
if (fields.size() > 1024) {
throw new IllegalArgumentException("field expansion matches too many fields, limit: 1024, got: " + fields.size());
private static void checkForTooManyFields(Map<String, Float> fields, QueryShardContext context) {
Integer limit = SearchModule.INDICES_MAX_CLAUSE_COUNT_SETTING.get(context.getIndexSettings().getSettings());
if (fields.size() > limit) {
throw new IllegalArgumentException("field expansion matches too many fields, limit: " + limit + ", got: " + fields.size());
}
}
}
25 changes: 22 additions & 3 deletions server/src/test/java/org/elasticsearch/search/query/QueryStringIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@
import org.elasticsearch.index.query.QueryStringQueryBuilder;
import org.elasticsearch.search.SearchHit;
import org.elasticsearch.search.SearchHits;
import org.elasticsearch.search.SearchModule;
import org.elasticsearch.test.ESIntegTestCase;
import org.junit.Before;
import org.junit.BeforeClass;

import java.io.IOException;
import java.util.ArrayList;
Expand All @@ -51,13 +53,28 @@

public class QueryStringIT extends ESIntegTestCase {

private static int CLUSTER_MAX_CLAUSE_COUNT;

@BeforeClass
public static void createRandomClusterSetting() {
CLUSTER_MAX_CLAUSE_COUNT = randomIntBetween(500, 1500);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we test with small numbers, this seems big for a simple ut.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, the test was running with the default of 1024 before, so I wanted to stay in that area but there's no need to. Will lower it to the hundreds if that sounds okay.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

}

@Before
public void setup() throws Exception {
String indexBody = copyToStringFromClasspath("/org/elasticsearch/search/query/all-query-index.json");
prepareCreate("test").setSource(indexBody, XContentType.JSON).get();
ensureGreen("test");
}

@Override
protected Settings nodeSettings(int nodeOrdinal) {
return Settings.builder()
.put(super.nodeSettings(nodeOrdinal))
.put(SearchModule.INDICES_MAX_CLAUSE_COUNT_SETTING.getKey(), CLUSTER_MAX_CLAUSE_COUNT)
.build();
}

public void testBasicAllQuery() throws Exception {
List<IndexRequestBuilder> reqs = new ArrayList<>();
reqs.add(client().prepareIndex("test", "_doc", "1").setSource("f1", "foo bar baz"));
Expand Down Expand Up @@ -253,15 +270,16 @@ public void testLimitOnExpandedFields() throws Exception {
builder.startObject();
builder.startObject("type1");
builder.startObject("properties");
for (int i = 0; i < 1025; i++) {
for (int i = 0; i < CLUSTER_MAX_CLAUSE_COUNT + 1; i++) {
builder.startObject("field" + i).field("type", "text").endObject();
}
builder.endObject(); // properties
builder.endObject(); // type1
builder.endObject();

assertAcked(prepareCreate("toomanyfields")
.setSettings(Settings.builder().put(MapperService.INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING.getKey(), 1200))
.setSettings(Settings.builder().put(MapperService.INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING.getKey(),
CLUSTER_MAX_CLAUSE_COUNT + 100))
.addMapping("type1", builder));

client().prepareIndex("toomanyfields", "type1", "1").setSource("field171", "foo bar baz").get();
Expand All @@ -276,7 +294,8 @@ public void testLimitOnExpandedFields() throws Exception {
client().prepareSearch("toomanyfields").setQuery(qb).get();
});
assertThat(ExceptionsHelper.detailedMessage(e),
containsString("field expansion matches too many fields, limit: 1024, got: 1025"));
containsString("field expansion matches too many fields, limit: " + CLUSTER_MAX_CLAUSE_COUNT + ", got: "
+ (CLUSTER_MAX_CLAUSE_COUNT + 1)));
}

public void testFieldAlias() throws Exception {
Expand Down
26 changes: 23 additions & 3 deletions server/src/test/java/org/elasticsearch/search/query/SimpleQueryStringIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,10 @@
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.search.SearchHit;
import org.elasticsearch.search.SearchHits;
import org.elasticsearch.search.SearchModule;
import org.elasticsearch.search.builder.SearchSourceBuilder;
import org.elasticsearch.test.ESIntegTestCase;
import org.junit.BeforeClass;

import java.io.IOException;
import java.util.ArrayList;
Expand Down Expand Up @@ -76,6 +78,22 @@
* Tests for the {@code simple_query_string} query
*/
public class SimpleQueryStringIT extends ESIntegTestCase {

private static int CLUSTER_MAX_CLAUSE_COUNT;

@BeforeClass
public static void createRandomClusterSetting() {
CLUSTER_MAX_CLAUSE_COUNT = randomIntBetween(500, 1500);
}

@Override
protected Settings nodeSettings(int nodeOrdinal) {
return Settings.builder()
.put(super.nodeSettings(nodeOrdinal))
.put(SearchModule.INDICES_MAX_CLAUSE_COUNT_SETTING.getKey(), CLUSTER_MAX_CLAUSE_COUNT)
.build();
}

@Override
protected Collection<Class<? extends Plugin>> nodePlugins() {
return Collections.singletonList(MockAnalysisPlugin.class);
Expand Down Expand Up @@ -559,15 +577,16 @@ public void testLimitOnExpandedFields() throws Exception {
builder.startObject();
builder.startObject("type1");
builder.startObject("properties");
for (int i = 0; i < 1025; i++) {
for (int i = 0; i < CLUSTER_MAX_CLAUSE_COUNT + 1; i++) {
builder.startObject("field" + i).field("type", "text").endObject();
}
builder.endObject(); // properties
builder.endObject(); // type1
builder.endObject();

assertAcked(prepareCreate("toomanyfields")
.setSettings(Settings.builder().put(MapperService.INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING.getKey(), 1200))
.setSettings(Settings.builder().put(MapperService.INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING.getKey(),
CLUSTER_MAX_CLAUSE_COUNT + 100))
.addMapping("type1", builder));

client().prepareIndex("toomanyfields", "type1", "1").setSource("field171", "foo bar baz").get();
Expand All @@ -582,7 +601,8 @@ public void testLimitOnExpandedFields() throws Exception {
client().prepareSearch("toomanyfields").setQuery(qb).get();
});
assertThat(ExceptionsHelper.detailedMessage(e),
containsString("field expansion matches too many fields, limit: 1024, got: 1025"));
containsString("field expansion matches too many fields, limit: " + CLUSTER_MAX_CLAUSE_COUNT + ", got: "
+ (CLUSTER_MAX_CLAUSE_COUNT + 1)));
}

public void testFieldAlias() throws Exception {
Expand Down