-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Fix SearchExecutionContextTests.testFielddataLookupSomtimesLoop #137595
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
base: main
Are you sure you want to change the base?
Fix SearchExecutionContextTests.testFielddataLookupSomtimesLoop #137595
Conversation
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
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.
Nice find!
| runtimeField("1", leafLookup -> leafLookup.doc().get("2").get(0).toString()), | ||
| runtimeField("2", leafLookup -> leafLookup.doc().get("3").get(0).toString()), | ||
| runtimeField("3", leafLookup -> leafLookup.doc().get("4").get(0).toString()), | ||
| runtimeField("4", (leafLookup, docId) -> { |
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 we remove docId?
| try (Directory directory = newDirectory(); RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) { | ||
| indexWriter.addDocument(List.of(new StringField("indexed_field", "first", Field.Store.NO))); | ||
| indexWriter.addDocument(List.of(new StringField("indexed_field", "second", Field.Store.NO))); | ||
| indexWriter.addDocument(List.of(new KeywordField("indexed_field", "first", Field.Store.YES))); |
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.
You sure this doesn't mess with other tests that use this method? I would run @Repeat on the suite to be sure.
Closes #136766.
The test creates a cycle in runtime fields that is broken if the doc with
docId == 0's field4is accessed. The first part of the test attempts to only fetch the document withindexed_fieldfirstwhich should not fail because the runtime fields would not have a loop sincefirstshould havedocId == 0. The second part fetches all of the documents which should fail since all of the documents are fetched and the runtime fields are calculated.The random seed used in the failing test results in the document with
firstactually getting adocIdof 1 causing the test to fail. Since we can't depend ondocIdto break the cycle in the runtime fields, changes were made to be able to useindexed_field == "first"to break the cycle in the first part of the test.