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

      Like LUCENE-6006, here's another pre-cursor for LUCENE-6005
      ... because I think it's important to nail down Lucene's low-schema
      (FieldType/FieldInfos) semantics before adding a high-schema.

      IndexableFieldType.indexed() is redundant with
      IndexableFieldType.indexOptions() != null, so we should remove it,
      codecs shouldn't have to write/read it, high-schema should not configure it, etc.

      Similarly, the FieldInfo.indexed bit is redundant, so I removed it, but I
      left the sugar API (FieldInfo.isIndexed) and implement it as just
      checking IndexOptions != null.

      1. LUCENE-6013.patch
        91 kB
        Michael McCandless
      2. LUCENE-6013.patch
        90 kB
        Michael McCandless
      3. LUCENE-6013.patch
        84 kB
        Michael McCandless
      4. LUCENE-6013.patch
        84 kB
        Michael McCandless

        Activity

        Hide
        Michael McCandless added a comment -

        Patch, I think it's ready.

        I think this one is easier than normsType was because there is no
        equivalent of the dreaded "undead norms" situation: we don't have
        undead postings, I think.

        Show
        Michael McCandless added a comment - Patch, I think it's ready. I think this one is easier than normsType was because there is no equivalent of the dreaded "undead norms" situation: we don't have undead postings, I think.
        Hide
        Robert Muir added a comment -

        Patch looks good.

        I'm afraid about FieldInfosReader/Writer, can you please look this over one more time just to ensure its ok?

        Since it was writing a bit for this before, this is absolutely the most likely place for bugs in the patch... and the worse place for my brain to hit TooManyUnrelatedStyleChangesException when reviewing it

        Show
        Robert Muir added a comment - Patch looks good. I'm afraid about FieldInfosReader/Writer, can you please look this over one more time just to ensure its ok? Since it was writing a bit for this before, this is absolutely the most likely place for bugs in the patch... and the worse place for my brain to hit TooManyUnrelatedStyleChangesException when reviewing it
        Hide
        Michael McCandless added a comment -

        Thanks Rob, sorry about the noise in Lucene50FIS, I'll double check: there is a REAL change here, I completely rewrote how we encode IndexOptions. I found it really confusing how it was OMIT_THIS, STORE_THAT, OMIT_THIS_OTHER_THING before.

        Show
        Michael McCandless added a comment - Thanks Rob, sorry about the noise in Lucene50FIS, I'll double check: there is a REAL change here, I completely rewrote how we encode IndexOptions. I found it really confusing how it was OMIT_THIS, STORE_THAT, OMIT_THIS_OTHER_THING before.
        Hide
        Robert Muir added a comment -

        No, its good to fix the style, its just scary in a 'nuke fieldinfos boolean change'.

        I also feel the encoding of this thing is complex in packing norms/docvalues bit flags?

        Show
        Robert Muir added a comment - No, its good to fix the style, its just scary in a 'nuke fieldinfos boolean change'. I also feel the encoding of this thing is complex in packing norms/docvalues bit flags?
        Hide
        Michael McCandless added a comment -

        New patch, just pulling out the index options as their own byte ... I think this it makes the serializing simpler. I think it's ready.

        Show
        Michael McCandless added a comment - New patch, just pulling out the index options as their own byte ... I think this it makes the serializing simpler. I think it's ready.
        Hide
        Michael McCandless added a comment -

        I'll update the javadocs for the format changes ... and I'll compact the current 3 bit flags.

        Show
        Michael McCandless added a comment - I'll update the javadocs for the format changes ... and I'll compact the current 3 bit flags.
        Hide
        Michael McCandless added a comment -

        New patch, fixing docs and factoring out separate methods for decode/encode of IndexOptions into byte.

        Show
        Michael McCandless added a comment - New patch, fixing docs and factoring out separate methods for decode/encode of IndexOptions into byte.
        Hide
        Robert Muir added a comment -

        Looks much simpler!

        I guess the one thing is the "duplication" of indexed constants (e.g. INDEXED_DOCS_ONLY). I dont think we should make anything sophisticated, lets keep it simple, but right after the last constant we could add something like static assert IndexOptions.values().length == 4 or something?

        Show
        Robert Muir added a comment - Looks much simpler! I guess the one thing is the "duplication" of indexed constants (e.g. INDEXED_DOCS_ONLY). I dont think we should make anything sophisticated, lets keep it simple, but right after the last constant we could add something like static assert IndexOptions.values().length == 4 or something?
        Hide
        Michael McCandless added a comment -

        Good idea Rob: I added the static asserts, for IndexOptions and DocValues. I also removed the separate constants (matching how we handle DocValues), and "collated" the methods so the numbers used for serializing are methods right next to one another.

        Show
        Michael McCandless added a comment - Good idea Rob: I added the static asserts, for IndexOptions and DocValues. I also removed the separate constants (matching how we handle DocValues), and "collated" the methods so the numbers used for serializing are methods right next to one another.
        Hide
        Robert Muir added a comment -

        +1

        We may want to followup with some modification of the name "indexOptions". It would be nice in the future to maybe just call it 'indexed', just have e.g. setIndexed(DOCS_ONLY) or whatever. But we should remove the boolean here first!

        Show
        Robert Muir added a comment - +1 We may want to followup with some modification of the name "indexOptions". It would be nice in the future to maybe just call it 'indexed', just have e.g. setIndexed(DOCS_ONLY) or whatever. But we should remove the boolean here first!
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-6013: remove IndexableFieldType.indexed and FieldInfo.indexed (it's redundant with IndexOptions != null)

        Show
        ASF subversion and git services added a comment - Commit 1633296 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1633296 ] LUCENE-6013 : remove IndexableFieldType.indexed and FieldInfo.indexed (it's redundant with IndexOptions != null)
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-6013: remove IndexableFieldType.indexed and FieldInfo.indexed (it's redundant with IndexOptions != null)

        Show
        ASF subversion and git services added a comment - Commit 1633328 from Michael McCandless in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1633328 ] LUCENE-6013 : remove IndexableFieldType.indexed and FieldInfo.indexed (it's redundant with IndexOptions != null)
        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:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development