-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Use DV rewrites where possible in Keyword queries #137536
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
Use DV rewrites where possible in Keyword queries #137536
Conversation
|
Pinging @elastic/es-search (Team:Search) |
|
Hi @romseygeek, I've created a changelog YAML for you. |
|
Do you feel like the existing tests are enough to give us confidence in that there are no regressions or is there an opportunity to add more coverage? |
|
The behaviour should be exactly the same, so other than checking the type of the queries (which I'm a bit reluctant to do because that then gives us spurious failures if other ways of querying get added later) I don't think there's a meaningful test to be added. I will see if there is an existing benchmark suite that will demonstrate the performance 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.
LGTM
There is no test suite to guarantee that exact same behavior, but it should be the same.
The yaml tests that were introduced when script based lucene queries were added are still there and did pass 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.
I'm good to merge as-is. We can observe the impact on nightlies but I don't see how this could cause a regression. About the comment regarding tests, I was trying to make sure we have some coverage, I agree that we shouldn't assert on the type of the query, just that the outcome is similar. But it seems we have existing yaml tests for it 👍
Once this is merged, I'll also re-run the benchmarks for #137029.
|
Looks like this helped to avoid the regression for prefix filter queries: #137029 (comment) |
Keyword automaton queries against doc-values-only fields can use standard MultiTermQuery
implementations with
DOC_VALUES_REWRITErewrite methods. These should beconsiderably faster and more efficient than the scripted query implementations.