Lucene - Core
  1. Lucene - Core
  2. LUCENE-3854

Non-tokenized fields become tokenized when a document is deleted and added back

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 4.0-ALPHA
    • Fix Version/s: None
    • Component/s: core/index
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      https://github.com/bimargulies/lucene-4-update-case is a JUnit test case that seems to show a problem with the current trunk. It creates a document with a Field typed as StringField.TYPE_STORED and a value with a "-" in it. A TermQuery can find the value, initially, since the field is not tokenized.

      Then, the case reads the Document back out through a reader. In the copy of the Document that gets read out, the Field now has the tokenized bit turned on.

      Next, the case deletes and adds the Document. The 'tokenized' bit is respected, so now the field gets tokenized, and the result is that the query on the term with the - in it no longer works.

      So I think that the defect here is in the code that reconstructs the Document when read from the index, and which turns on the tokenized bit.

      I have an ICLA on file so you can take this code from github, but if you prefer I can also attach it here.

        Activity

        Hide
        Michael McCandless added a comment -

        OK I see the problem... it's not a bug, but is a looongstanding trap in Lucene: you cannot retrieve a Document (from IR.document API) and expect it to accurately reflect what you had indexed. Information is lost, eg whether each field was tokenized or not, what the document boost was, fields that were not stored are missing, etc. In this particular case, IR.document will enable "tokenized" for each text field it loads, which then causes the test failure.

        This is a bad trap, since it tricks you into thinking you can load a stored document and reindex it; instead, you have to re-create a new Document with the correct details on how it should be indexed.

        Really, IR.document should not even return a Document/Field.

        Show
        Michael McCandless added a comment - OK I see the problem... it's not a bug, but is a looongstanding trap in Lucene: you cannot retrieve a Document (from IR.document API) and expect it to accurately reflect what you had indexed. Information is lost, eg whether each field was tokenized or not, what the document boost was, fields that were not stored are missing, etc. In this particular case, IR.document will enable "tokenized" for each text field it loads, which then causes the test failure. This is a bad trap, since it tricks you into thinking you can load a stored document and reindex it; instead, you have to re-create a new Document with the correct details on how it should be indexed. Really, IR.document should not even return a Document/Field.
        Hide
        Andrzej Bialecki added a comment - - edited

        I suspect the problem lies in DocuemntStoredFieldVisitor.stringField(...). It uses FieldInfo to populate FieldType of the retrieved field, and there is no information there about the tokenization (so it assumes true by default). AFAIK the information about the tokenization is lost once the document is indexed so it's not possible to retrieve it back, hence the use of a default value.

        (Mike said the same while I was typing this comment ).

        Show
        Andrzej Bialecki added a comment - - edited I suspect the problem lies in DocuemntStoredFieldVisitor.stringField(...). It uses FieldInfo to populate FieldType of the retrieved field, and there is no information there about the tokenization (so it assumes true by default). AFAIK the information about the tokenization is lost once the document is indexed so it's not possible to retrieve it back, hence the use of a default value. (Mike said the same while I was typing this comment ).
        Hide
        Benson Margulies added a comment -

        FWIW, there are blog posts out there with more or less the recipe I followed to get into this pickle.

        Do you want to keep this open for nulling some things in IR.document()? Obviously, not returning a Document at all would be a bit on the violent side.

        Show
        Benson Margulies added a comment - FWIW, there are blog posts out there with more or less the recipe I followed to get into this pickle. Do you want to keep this open for nulling some things in IR.document()? Obviously, not returning a Document at all would be a bit on the violent side.
        Hide
        Uwe Schindler added a comment - - edited

        In my opinion Document for indexing should be different from document retrieved from stored fields (I am argueing all the time about that).

        One simple solution:
        When a field is loaded using StoredFieldsVisitor from index, lets set an internal flag in the document/field instances (e.g. by a pkg-private ctor of Document), so when you try to readd such a loaded document to IndexWriter you get an exception. Very simple and is a good solution for now.

        But I agree with Robert, Document/Field API is messy and trappy in that regard.

        Show
        Uwe Schindler added a comment - - edited In my opinion Document for indexing should be different from document retrieved from stored fields (I am argueing all the time about that). One simple solution: When a field is loaded using StoredFieldsVisitor from index, lets set an internal flag in the document/field instances (e.g. by a pkg-private ctor of Document), so when you try to readd such a loaded document to IndexWriter you get an exception. Very simple and is a good solution for now. But I agree with Robert, Document/Field API is messy and trappy in that regard.
        Hide
        Michael McCandless added a comment -

        FWIW, there are blog posts out there with more or less the recipe I followed to get into this pickle.

        Sigh Bad bad trap.

        When a field is loaded using StoredFieldsVisitor from index, lets set an internal flag in the document/field instances (e.g. by a pkg-private ctor of Document), so when you try to readd such a loaded document to IndexWriter you get an exception. Very simple and is a good solution for now.

        +1

        Show
        Michael McCandless added a comment - FWIW, there are blog posts out there with more or less the recipe I followed to get into this pickle. Sigh Bad bad trap. When a field is loaded using StoredFieldsVisitor from index, lets set an internal flag in the document/field instances (e.g. by a pkg-private ctor of Document), so when you try to readd such a loaded document to IndexWriter you get an exception. Very simple and is a good solution for now. +1
        Hide
        Andrzej Bialecki added a comment -

        +1, though separate classes for input / output documents would be better. Solr uses SolrInputDocument for input and SolrDocument for output, and obviously they are not interchangeable.

        Show
        Andrzej Bialecki added a comment - +1, though separate classes for input / output documents would be better. Solr uses SolrInputDocument for input and SolrDocument for output, and obviously they are not interchangeable.
        Hide
        Robert Muir added a comment -

        though separate classes for input / output documents would be better. Solr uses SolrInputDocument for input and SolrDocument for output, and obviously they are not interchangeable.

        +1

        Show
        Robert Muir added a comment - though separate classes for input / output documents would be better. Solr uses SolrInputDocument for input and SolrDocument for output, and obviously they are not interchangeable. +1
        Hide
        Hoss Man added a comment -

        i tried arguing a long time ago that IndexReader.document(...) should return "Map<String,String[]>" since known of the Document/Field object metdata makes sense at "read" time ... never got any buy in from anybody else.

        Show
        Hoss Man added a comment - i tried arguing a long time ago that IndexReader.document(...) should return "Map<String,String[]>" since known of the Document/Field object metdata makes sense at "read" time ... never got any buy in from anybody else.
        Hide
        Benson Margulies added a comment -

        Notes:

        1) The trap opened a bit wider in 4.0 with the removal of IndexReader.deleteDocument. I'm not sure I exactly understand how, but by deleting through the reader we didn't hit this.

        2) I got into this because I wanted, really, to update a field value. There isn't a better way to attack that problem in 4.0, is there?

        Show
        Benson Margulies added a comment - Notes: 1) The trap opened a bit wider in 4.0 with the removal of IndexReader.deleteDocument. I'm not sure I exactly understand how, but by deleting through the reader we didn't hit this. 2) I got into this because I wanted, really, to update a field value. There isn't a better way to attack that problem in 4.0, is there?
        Hide
        Andrzej Bialecki added a comment -

        There isn't a better way to attack that problem in 4.0, is there?

        Not yet - LUCENE-3837 is still in early stages.

        Show
        Andrzej Bialecki added a comment - There isn't a better way to attack that problem in 4.0, is there? Not yet - LUCENE-3837 is still in early stages.
        Hide
        András Péteri added a comment -

        Isn't this considered a regression from 3.x? In 3.6.0 I'm seeing an additional byte being read from the stream in FieldsReader, which contained bits that allowed the reader to reconstruct the Index enum correctly for the field. This should make it possible to properly update a document in which all fields were stored, with the exception of boost values (and they could be stored redundantly in a field as well to overcome this limitation).

        Show
        András Péteri added a comment - Isn't this considered a regression from 3.x? In 3.6.0 I'm seeing an additional byte being read from the stream in FieldsReader, which contained bits that allowed the reader to reconstruct the Index enum correctly for the field. This should make it possible to properly update a document in which all fields were stored, with the exception of boost values (and they could be stored redundantly in a field as well to overcome this limitation).

          People

          • Assignee:
            Unassigned
            Reporter:
            Benson Margulies
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:

              Development