Lucene - Core
  1. Lucene - Core
  2. LUCENE-2314

Add AttributeSource.copyTo(AttributeSource)

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0-ALPHA
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      One problem with AttributeSource at the moment is the missing "insight" into AttributeSource.State. If you want to create TokenStreams that inspect cpatured states, you have no chance. Making the contents of State public is a bad idea, as it does not help for inspecting (its a linked list, so you have to iterate).

      AttributeSource currently contains a cloneAttributes() call, which returns a new AttrubuteSource with all current attributes cloned. This is the (more expensive) captureState. The problem is that you cannot copy back the cloned AS (which is the restoreState). To use this behaviour (by the way, ShingleMatrix can use it), one can alternatively use cloneAttributes and copyTo. You can easily change the cloned attributes and store them in lists and copy them back. The only problem is lower performance of these calls (as State is a very optimized class).

      One use case could be:

      AttributeSource state = cloneAttributes();
      // .... do something ...
      state.getAttribute(TermAttribute.class).setTermBuffer(foobar);
      // ... more work
      state.copyTo(this);
      
      1. LUCENE-2314.patch
        6 kB
        Uwe Schindler
      2. LUCENE-2314.patch
        5 kB
        Uwe Schindler
      3. LUCENE-2314.patch
        3 kB
        Uwe Schindler

        Issue Links

          Activity

          Hide
          Uwe Schindler added a comment -

          Here the patch.

          Show
          Uwe Schindler added a comment - Here the patch.
          Hide
          Shai Erera added a comment -

          Minor comment - in copyTo, can you put state.attribute.getClass() in the message of the thrown exception, so whoever encounters it will know what's the invalid attribute?

          On a more general note, can State implement Iterable?

          Show
          Shai Erera added a comment - Minor comment - in copyTo, can you put state.attribute.getClass() in the message of the thrown exception, so whoever encounters it will know what's the invalid attribute? On a more general note, can State implement Iterable?
          Hide
          Uwe Schindler added a comment -

          Minor comment - in copyTo, can you put state.attribute.getClass() in the message of the thrown exception, so whoever encounters it will know what's the invalid attribute?

          Good idea, the same should be done for restoreState (the code way copied from there).

          On a more general note, can State implement Iterable?

          It could, but as State is itsself a linked list element it would be... strange. But of course we could make it Iterable<AttributeImpl>. But the internal implementations of AttributeSource should not use this interface as it is optimized for speed, so the creation of iterators is a no-go here.

          Show
          Uwe Schindler added a comment - Minor comment - in copyTo, can you put state.attribute.getClass() in the message of the thrown exception, so whoever encounters it will know what's the invalid attribute? Good idea, the same should be done for restoreState (the code way copied from there). On a more general note, can State implement Iterable? It could, but as State is itsself a linked list element it would be... strange. But of course we could make it Iterable<AttributeImpl>. But the internal implementations of AttributeSource should not use this interface as it is optimized for speed, so the creation of iterators is a no-go here.
          Hide
          Shai Erera added a comment -

          I just thought that instead of the for loop you have now you could have written something like: "for (State state : this)" ... a Java 5.0 style iteration. But it's not critical.

          Show
          Shai Erera added a comment - I just thought that instead of the for loop you have now you could have written something like: "for (State state : this)" ... a Java 5.0 style iteration. But it's not critical.
          Hide
          Uwe Schindler added a comment -

          Because of speed we do not do this. That was performance tested in 2.9 development. The for-loop using the linked list directly is far faster. the captureState is one of the most optimized methods.

          Show
          Uwe Schindler added a comment - Because of speed we do not do this. That was performance tested in 2.9 development. The for-loop using the linked list directly is far faster. the captureState is one of the most optimized methods.
          Hide
          Shai Erera added a comment -

          Ok. Performance is always preferred than beautiful looking code .

          Show
          Shai Erera added a comment - Ok. Performance is always preferred than beautiful looking code .
          Hide
          Uwe Schindler added a comment -

          New patch with some improvements in cloneAttributes() and the requested class names in the IAEs.

          Show
          Uwe Schindler added a comment - New patch with some improvements in cloneAttributes() and the requested class names in the IAEs.
          Hide
          Simon Willnauer added a comment -

          Small comment on javadoc wording.

          Maybe like that:

          /**
           * Copies the contents of this AttributeSource to the given AttributeSource.
           * The given instance has to provide all {@link Attribute}s this instance contains. 
           * The actual attribute implementations must be identical in both {@link AttributeSource} instances.
           * Ideally both AttributeSource instances should use the same {@link AttributeFactory} 
           */
          
          Show
          Simon Willnauer added a comment - Small comment on javadoc wording. Maybe like that: /** * Copies the contents of this AttributeSource to the given AttributeSource. * The given instance has to provide all {@link Attribute}s this instance contains. * The actual attribute implementations must be identical in both {@link AttributeSource} instances. * Ideally both AttributeSource instances should use the same {@link AttributeFactory} */
          Hide
          Uwe Schindler added a comment -

          Updated javadocs. Will commit tomorrow.

          Show
          Uwe Schindler added a comment - Updated javadocs. Will commit tomorrow.
          Hide
          Simon Willnauer added a comment -

          looks good to me!

          Show
          Simon Willnauer added a comment - looks good to me!
          Hide
          Uwe Schindler added a comment -

          Committed revision: 922797

          Show
          Uwe Schindler added a comment - Committed revision: 922797

            People

            • Assignee:
              Uwe Schindler
              Reporter:
              Uwe Schindler
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development