I reviewed this patch and think that it is valid.
This seems like a real bug:
- In FieldSortedHitQueue, when no locale is specified, a comparator is based on getStringsIndex() - because the natural terms order can be used for comparison.
- Notice that getStringsIndex() has a special care for docs with no values - their index is set to 0 (default index array initialization) and all terms are placed from index 1 and up.
- But when a locale is specified, the natural terms cannot be used, and hence getStrings() is used (instead of getStringsIndex()). Here there is no special care for "no values", and hence the result strings array can contain nulls.
- These null values are later passed to the locale comparator, and e get wa NPE.
- The Javadocs for java.text.Collator do not specify behavior for null argument, so this can not be regarded as an issue of a certain JDK implementation - we should avoid passing null to the comparator.
I see two ways to solve this:
- one is to assign "" (empty string) in the comparator for all the null values. But this would modify the semantics of the FieldCache, that stores this array of strings. It would be wrong to do so, and it would also take more time.
- the other way - that seems the right way - is that of this patch, which handles null values just before passing them to the collator compare method. (The 3 if statements added may have performance implications - I did not test performance.)
I ran the TestSort (from the patch) without applying the patch fix to FieldSortedHitQueue, and the test failed. After applying the fix to FieldSortedHitQueue the test passed. All "ant test" tests pass as well.