-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Add term query for keyword script fields #59372
Add term query for keyword script fields #59372
Conversation
This adds what I think is just about the simplest possible `term` query implementation for `keyword` script fields and wires it into the field mapper that we build for them.
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 thanks!
@Override | ||
public float matchCost() { | ||
// TODO we don't have a good way of estimating the complexity of the script so we just go with 9000 | ||
return 9000f; |
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.
did we say that approximation.cost() was not good here?
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.
Yeah, Lucene documents this as "An estimate of the expected cost to determine that a single document". So multiplying by the cost of the approximation don't feel right. 9000 is kind of bogus too, but maybe less so.
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.
let's ask @jpountz what he thinks. I thought I remembered discussing with him that the cost could be the cost of the approximation but I may be wrong.
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.
this should not block merging the PR, we can always update the cost later.
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.
++
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.
+1 a high constant looks good to me
Pinging @elastic/es-search (:Search/Search) |
This adds what I think is just about the simplest possible
term
queryimplementation for
keyword
script fields and wires it into the fieldmapper that we build for them.
Relates to #59332