-
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
Implement runtime script ips #60533
Implement runtime script ips #60533
Conversation
This implements the `ip` typed runtime fields. They share a fair bit with `string` runtime fields but we represent them as a `BytesRef` containing 128 bits so that the comparisons all happen in the same way as Lucene's `InetAddressPoint`.
Pinging @elastic/es-search (:Search/Search) |
- match: {hits.total.value: 1} | ||
- match: {hits.hits.0._source.timestamp: 1998-04-30T14:31:27-05:00} | ||
|
||
# TODO tests for using the ip in a script. there is almost certainly whitelist "fun" 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.
I'd like to grab this in a follow up because it is going to take some hacking to pass.
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.
left a couple of comments
* Processes query bounds into {@code long}s and delegates the | ||
* provided {@code builder} to build a range query. | ||
*/ | ||
public static Query rangeQuery( |
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.
been thinking about these static methods that we are sharing here and there: should we have a better way to formalize the fact that runtime fields share some bits with their corresponding concrete field? e.g. should they share some base class or something along those lines? Or would it make any sense for runtime field type to extend its corresponding concrete field type? probably both are bad ideas, but probably good to look into what the alternatives are.
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 talked with @javanna and we decided that we should keep this in mind, but we don't really have a cleaner way to do it for now. There are certainly other ways, but they aren't obviously cleaner.
* reasons to do this: | ||
* <ul> | ||
* <li>{@link Inet4Address}es and {@link Inet6Address} are not comparable with one another. | ||
* That is correct in some contexts, but not for our queries. Our queries most consider the |
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.
s/most/must ?
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.
👍
import java.io.IOException; | ||
import java.net.InetAddress; | ||
|
||
public class ScriptIpScriptDocValues extends ScriptDocValues<String> { |
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.
double checking that this is the name you meant to use. I struggle to see what this class does
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 is a pretty terrible name! I talked with @javanna and it will think it over some more. Or more it so it is a bit more obvious.
This implements the
ip
typed runtime fields. They share a fair bitwith
string
runtime fields but we represent them as aBytesRef
containing 128 bits so that the comparisons all happen in the same way
as Lucene's
InetAddressPoint
.