Fix range query for NumericDocValues for float and double values

[HoustonPutman]

Description

Fixes #16573 by reverting #15740 and #15760.

I've also incorporated @hossman 's test, so thank you for that Hoss.

This is a back-incompat change from 10.5, because it is removing the public NumericFieldStats class (even though it is listed as experimental). This is removed, because there is no way for the NumericFieldStats class to know whether or not the Point and DocValues data are encoded similarly. (For SortedNumericDocValues, this can be assumed. So we could just limit to that, but it gets tricky at that point)

[uschindler]

We should possibly not backport the deletion of the NumericFieldStats class and keep it in 10.x (deprecated with big warnings).

[uschindler]

Maybe we can remove it, as it was introduced in 10.5.0 and it was buggy from beginning.

[romseygeek]

Catching up here... is the issue that the min and max values as stored in Points don't necessarily translate to the actual min and max double values? I think we need to check that that we aren't making this assumption in other places as well, eg looking at IndexSortSortedNumericDocValuesRangeQuery we use IntPoint.pack() and LongPoint.pack()?

+1 to reverting the change to SortedNumericDocValuesRangeQuery, but I think NumericFieldStats is worth keeping. We need to update it though to take a decoding parameter, and add a bunch more tests. In general I think we don't have enough tests for non-integral numeric values. I can pick that up.

[ChrisHegarty]

cc @salvatorecampagna

[salvatorecampagna]

Thanks @hossman for the repro and the float/double test, nice catch. +1 to reverting.

Reading back through it, the real issue is that NumericFieldStats.getStats() sources the min/max from points while SortedNumericDocValuesRangeQuery executes against doc values, and the index never guarantees those two structures encode (or even hold) the same values. So for negative floats and doubles the stats read from points can land in a different number space than the query bounds, depending on how the doc values were encoded.

On keeping NumericFieldStats with a decoder param (@romseygeek): a decoder does fix how we interpret point values (the current decodeLong hardcodes one encoding, which is its own latent bug), but I don't think it addresses this particular failure, since we would still be feeding stats read from points into a doc values query. For the doc values range path the DocValuesSkipper already gives correct stats in the same space.

My understanding is that the important things here are:

• for a query using doc values, derive stats from doc values (the skipper)
• for a query using points, derive stats from points

That said, since each path already has its own stats source, I would lean toward removing it rather than keeping even separate methods. The whole point of NumericFieldStats was to produce stats no matter whether they come from points or the skipper, and that is exactly what is unsafe here, because points and doc values are not guaranteed to be in the same space. Removing it does not lose any capability, it just removes the footgun.

[msokolov]

once we get this merged I will also copy it to 10.5 branch (or feel free to do so, whomever merges) so we can include in 10.5.2 bugfix release

[HoustonPutman]

Yeah I thought about keeping NumericFieldStats, but it just got complicated. Because if the class is intended to be used by points and if the class is intended to be used by docValues, they need to return different data. So you need a decoder and encoder for both docValues and points. Or at least a bi-directional encoder. Which, just seems complex.

As I said in the issue, it would be amazing if we could enforce changing NumericDocValues to require sorted-order long-enocded floats and doubles (like SortedNumericDocValues does), and then we could really start to reason the same between Points and Numeric/SortedNumeric docValues. But that would be a really big (back-compat-breaking) change even for a major version.

@romseygeek good point about IndexSortSortedNumericDocValuesRangeQuery. We kind of have to revert the functionality unless we know the docValues are in fact SortedNumeric not Numeric.

[romseygeek]

OK, let's remove NumericFieldStats too. I'll take a look at IndexSortSortedNumericDocValuesQuery - like you said, it's a docvalues query, it shouldn't be looking at points in the first place, and you can always use IndexOrDocValuesQuery if you have to same values in both structures.

[romseygeek]

Do you need help unblocking this @HoustonPutman?

[HoustonPutman]

Sorry I was out for a while @romseygeek . I'll get this merged.

[HoustonPutman]

OK, let's remove NumericFieldStats too. I'll take a look at IndexSortSortedNumericDocValuesQuery - like you said, it's a docvalues query, it shouldn't be looking at points in the first place, and you can always use IndexOrDocValuesQuery if you have to same values in both structures.

@romseygeek So while I did say that, I think it's just a poorly named class. It uses either points or docValues, depending. Instead of removing the functionality, it wasn't too hard to add support for doubles and floats (both numeric and sorted_numeric docValue encoded) to IndexSortSortedNumericDocValuesQuery, since we have docValues type and numeric type (through the sort spec). Take a look: #16699