Improve testing of mismatched field numbers. #13812
Conversation
This improves testing of mismatched field numbers by - improving `AssertingDocValuesProducer` to detect mismatched field numbers, - introducing a `MismatchedCodecReader` to actually test mismatched field numbers on `DocValuesProducer` (a `MismatchedLeafReader` wrapping a `SlowCodecReaderWrapper` doesn't work since `SlowCodecReaderWrapper` implicitly resolves the correct `FieldInfo` object), - introducing an explicit test for mismatched field numbers in `BaseDocValuesFormatTestCase`. These new tests uncovered a bug when merging sorted doc values, which would call the underlying doc values producer with the merged field info. Closes apache#13805
| @@ -613,7 +613,7 @@ public void mergeSortedField(FieldInfo fieldInfo, final MergeState mergeState) | |||
| if (docValuesProducer != null) { | |||
| FieldInfo readerFieldInfo = mergeState.fieldInfos[i].fieldInfo(fieldInfo.name); | |||
| if (readerFieldInfo != null && readerFieldInfo.getDocValuesType() == DocValuesType.SORTED) { | |||
| values = docValuesProducer.getSorted(fieldInfo); | |||
| values = docValuesProducer.getSorted(readerFieldInfo); | |||
| @@ -229,6 +234,7 @@ static class AssertingDocValuesProducer extends DocValuesProducer { | |||
|
|
|||
| @Override | |||
| public NumericDocValues getNumeric(FieldInfo field) throws IOException { | |||
| assert fieldInfos.fieldInfo(field.name).number == field.number; | |||
|
I can confirm that the previously failing tests seen in the 9.x branch pass successfully with the one-liner fix in this PR. 👍 |
|
The failure seems legit. It happens because of this method in MismatchedCodecReader: if you replace this with return in.getFieldInfos() then it is successful. |
|
We haven't had failures in thes test case for a very long time, so I suspect it's a genuine failure indeed, which got triggered by the additional testing of mismatched field numbers? |
|
Seems the test/functionality uses points and docvalues. Original failing test that inspired this PR I guess my concern here is that fixes/test heres are focused on docvalues, but fieldinfo numbers are used by all index structures and there might be bugs elsewhere too... just do they have the tests to find issues? Thanks for digging into the field numbers issues. |
|
I found the bug, it's in the points merging logic, which assumes that it can look up the internal map<fieldnumber, PointValues> based on the field infos that are returned by the reader. This is incorrect when the reader is a wrapper than renumbers fields such as MismatchedCodecReader. It's unlikely that this bug would have hit anyone in production, but it's still a bug. I'll push a fix and more tests shortly. |
This improves testing of mismatched field numbers by - improving `AssertingDocValuesProducer` to detect mismatched field numbers, - introducing a `MismatchedCodecReader` to actually test mismatched field numbers on `DocValuesProducer` (a `MismatchedLeafReader` wrapping a `SlowCodecReaderWrapper` doesn't work since `SlowCodecReaderWrapper` implicitly resolves the correct `FieldInfo` object), - introducing an explicit test for mismatched field numbers for doc values, points, postings and knn vectors. These new tests uncovered a bug when merging sorted doc values, which would call the underlying doc values producer with the merged field info. Closes #13805
This improves testing of mismatched field numbers by - improving `AssertingDocValuesProducer` to detect mismatched field numbers, - introducing a `MismatchedCodecReader` to actually test mismatched field numbers on `DocValuesProducer` (a `MismatchedLeafReader` wrapping a `SlowCodecReaderWrapper` doesn't work since `SlowCodecReaderWrapper` implicitly resolves the correct `FieldInfo` object), - introducing an explicit test for mismatched field numbers for doc values, points, postings and knn vectors. These new tests uncovered a bug when merging sorted doc values, which would call the underlying doc values producer with the merged field info. Closes #13805
This improves testing of mismatched field numbers by
AssertingDocValuesProducerto detect mismatched field numbers,MismatchedCodecReaderto actually test mismatched field numbers onDocValuesProducer(aMismatchedLeafReaderwrapping aSlowCodecReaderWrapperdoesn't work sinceSlowCodecReaderWrapperimplicitly resolves the correctFieldInfoobject),BaseDocValuesFormatTestCase.These new tests uncovered a bug when merging sorted doc values, which would call the underlying doc values producer with the merged field info.
Closes #13805