-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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.
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
Exclude unmapped fields during max clause limit checking for querying #49523
Changes from 2 commits
81b1256
79df51c
4268745
b4279d1
60efd2c
abbb18d
0c72d0b
fe53f2d
6804d83
96436a3
a6ef57d
4ef62f5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,2 @@ | ||
|
||
yaml.config.exists: true | ||
|
||
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -212,21 +212,17 @@ public static void afterClass() throws Exception { | |||||||
|
||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changes on this file are not needed. Can you revert ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is it not possible to add your extra fields in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did look at that method before. I think the problem of adding additional fields in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for the delay. I tried to modify
and all tests worked without modification. Could you try so that we can remove this unneeded change ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes sorry I checked without the modified elasticsearch/server/src/test/java/org/elasticsearch/search/query/QueryStringIT.java Line 266 in a59b065
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, mainly because it seems challenging to have a simple unit test for this and we already have integration tests.
I don't think it's needed either but that's unrelated to this pr so not sure if we should remove it now ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||||||||
@Before | ||||||||
public void beforeTest() throws Exception { | ||||||||
if (serviceHolder == null) { | ||||||||
assert serviceHolderWithNoType == null; | ||||||||
// we initialize the serviceHolder and serviceHolderWithNoType just once, but need some | ||||||||
// calls to the randomness source during its setup. In order to not mix these calls with | ||||||||
// the randomness source that is later used in the test method, we use the master seed during | ||||||||
// this setup | ||||||||
long masterSeed = SeedUtils.parseSeed(RandomizedTest.getContext().getRunnerSeedAsString()); | ||||||||
RandomizedTest.getContext().runWithPrivateRandomness(masterSeed, (Callable<Void>) () -> { | ||||||||
serviceHolder = new ServiceHolder(nodeSettings, createTestIndexSettings(), getPlugins(), nowInMillis, | ||||||||
AbstractBuilderTestCase.this, true); | ||||||||
serviceHolderWithNoType = new ServiceHolder(nodeSettings, createTestIndexSettings(), getPlugins(), nowInMillis, | ||||||||
AbstractBuilderTestCase.this, false); | ||||||||
return null; | ||||||||
}); | ||||||||
} | ||||||||
// we re-initiate serviceHolder for every test as some test cases may alter the state of serviceHolder | ||||||||
// and its inner objects, and reusing the same one may cause failure of other test cases. | ||||||||
// E.g. some test cases may alter field mapping information in serviceHolder.mapperService | ||||||||
long masterSeed = SeedUtils.parseSeed(RandomizedTest.getContext().getRunnerSeedAsString()); | ||||||||
RandomizedTest.getContext().runWithPrivateRandomness(masterSeed, (Callable<Void>) () -> { | ||||||||
serviceHolder = new ServiceHolder(nodeSettings, createTestIndexSettings(), getPlugins(), nowInMillis, | ||||||||
AbstractBuilderTestCase.this, true); | ||||||||
serviceHolderWithNoType = new ServiceHolder(nodeSettings, createTestIndexSettings(), getPlugins(), nowInMillis, | ||||||||
AbstractBuilderTestCase.this, false); | ||||||||
return null; | ||||||||
}); | ||||||||
|
||||||||
serviceHolder.clientInvocationHandler.delegate = this; | ||||||||
serviceHolderWithNoType.clientInvocationHandler.delegate = this; | ||||||||
|
@@ -400,11 +396,11 @@ public void onRemoval(ShardId shardId, Accountable accountable) { | |||||||
testCase.initializeAdditionalMappings(mapperService); | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
public static Predicate<String> indexNameMatcher() { | ||||||||
// Simplistic index name matcher used for testing | ||||||||
return pattern -> Regex.simpleMatch(pattern, index.getName()); | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
@Override | ||||||||
public void close() throws IOException { | ||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you revert this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah missed this, removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that it's still there ? ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.