Exclude unmapped fields during max clause limit checking for querying #49523

zacharymorn · 2019-11-25T07:27:53Z

Take into account of number of unmapped fields when calculating against limit. For details, please see #49002

jimczi

Thanks for your contribution @zacharymorn . I left some comments that need to be addressed. The pr could also be simplified to ignore unmapped fields completely rather than keeping the count in a separate Set (see review comment).

jimczi · 2019-11-25T08:40:34Z

server/src/main/java/org/elasticsearch/index/search/QueryParserHelper.java

@@ -143,6 +152,7 @@ private QueryParserHelper() {}
            if (fieldType == null) {
                // Note that we don't ignore unmapped fields.
                fields.put(fieldName, weight);
+                unmappedFields.add(fieldName);


We can ignore unmapped fields here and change the comment ? That's the scope of #49002, we want to ignore unmapped fields so we don't need to forward them to the Lucene parsing.

Got it. I thought previously they may still be needed down the pipe. I've updated the PR per your suggestion.

jimczi · 2019-11-25T08:41:30Z

server/src/test/resources/config/elasticsearch.yml

@@ -1,3 +1,2 @@

 yaml.config.exists: true
-


can you revert this change

Ah missed this, removed.

It seems that it's still there ? ;)

Hmm that's strange, might be my IDE setup problem. Anyway I checked out and committed the previous version of this file before my changes, and I don't see this change in Files Changed tab anymore. so it should be good now.

jimczi · 2019-11-25T08:42:22Z

test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java

@@ -212,21 +212,14 @@ public static void afterClass() throws Exception {



Changes on this file are not needed. Can you revert ?

Sorry I forgot to add a comment for this. The changes here are because in two of the new test cases I added, the state of shardContext.mapperService was changed to force the condition, which affected other test cases as well. By re-creating serviceHolder for every test case, side effects from test case does not affect another.

Why is it not possible to add your extra fields in QueryStringQueryBuilderTests#initializeAdditionalMappings ? That's where we expect tests to modify the shared mapping. Can you show the test failures if that doesn't work ?

I did look at that method before. I think the problem of adding additional fields in QueryStringQueryBuilderTests#initializeAdditionalMappings was that it is adding additional mappings for ALL test cases in that class (the additional xContent mapping is hard coded in the method, which is an override one as well so I can't change its interface to take in additional parameter easily), so that may cause other test cases in that class to also fail.

Sorry for the delay. I tried to modify QueryStringQueryBuilderTests#initializeAdditionalMappings like the following:

@Override protected void initializeAdditionalMappings(MapperService mapperService) throws IOException { XContentBuilder mapping = jsonBuilder().startObject().startObject("_doc").startObject("properties") .startObject("prefix_field") .field("type", "text") .startObject("index_prefixes").endObject() .endObject() .startObject("additionalMappedField1") .field("type", "text") .endObject() .startObject("additionalMappedField2") .field("type", "text") .endObject() .startObject("additionalMappedField3") .field("type", "text") .endObject() .startObject("additionalMappedField4") .field("type", "text") .endObject() .endObject().endObject().endObject(); mapperService.merge("_doc", new CompressedXContent(Strings.toString(mapping)), MapperService.MergeReason.MAPPING_UPDATE); }

and all tests worked without modification. Could you try so that we can remove this unneeded change ?

Yes sorry I checked without the modified indices.query.bool.max_clause_count. It seems tricky to test this behavior here so maybe you can modify or add a new test similar to

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

Line 266 in a59b065

public void testLimitOnExpandedFields() throws Exception {

?

Didn’t realize there’s an integration test for this as well…I just added a new test there.

I also removed this line of Index setting

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

Lines 279 to 280 in a59b065

.setSettings(Settings.builder().put(MapperService.INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING.getKey(),

CLUSTER_MAX_CLAUSE_COUNT + 100))

in test QueryStringIT.testLimitOnExpandedFields as it doesn’t seems to make sense in the context (why is it setting INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING to CLUSTER_MAX_CLAUSE_COUNT + 100?), and have no impact to test result. But please let me know if that’s indeed needed and I can put it back.

Having added the additional test though, I feel this test and the ones I’ve already put in and passing are different levels of tests (integration & unit tests) and thus it doesn’t hurt for them to coexist. Are you suggesting to have integration test only for these changes?

Are you suggesting to have integration test only for these changes?

Yes, mainly because it seems challenging to have a simple unit test for this and we already have integration tests.

In test QueryStringIT.testLimitOnExpandedFields as it doesn’t seems to make sense in the context (why is it setting INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING to CLUSTER_MAX_CLAUSE_COUNT + 100?), and have no impact to test result. But please let me know if that’s indeed needed and I can put it back.

I don't think it's needed either but that's unrelated to this pr so not sure if we should remove it now ?

I just pushed another commit that reverted the unneeded change to the integration test, and the unit tests and their related changes.

However, I guess I’m still a bit confused for why it’s tricky to keep those unit tests there, when they are already passing and verifying the right logic. As can be seen in the new (reverting) commit, the changes I made to allow unit tests there are the following:

The new unit tests themselves

Local, test class level change to force the condition for the added unit tests (setting indices.query.bool.max_clause_count)

Super test class level change to make sure that each tests are independent to each other so that Discovery: Support local (JVM level) discovery #2 doesn’t break other test cases (the changes in AbstractBuilderTestCase.beforeTest)

These changes combined would allow the newly added test cases to pass, and not break any existing test cases.

I feel I’m still having some gap in understanding the reasoning here, but I’ll leave it to you. Thanks for your patience and time for reviewing my changes!

I feel I’m still having some gap in understanding the reasoning here, but I’ll leave it to you

My reasoning here is that the abstract class for these tests was designed to share the mapping within the tests so it would require a bigger refactoring to change this behavior. We also have a lot of unit tests here so even if they pass I am cautious of the additional work that would be needed if we switch to one configuration per test.

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

elasticmachine · 2019-11-25T11:04:10Z

Pinging @elastic/es-search (:Search/Search)

jimczi

Thanks @zacharymorn , I left more comments

jimczi · 2019-11-28T10:14:25Z

test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java

@@ -212,21 +212,14 @@ public static void afterClass() throws Exception {



Why is it not possible to add your extra fields in QueryStringQueryBuilderTests#initializeAdditionalMappings ? That's where we expect tests to modify the shared mapping. Can you show the test failures if that doesn't work ?

jimczi · 2019-11-28T10:14:53Z

server/src/test/resources/config/elasticsearch.yml

@@ -1,3 +1,2 @@

 yaml.config.exists: true
-


It seems that it's still there ? ;)

jimczi · 2020-01-15T16:10:58Z

@elasticmachine ok to test.

jimczi · 2020-01-15T16:45:37Z

@elasticmachine update branch

jimczi · 2020-01-15T17:33:05Z

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

+        builder.endObject(); // type1
+        builder.endObject();
+
+        assertAcked(prepareCreate("ignoreunmappedfields").addMapping("type1", builder));


@zacharymorn I merged master in your pr and now there's a test compilation error. Can you take a look ?

I merged master in and looks like there’s change in CreateIndexRequestBuilder.addMapping . Just updated the test and pushed up the commits. The CI is still failing though it was broken for an seemingly unrelated integration test org.elasticsearch.snapshots.DedicatedClusterSnapshotRestoreIT.testMasterShutdownDuringSnapshot. Is this something I should be looking into as well ?

…rom latest

…earch into issue-49002

jimczi · 2020-01-17T09:55:58Z

@elasticmachine test elasticsearch-ci/1

jimczi · 2020-01-17T09:58:04Z

@elasticmachine ok to test

jimczi · 2020-01-17T12:09:03Z

@elasticmachine update branch

jimczi · 2020-01-20T10:54:13Z

Thanks @zacharymorn

Take into account of number of unmapped fields when calculating against limit. Closes #49002

Take into account of number of unmapped fields when calculating against limit. Closes elastic#49002

Exclude unmapped fileds during max clause limit checking for querying

81b1256

jimczi requested changes Nov 25, 2019

View reviewed changes

cbuescher added the :Search/Search Search-related issues that do not fall into other categories label Nov 25, 2019

Address comments

79df51c

jimczi requested changes Nov 28, 2019

View reviewed changes

zacharymorn added 3 commits November 28, 2019 10:52

Remove unneeded changes in elasticsearch.yml

4268745

Add integration test for igoring unmapping fields

b4279d1

Remove unit tests and unneeded changes to integration test

60efd2c

Merge branch 'master' into issue-49002

abbb18d

jimczi reviewed Jan 15, 2020

View reviewed changes

zacharymorn added 5 commits January 15, 2020 10:06

Merge branch 'master' into issue-49002

0c72d0b

Update test to use CreateIndexRequestBuilder.setMapping after merge f…

fe53f2d

…rom latest

Merge branch 'master' into issue-49002

6804d83

Merge branch 'issue-49002' of https://github.com/zacharymorn/elastics…

96436a3

…earch into issue-49002

ix test failure as type has been removed in CreateIndexRequest

a6ef57d

Merge branch 'master' into issue-49002

4ef62f5

jimczi approved these changes Jan 20, 2020

View reviewed changes

jimczi changed the title ~~Exclude unmapped fileds during max clause limit checking for querying~~ Exclude unmapped fields during max clause limit checking for querying Jan 20, 2020

jimczi merged commit 3fba171 into elastic:master Jan 20, 2020

jimczi added >enhancement v7.7.0 v8.0.0 labels Jan 20, 2020

jimczi pushed a commit that referenced this pull request Jan 20, 2020

Exclude unmapped fields from query max_clause limit (#49523)

dc02458

Take into account of number of unmapped fields when calculating against limit. Closes #49002

SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this pull request Jan 23, 2020

Exclude unmapped fields from query max_clause limit (elastic#49523)

c31d60e

Take into account of number of unmapped fields when calculating against limit. Closes elastic#49002

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket (Part 2) elastic/elasticsearch-net#4533

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude unmapped fields during max clause limit checking for querying #49523

Exclude unmapped fields during max clause limit checking for querying #49523

zacharymorn commented Nov 25, 2019

jimczi left a comment

jimczi Nov 25, 2019

zacharymorn Nov 26, 2019

jimczi Nov 25, 2019

zacharymorn Nov 26, 2019

jimczi Nov 28, 2019

zacharymorn Nov 28, 2019

jimczi Nov 25, 2019

zacharymorn Nov 26, 2019

jimczi Nov 28, 2019

zacharymorn Nov 28, 2019 •

edited

Loading

jimczi Dec 12, 2019

jimczi Jan 9, 2020

zacharymorn Jan 10, 2020

jimczi Jan 13, 2020

zacharymorn Jan 15, 2020

jimczi Jan 15, 2020

elasticmachine commented Nov 25, 2019

jimczi left a comment

jimczi Nov 28, 2019

jimczi Nov 28, 2019

jimczi commented Jan 15, 2020

jimczi commented Jan 15, 2020

jimczi Jan 15, 2020

zacharymorn Jan 17, 2020

jimczi commented Jan 17, 2020

jimczi commented Jan 17, 2020

jimczi commented Jan 17, 2020

jimczi commented Jan 20, 2020

		@@ -212,21 +212,14 @@ public static void afterClass() throws Exception {

	.setSettings(Settings.builder().put(MapperService.INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING.getKey(),
	CLUSTER_MAX_CLAUSE_COUNT + 100))

Exclude unmapped fields during max clause limit checking for querying #49523

Exclude unmapped fields during max clause limit checking for querying #49523

Conversation

zacharymorn commented Nov 25, 2019

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

zacharymorn Nov 28, 2019 • edited Loading

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

elasticmachine commented Nov 25, 2019

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

jimczi commented Jan 15, 2020

jimczi commented Jan 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimczi commented Jan 17, 2020

jimczi commented Jan 17, 2020

jimczi commented Jan 17, 2020

jimczi commented Jan 20, 2020

zacharymorn Nov 28, 2019 •

edited

Loading