incorrect results from SortedNumericDocValuesRangeQuery when using double/float based points+NumericDocValuesField

[hossman]

Description

When attempting to upgrade Solr from Lucene 10.4.0 -> 10.5.1 the Solr tests identified a regression causing test failures in (single-valued) float & double based fields that use both points & docvalues: Range queries are failing to match all expected documents.

• Identical Solr tests using multivalued fields still pass.
• Identical Solr tests using only docvalues, or only points also still pass.
• Similar Solr tests using int & long types still pass regardless of multivalue/points/dv permutations

After writing a scaled down Lucene equivilent test, git bisect identified da18ffc5890f72df279373d40ce5c4192734cd11 (aka: #15760) as the cause of the bug.

I'm not an expert here, but the underlying issue seems to be that #15760 modified SortedNumericDocValuesRangeQuery.rewrite to refactor some logic identifying min/max values "on disk" (in order to optimize away queries known to be out of range of any valid docs) into a new helper function (NumericFieldStats.getStats) that not only considers the stats of the docvalues (if they exist), but (starting with #15760) this method also (first) looks at the stats from any PointValues -- and those stats may differ from what's actually in the doc values.

The problem being that there is no guarantee that a PointsField named foo has the same internal long values in it as a DocValues field named foo.

IIUC, fields like DoublePoint and FloatPoint internally use the same encodings methods in NumericUtils that are recommended when using SortedNumericDocValuesField -- but AFAIK there is no requirement that all doubles & floats be encoded that way when using NumericDocValuesField -- which is also supported by SortedNumericDocValuesRangeQuery.

• Solr uses Double.doubleToLongBits with NumericDocValuesField for historic reasons (predating Lucene's NumericUtils IIRC?)
  • likewise for Floats
• Lucene's DoubleDocValuesField helper class uses Double.doubleToRawLongBits
  • likewise for Floats
• Neither seem to safely work with this new NumericFieldStats.getStats() logic

I'm attaching a quickly hacked together patch with a test case demonstrating the problem:

• lucene_bug_caused_by_github15760.patch. This tests passes with Lucene 10.4.0

I suspect the only viable fix here is to remove the PointValues optimizations added in #15760 ... or ... maybe: make the PointValues optimization used if-and-only-if we can confirm from the IndexReader that SortedNumericDocValues are in use -- since SortedNumericDocValues they are documented as requiring the use of the sae NumericUtils encoding for double & float that Points use (IIUC this is easy/cheap to do? ... just check if null != LeafReader.getSortedNumericDocValues() ... correct?)

Version and environment details

Lucene 10.5.1

[HoustonPutman]

I have to say it is really a shame that doubles/floats are expected to be encoded differently in NUMERIC and SORTED_NUMERIC docValues. It makes life much harder in a good number of ways that NUMERIC expects floats and doubles to not be in sorted order (primarily, range queries across zero are no longer easy, they have to be disjunctions). But this issue is another good reason why.

[HoustonPutman]

Also I'm not sure that #15760 is buying anything at all when an IndexOrDocValues query is used. In this case, both the Points range query and the DocValues range query would be optimized, and since the Points data is correct, it would be optimized to a MatchAllDocs query or a MatchNoDocs query. This is just helpful for a user using SortedNumericDocValues directly even though they have points indexed. It just kind of feels wrong to be using the wrong data for the query. The PointRangeQuery does not check the docValuesSkipList, for example.

I think the PR could just be reverted probably...

[hossman]

Also I'm not sure that #15760 is buying anything at all when an IndexOrDocValues query is used. ...

+1

I think the PR could just be reverted probably...

+1

The existence of NumericFieldStats.getStats() feels intrinsically like a trap for developers. If there are long term plans for why it makes sense to to have this as a general utility class so be it -- but then the names of the (public) functions should be very explicit: NumericFieldStats.getStatsFromPoints vs NumericFieldStats.getStatsFromDocValuesSkipper

But if the only caller for any NumericFieldStats methods are SortedNumericDocValuesRangeQuery -- then put the logic back in a private method in SortedNumericDocValuesRangeQuery until such time as multiple callers exist.

The one change i would suggest over a simple "revert the original PR" is the addition of robust range query tests using floats & doubles (like in my attached patch, or in the proposed PR #16575)

[HoustonPutman]

The one change i would suggest over a simple "revert the original PR" is the addition of robust range query tests using floats & doubles (like in my attached patch, or in the proposed PR #16575)

+1 - yeah the test should absolutely make it in.