Lucene - Core
  1. Lucene - Core
  2. LUCENE-1561

Maybe rename Field.omitTf, and strengthen the javadocs

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.9
    • Component/s: core/index
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Spinoff from here:

      http://www.nabble.com/search-problem-when-indexed-using-Field.setOmitTf()-td22456141.html

      Maybe rename omitTf to something like omitTermPositions, and make it clear what queries will silently fail to work as a result.

      1. LUCENE-1561.patch
        5 kB
        Michael McCandless
      2. LUCENE-1561.patch
        35 kB
        Michael McCandless

        Issue Links

          Activity

          Hide
          Michael McCandless added a comment -

          Attached patch. I renamed to "omitTermFreqAndPositions", and added a NOTE to the javadoc about positional queries silently not working when you use this option. I plan to commit in a day or so.

          Show
          Michael McCandless added a comment - Attached patch. I renamed to "omitTermFreqAndPositions", and added a NOTE to the javadoc about positional queries silently not working when you use this option. I plan to commit in a day or so.
          Hide
          Otis Gospodnetic added a comment -

          Might be good to keep a consistent name across Lucene/Solr.
          More info coming up in SOLR-1079.

          Show
          Otis Gospodnetic added a comment - Might be good to keep a consistent name across Lucene/Solr. More info coming up in SOLR-1079 .
          Hide
          Michael McCandless added a comment -

          Naming is the hardest part!!

          Show
          Michael McCandless added a comment - Naming is the hardest part!!
          Hide
          Eks Dev added a comment -

          maybe something along the lines:

          usePureBooleanPostings()
          minimalInvertedList()

          Show
          Eks Dev added a comment - maybe something along the lines: usePureBooleanPostings() minimalInvertedList()
          Hide
          Mike Klaas added a comment -

          I agree that it is going to be almost impossible to convey that phrase queries don't work by renaming the flag. I agree with Eks Dev that a positive formulation is the only chance, although this deviates from the current omit* flags.

          termPresenceOnly()
          trackTermPresenceOnly()
          onlyTermPresence()
          omitEverythingButTermPresence() // just kidding

          Show
          Mike Klaas added a comment - I agree that it is going to be almost impossible to convey that phrase queries don't work by renaming the flag. I agree with Eks Dev that a positive formulation is the only chance, although this deviates from the current omit* flags. termPresenceOnly() trackTermPresenceOnly() onlyTermPresence() omitEverythingButTermPresence() // just kidding
          Hide
          Uwe Schindler added a comment -

          I found a deprecation bug:
          setOmitTf() and other are only deprecated in the Fieldable interface, but not in the implementations. Code using setOmitTf() does not show a warning. So in AbstractField/Field, the @deprecated should be added, too.

          And a problem with SOLR:
          The ChangeLog should clearly say, that the Fieldable interface was again changed. Plugging the new Lucene JAR into Solr fails, because one of the Fieldable implementations of Solr do not have the new methods.
          Maybe we should remove the rename inside the interfae (keep omitTf there) and only change it in the Field/AbstractField class. This would make jar-replacements possible. As most people will not implement Fieldable, I think it can be left out (as it is only a rename). And generally Interface should not have duplicate method declarations with different names because of deprecation... (that looks very bad)

          Show
          Uwe Schindler added a comment - I found a deprecation bug: setOmitTf() and other are only deprecated in the Fieldable interface, but not in the implementations. Code using setOmitTf() does not show a warning. So in AbstractField/Field, the @deprecated should be added, too. And a problem with SOLR: The ChangeLog should clearly say, that the Fieldable interface was again changed. Plugging the new Lucene JAR into Solr fails, because one of the Fieldable implementations of Solr do not have the new methods. Maybe we should remove the rename inside the interfae (keep omitTf there) and only change it in the Field/AbstractField class. This would make jar-replacements possible. As most people will not implement Fieldable, I think it can be left out (as it is only a rename). And generally Interface should not have duplicate method declarations with different names because of deprecation... (that looks very bad)
          Hide
          Michael McCandless added a comment -

          setOmitTf() and other are only deprecated in the Fieldable interface, but not in the implementations. Code using setOmitTf() does not show a warning. So in AbstractField/Field, the @deprecated should be added, too.

          Urgh, I'll add @deprecated to the methods in AbstractField.java.

          Maybe we should remove the rename inside the interfae (keep omitTf there) and only change it in the Field/AbstractField class.

          Darned interface! The problem with this is we use this interface from within Lucene, to access OTFAP, so we'd have to switch those back to deprecated calls, though it does look to be in only a few places. It's also confusing to deprecate it in Fieldable & AbstractField but not offer the non-deprecated variant in Fieldable. Since it's possible we won't get the "cleanup Fieldable/AbstractField/Field" done for 2.9, I'd like in 3.0 to be able to consistently name it in both the Fieldable interface and the AbstractField abstract class. Or are you thinking we'd leave omitTf in Fieldable and then in 3.0 forcefully rename it to omitTermFreqAndPositions?

          Show
          Michael McCandless added a comment - setOmitTf() and other are only deprecated in the Fieldable interface, but not in the implementations. Code using setOmitTf() does not show a warning. So in AbstractField/Field, the @deprecated should be added, too. Urgh, I'll add @deprecated to the methods in AbstractField.java. Maybe we should remove the rename inside the interfae (keep omitTf there) and only change it in the Field/AbstractField class. Darned interface! The problem with this is we use this interface from within Lucene, to access OTFAP, so we'd have to switch those back to deprecated calls, though it does look to be in only a few places. It's also confusing to deprecate it in Fieldable & AbstractField but not offer the non-deprecated variant in Fieldable. Since it's possible we won't get the "cleanup Fieldable/AbstractField/Field" done for 2.9, I'd like in 3.0 to be able to consistently name it in both the Fieldable interface and the AbstractField abstract class. Or are you thinking we'd leave omitTf in Fieldable and then in 3.0 forcefully rename it to omitTermFreqAndPositions?
          Hide
          Uwe Schindler added a comment -

          Wasn't it the plan to remove these interfaces in 3.0?

          We could deprecate Fieldable in complete and leave it as it is.From Lucene 3.0 on we only have AbstractField. So the old Fieldable interface must be used internally until 3.0 (with the deprecated methods), but user-land code like Solr should only overwrite AbstractField and not implement the interface anymore (I am not really sure, why Solr needs this Fieldable implementation at all, it the only place in Solr where problems occur, it would be good to reimplement this internal class using AbstractField).

          Document.add() and all other public appearences of Fieldable should be overloaded with AbstractField counterparts and so on, so that all public API only use the abstract class anymore.

          But thats my opinion, and maybe is related to the other issue "whole document/field reimplementation".

          Show
          Uwe Schindler added a comment - Wasn't it the plan to remove these interfaces in 3.0? We could deprecate Fieldable in complete and leave it as it is.From Lucene 3.0 on we only have AbstractField. So the old Fieldable interface must be used internally until 3.0 (with the deprecated methods), but user-land code like Solr should only overwrite AbstractField and not implement the interface anymore (I am not really sure, why Solr needs this Fieldable implementation at all, it the only place in Solr where problems occur, it would be good to reimplement this internal class using AbstractField). Document.add() and all other public appearences of Fieldable should be overloaded with AbstractField counterparts and so on, so that all public API only use the abstract class anymore. But thats my opinion, and maybe is related to the other issue "whole document/field reimplementation".
          Hide
          Michael McCandless added a comment -

          Wasn't it the plan to remove these interfaces in 3.0?

          That would be fabulous. How would you do it? Collapse everything down to concrete Field (no more interface nor abstract base class)? Make type/use specific subclasses (eg NumericField)? What would you replace IndexReader.document with? Would we better/explicitly represent multi-valued fields?

          We could deprecate Fieldable in complete and leave it as it is.

          I think that's a good fallback if we can't get the full cleanup of Field/Document done for 2.9.

          I guess in either of these two cases, we would want to leave Fieldable unchanged (ie put back only the deprecated omitTf), so I'll make that change now.

          Show
          Michael McCandless added a comment - Wasn't it the plan to remove these interfaces in 3.0? That would be fabulous. How would you do it? Collapse everything down to concrete Field (no more interface nor abstract base class)? Make type/use specific subclasses (eg NumericField)? What would you replace IndexReader.document with? Would we better/explicitly represent multi-valued fields? We could deprecate Fieldable in complete and leave it as it is. I think that's a good fallback if we can't get the full cleanup of Field/Document done for 2.9. I guess in either of these two cases, we would want to leave Fieldable unchanged (ie put back only the deprecated omitTf), so I'll make that change now.
          Hide
          Michael McCandless added a comment -

          Attached patch, also deprecating omitTf in AbstractField, and not adding the newly named methods into Fieldable. I plan to commit in a day or two.

          Show
          Michael McCandless added a comment - Attached patch, also deprecating omitTf in AbstractField, and not adding the newly named methods into Fieldable. I plan to commit in a day or two.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development