Fix compile errors #101874
Fix compile errors #101874
Conversation
IndexDiskUsageAnalyzer needs adjusting after apache/lucene#12741
javanna
added
>non-issue
:Search/Search
|
Pinging @elastic/es-search (Team:Search) |
| import org.apache.lucene.codecs.DocValuesProducer; | ||
| import org.apache.lucene.codecs.FieldsProducer; | ||
| import org.apache.lucene.codecs.KnnVectorsReader; | ||
| import org.apache.lucene.codecs.NormsProducer; | ||
| import org.apache.lucene.codecs.PointsReader; | ||
| import org.apache.lucene.codecs.StoredFieldsReader; | ||
| import org.apache.lucene.codecs.TermVectorsReader; | ||
| import org.apache.lucene.codecs.lucene90.Lucene90PostingsFormat; |
There was a problem hiding this comment.
I am not sure if besides updating the import for 90 we need an additional conditional for 99 here
There was a problem hiding this comment.
It seems like we should have one. I honestly don't know, we have one for other older formats :/
There was a problem hiding this comment.
So we don't forget, could you make that change in this PR or add a TODO?
There was a problem hiding this comment.
I pushed the change, I don't feel good about no failing tests with or without it though.
There was a problem hiding this comment.
I pushed the change, I don't feel good about no failing tests with or without it though.
No test fails because we miss an inverted index unit test, and the assertion in the IT test is too relaxed. There's a strict assertion in a unit test, but it compares the results between the per-field format and the regular format, both of which silently skip a term state if the format is missing. Can you add an assertion at the end of these format checks to ensure we won't forget to add a new format here? I can add an inverted index unit test next week.
It seems like we should have one. I honestly don't know, we have one for other older formats :/
Yes, and we should have BWC tests for this API too.
| import org.apache.lucene.codecs.DocValuesProducer; | ||
| import org.apache.lucene.codecs.FieldsProducer; | ||
| import org.apache.lucene.codecs.KnnVectorsReader; | ||
| import org.apache.lucene.codecs.NormsProducer; | ||
| import org.apache.lucene.codecs.PointsReader; | ||
| import org.apache.lucene.codecs.StoredFieldsReader; | ||
| import org.apache.lucene.codecs.TermVectorsReader; | ||
| import org.apache.lucene.codecs.lucene90.Lucene90PostingsFormat; |
There was a problem hiding this comment.
I pushed the change, I don't feel good about no failing tests with or without it though.
No test fails because we miss an inverted index unit test, and the assertion in the IT test is too relaxed. There's a strict assertion in a unit test, but it compares the results between the per-field format and the regular format, both of which silently skip a term state if the format is missing. Can you add an assertion at the end of these format checks to ensure we won't forget to add a new format here? I can add an inverted index unit test next week.
It seems like we should have one. I honestly don't know, we have one for other older formats :/
Yes, and we should have BWC tests for this API too.
IndexDiskUsageAnalyzer and IndexDiskUsageAnalyzerTests, as well as CompletionFieldMapper, CompletionFieldMapperTests and CompletionStatsCacheTests need adjusting after apache/lucene#12741 , to refer to the latest postings format.
KuromojiTokenizerFactory needs adjusting after apache/lucene#12390