Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-650

NPE doing local sensitive sorting when sort field is missing

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0
    • Fix Version/s: None
    • Component/s: core/search
    • Labels:
      None

      Description

      If you do a local sensitive sort against a field that is missing from some documents in the index an NPE will get thrown.

      Attached is a patch which resolved the issue and updates the sort test case to give coverage to this issue.

        Activity

        Hide
        doronc Doron Cohen added a comment -

        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.

        • Doron
        Show
        doronc Doron Cohen added a comment - 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. Doron
        Hide
        hossman Hoss Man added a comment -

        Attached patch: demonstrates bug with test, provides fix, applies cleanly, breaks no other existing tests.

        Applied and commited.

        Thanx Oliver

        Show
        hossman Hoss Man added a comment - Attached patch: demonstrates bug with test, provides fix, applies cleanly, breaks no other existing tests. Applied and commited. Thanx Oliver

          People

          • Assignee:
            hossman Hoss Man
            Reporter:
            ohutchison Oliver Hutchison
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development