Add back range support to ip fields.

[jpountz]

ip fields currently fail range queries when either bound is inclusive. This
commit makes ranges also work in the exclusive case to be consistent with other
data types.

Relates to #17971

[rjernst]

What about passing in the same value for upper and lower, and both inclusive=false. This should be an invalid range right? Do we have a check for that?

[rjernst]

LGTM

[rmuir]

this looks correct to me (for ipv4 and ipv6). we encode into internal format (which is always 128 bits) and treat it as a 128-bit number, adding or subtracting one to the endpoints.

maybe it would be good to test out some of the real corner cases (address already all zeros or all 1s, addresses on the ipv4-mapped boundary). Its not clear to me what will happen, i think probably a strange exception from numericutils.

[jpountz]

@rjernst @rmuir I pushed more tests

[rmuir]

Thanks for the endpoints test. I'm not sure if its worth trying to improve the exception to be more understandable by the user (maybe the generic one could be improved, but it would still be limited to generic terms like underflow/overflow).

As far as the "boundaries" I meant more the cases around ::ffff:0.0.0.0 and ::ffff:255.255.255.255. I can see from your code that should work fine, because you increment/decrement in 128-bit space. But bugs around this area (where addresses change from 4 bytes to 16 bytes) seem likely if the code is refactored.

[rmuir]

Also, I strongly dislike booleans and nulls in the lucene api, but if we are going to support this thing, maybe we should consider methods in InetAddressPoint.java like:

public static InetAddress nextUp(InetAddress address);
public static InetAddress nextDown(InetAddress address);

These could have better error messages for overflow and anyone wanting to do this can do it in a less fragile way.

[rmuir]

Also, it is worth thinking about consistency for these ranges across all types as much as possible. Do integer/long fields throw exceptions in these cases or will Integer.MAX_VALUE exclusive still return Integer.MAX_VALUE?

For double/float fields the behavior may really need to be different, depends if you think -Infinity exclusive will still include -Infinity and so on.

[jpountz]

Do integer/long fields throw exceptions in these cases or will Integer.MAX_VALUE exclusive still return Integer.MAX_VALUE?

They return a MatchNoDocsQuery in that case since there is no greator integer than MAX_VALUE. We could do the same for ip addresses.

For double/float fields the behavior may really need to be different, depends if you think -Infinity exclusive will still include -Infinity and so on.

Right, this is what is happening right now. If you pass POSITIVE_INFINITY as a lower bound and it is exclusive, then we just call Math.nextUp which returns POSITIVE_INFINITY. So POSITIVE_INFINITY will be included in practice.

[jpountz]

maybe we should consider methods in InetAddressPoint.java

I opened https://issues.apache.org/jira/browse/LUCENE-7234.

[rmuir]

Right, this is what is happening right now. If you pass POSITIVE_INFINITY as a lower bound and it is exclusive, then we just call Math.nextUp which returns POSITIVE_INFINITY. So POSITIVE_INFINITY will be included in practice.

Technically i think NaN is better here (it sorts after POSITIVE_INFINITY). In all cases I don't know what to suggest: this is why i really think having an api taking booleans and nulls is unintuitive and too hard to reason about.

All of these problems are artificially created by a bad query DSL.

[jpountz]

I pushed one more commit. I had to fork some Lucene code in order to be able to leverage the new nextUp/nextDown functions and have the fix for newPrefixQuery. Hopefully Lucene 6.1 will be out before elasticsearch 5.0 and we will be able to remove this X class before the GA.

The range method now behaves consistently with integers: if the lower (resp. upper) bound is the maximum (resp. minimum) value and is not inclusive, then a MatchNoDocsQuery is returned.