Lucene - Core
  1. Lucene - Core
  2. LUCENE-6039

Add IndexOptions.NO and DocValuesType.NO, instead of null

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.0, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Idea from Simon: it seems dangerous for IndexOptions and DocValuesType
      via Indexable/FieldType and FieldInfo that we use null to mean it's
      not indexed or has no doc values.

      We should instead have an explicit choice (IndexOptions.NO,
      DocValuesType.NO) in the enum?

      1. LUCENE-6039.patch
        230 kB
        Michael McCandless

        Activity

        Hide
        Michael McCandless added a comment -

        Patch, tests pass, but this is really quite a dangerous change since I
        could easily (and likely did) miss places in the code that still think
        null means "not indexed" or "no doc values".

        I tried adding @NotNull annotations and ran code inspection with
        Intellij but was unable to get anything useful out of it: blatant
        violations weren't caught, and trivial things were caught, or maybe
        I just ran it wrong... If anyone has experience getting @NotNull/NonNull
        to report useful issues please help

        I also pulled DocValuesType and IndexOptions out of FieldInfo.java
        into their own sources (in oal.index), and renamed
        IndexOptions.DOCS_ONLY -> DOCS.

        Show
        Michael McCandless added a comment - Patch, tests pass, but this is really quite a dangerous change since I could easily (and likely did) miss places in the code that still think null means "not indexed" or "no doc values". I tried adding @NotNull annotations and ran code inspection with Intellij but was unable to get anything useful out of it: blatant violations weren't caught, and trivial things were caught, or maybe I just ran it wrong... If anyone has experience getting @NotNull/NonNull to report useful issues please help I also pulled DocValuesType and IndexOptions out of FieldInfo.java into their own sources (in oal.index), and renamed IndexOptions.DOCS_ONLY -> DOCS.
        Hide
        Uwe Schindler added a comment -

        +1

        Null is always bad. Its horrible to use null for empty collections instead of Collections.emptyList() or like that. To me the other worst thing is the "null" Scorer or "null" DocIdSets.... To me a query hitting no docs, should return an Empty Scorer implementation.

        Show
        Uwe Schindler added a comment - +1 Null is always bad. Its horrible to use null for empty collections instead of Collections.emptyList() or like that. To me the other worst thing is the "null" Scorer or "null" DocIdSets.... To me a query hitting no docs, should return an Empty Scorer implementation.
        Hide
        Robert Muir added a comment -

        Thank you Mike. I will review the patch.

        Show
        Robert Muir added a comment - Thank you Mike. I will review the patch.
        Hide
        Robert Muir added a comment -

        +1 (i would commit it soon, so it doesnt go out of date).

        When reviewing usages, i noticed some cases like this (SegmentReader):

        if (fieldInfo.getDocValuesType() == null) {
        

        We should open a followup issue to decide what to do about helper methods of hasDocValues()/isIndexed()... either cutover consistently or remove the helpers?

        Show
        Robert Muir added a comment - +1 (i would commit it soon, so it doesnt go out of date). When reviewing usages, i noticed some cases like this (SegmentReader): if (fieldInfo.getDocValuesType() == null ) { We should open a followup issue to decide what to do about helper methods of hasDocValues()/isIndexed()... either cutover consistently or remove the helpers?
        Hide
        Michael McCandless added a comment -

        Thanks, I'll commit shortly, and open a followup: I think we should remove the sugar methods, since they make it seem like there are somehow still separate booleans storing this information in the low schema.

        Show
        Michael McCandless added a comment - Thanks, I'll commit shortly, and open a followup: I think we should remove the sugar methods, since they make it seem like there are somehow still separate booleans storing this information in the low schema.
        Hide
        ASF subversion and git services added a comment -

        Commit 1635790 from Michael McCandless in branch 'dev/trunk'
        [ https://svn.apache.org/r1635790 ]

        LUCENE-6039: cutover to IndexOptions.NO/DocValuesType.NO instead of null

        Show
        ASF subversion and git services added a comment - Commit 1635790 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1635790 ] LUCENE-6039 : cutover to IndexOptions.NO/DocValuesType.NO instead of null
        Hide
        ASF subversion and git services added a comment -

        Commit 1635804 from Michael McCandless in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1635804 ]

        LUCENE-6039: cutover to IndexOptions.NO/DocValuesType.NO instead of null

        Show
        ASF subversion and git services added a comment - Commit 1635804 from Michael McCandless in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1635804 ] LUCENE-6039 : cutover to IndexOptions.NO/DocValuesType.NO instead of null
        Hide
        ASF subversion and git services added a comment -

        Commit 1635807 from Michael McCandless in branch 'dev/trunk'
        [ https://svn.apache.org/r1635807 ]

        LUCENE-6039: a few more null checks

        Show
        ASF subversion and git services added a comment - Commit 1635807 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1635807 ] LUCENE-6039 : a few more null checks
        Hide
        Simon Willnauer added a comment -

        thanks for fixing this mike!

        Show
        Simon Willnauer added a comment - thanks for fixing this mike!
        Hide
        Ryan Ernst added a comment -

        I realize I'm late to the party but...does anyone have an issue with these values being renamed to "NONE"? "NO" is...very odd sounding.

        Show
        Ryan Ernst added a comment - I realize I'm late to the party but...does anyone have an issue with these values being renamed to "NONE"? "NO" is...very odd sounding.
        Hide
        Michael McCandless added a comment -

        +1 for NONE; I'll change it.

        Show
        Michael McCandless added a comment - +1 for NONE; I'll change it.
        Hide
        ASF subversion and git services added a comment -

        Commit 1635861 from Michael McCandless in branch 'dev/trunk'
        [ https://svn.apache.org/r1635861 ]

        LUCENE-6039: NO -> NONE

        Show
        ASF subversion and git services added a comment - Commit 1635861 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1635861 ] LUCENE-6039 : NO -> NONE
        Hide
        ASF subversion and git services added a comment -

        Commit 1635864 from Michael McCandless in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1635864 ]

        LUCENE-6039: NO -> NONE

        Show
        ASF subversion and git services added a comment - Commit 1635864 from Michael McCandless in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1635864 ] LUCENE-6039 : NO -> NONE
        Hide
        ASF subversion and git services added a comment -

        Commit 1637541 from Michael McCandless in branch 'dev/trunk'
        [ https://svn.apache.org/r1637541 ]

        LUCENE-6039: add another null check

        Show
        ASF subversion and git services added a comment - Commit 1637541 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1637541 ] LUCENE-6039 : add another null check
        Hide
        ASF subversion and git services added a comment -

        Commit 1637542 from Michael McCandless in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1637542 ]

        LUCENE-6039: add another null check

        Show
        ASF subversion and git services added a comment - Commit 1637542 from Michael McCandless in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1637542 ] LUCENE-6039 : add another null check
        Hide
        Anshum Gupta added a comment -

        Bulk close after 5.0 release.

        Show
        Anshum Gupta added a comment - Bulk close after 5.0 release.

          People

          • Assignee:
            Michael McCandless
            Reporter:
            Michael McCandless
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development