Lucene - Core
  1. Lucene - Core
  2. LUCENE-1687

Remove ExtendedFieldCache by rolling functionality into FieldCache

    Details

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

      Description

      It is silly that we have ExtendedFieldCache. It is a workaround to our supposed back compatibility problem. This patch will merge the ExtendedFieldCache interface into FieldCache, thereby breaking back compatibility, but creating a much simpler API for FieldCache.

        Issue Links

          Activity

          Hide
          Yonik Seeley added a comment -

          As previously noted, FieldCache.DEFAULT is final because FieldCache is an interface, so no one could have had a different implementation they were using with Lucene, hence it would not have broken back compat in a meaningful way to add some new methods.

          Show
          Yonik Seeley added a comment - As previously noted, FieldCache.DEFAULT is final because FieldCache is an interface, so no one could have had a different implementation they were using with Lucene, hence it would not have broken back compat in a meaningful way to add some new methods.
          Hide
          Grant Ingersoll added a comment -

          True, but you know how we are about adding methods to an interface!

          Show
          Grant Ingersoll added a comment - True, but you know how we are about adding methods to an interface!
          Hide
          Michael McCandless added a comment -

          +1

          Show
          Michael McCandless added a comment - +1
          Hide
          Uwe Schindler added a comment -

          True, but you know how we are about adding methods to an interface!

          Yonik said it: Because nobody can change a static variable in an interface (its always final), there is no posibility to change the cache impl, so nobody would ever implement FieldCache. A short notice should be enough.

          Should I fix this together with LUCENE-1701? If yes, I would assign myself to this issue and add this to my own patch (which also fixes some issues by making the default parsers public).

          I would then only deprecate ExtendedFieldCache (as sub-interface of FieldCache, without new methods) and merge the two package-private impls behind.

          Show
          Uwe Schindler added a comment - True, but you know how we are about adding methods to an interface! Yonik said it: Because nobody can change a static variable in an interface (its always final), there is no posibility to change the cache impl, so nobody would ever implement FieldCache. A short notice should be enough. Should I fix this together with LUCENE-1701 ? If yes, I would assign myself to this issue and add this to my own patch (which also fixes some issues by making the default parsers public). I would then only deprecate ExtendedFieldCache (as sub-interface of FieldCache, without new methods) and merge the two package-private impls behind.
          Hide
          Grant Ingersoll added a comment -

          Go for it. No need to deprecate EFC, just delete it.

          Show
          Grant Ingersoll added a comment - Go for it. No need to deprecate EFC, just delete it.
          Hide
          Uwe Schindler added a comment -

          Removing ExtendedFieldCache complete would break backwards compatibility, e.g. when users implemented an own LongParser and so on. And code referring to ExtendedFieldCache.EXT_DEFAULT would also break.

          I changed ExtendedFieldCache to only export this really needed and named parts:

          /**
           * This interface is obsolete, use {@link FieldCache} instead.
           * @deprecated Will be removed in Lucene 3.0
           **/
          public interface ExtendedFieldCache extends FieldCache {
            
            /** @deprecated Use {@link FieldCache#DEFAULT}; this will be removed in Lucene 3.0 */
            public static ExtendedFieldCache EXT_DEFAULT = (ExtendedFieldCache)FieldCache.DEFAULT;
            
            /** @deprecated Use {@link FieldCache.LongParser}, this will be removed in Lucene 3.0 */
            public interface LongParser extends FieldCache.LongParser {
            }
          
            /** @deprecated Use {@link FieldCache.DoubleParser}, will be removed in Lucene 3.0 */
            public interface DoubleParser extends FieldCache.DoubleParser {
            }
          
          }
          
          Show
          Uwe Schindler added a comment - Removing ExtendedFieldCache complete would break backwards compatibility, e.g. when users implemented an own LongParser and so on. And code referring to ExtendedFieldCache.EXT_DEFAULT would also break. I changed ExtendedFieldCache to only export this really needed and named parts: /** * This interface is obsolete, use {@link FieldCache} instead. * @deprecated Will be removed in Lucene 3.0 **/ public interface ExtendedFieldCache extends FieldCache { /** @deprecated Use {@link FieldCache#DEFAULT}; this will be removed in Lucene 3.0 */ public static ExtendedFieldCache EXT_DEFAULT = (ExtendedFieldCache)FieldCache.DEFAULT; /** @deprecated Use {@link FieldCache.LongParser}, this will be removed in Lucene 3.0 */ public interface LongParser extends FieldCache.LongParser { } /** @deprecated Use {@link FieldCache.DoubleParser}, will be removed in Lucene 3.0 */ public interface DoubleParser extends FieldCache.DoubleParser { } }
          Hide
          Grant Ingersoll added a comment - - edited

          The whole point of this issue is that it breaks back compatibility. EFC is a pointless class. Read the description.

          Besides the fact that adding methods to FieldCache itself breaks back compatibility since it is an interface, even if no one can implement it.

          Show
          Grant Ingersoll added a comment - - edited The whole point of this issue is that it breaks back compatibility. EFC is a pointless class. Read the description. Besides the fact that adding methods to FieldCache itself breaks back compatibility since it is an interface, even if no one can implement it.
          Hide
          Uwe Schindler added a comment - - edited

          It breaks backwards compatibility in the case that somebody implements FieldCache/ExtendedFieldCache. As Yonik pointed out, this is unlikely the case, as there is no possibility to plug this implementation into Lucene, because you cannot change the FieldCache.DEFAULT interface variable (static final). Because of this nobody would ever want to implement this interface, and if he does, it would be nonsense and can break without problems.

          So, removing/changing this interfaces would not be a BW break, as long as we only think of implementing FieldCache/ExtendedFieldCache

          But we would really break backwards compatibility for all who refer to this interface! And because of this, the dummy stub keeps there, that classes still referencing parsers and ExtendedFieldCache.EXT_DEFAULT can still work.

          We should here think about not that this theoretically breaks bw, we should discuss where this break would affect users.

          Show
          Uwe Schindler added a comment - - edited It breaks backwards compatibility in the case that somebody implements FieldCache/ExtendedFieldCache. As Yonik pointed out, this is unlikely the case, as there is no possibility to plug this implementation into Lucene, because you cannot change the FieldCache.DEFAULT interface variable (static final). Because of this nobody would ever want to implement this interface, and if he does, it would be nonsense and can break without problems. So, removing/changing this interfaces would not be a BW break, as long as we only think of implementing FieldCache/ExtendedFieldCache But we would really break backwards compatibility for all who refer to this interface! And because of this, the dummy stub keeps there, that classes still referencing parsers and ExtendedFieldCache.EXT_DEFAULT can still work. We should here think about not that this theoretically breaks bw, we should discuss where this break would affect users.
          Hide
          Grant Ingersoll added a comment -

          Uwe,

          The ENTIRE point of this issue is to get rid of EFC. It is purposely breaking backwards compatibility. It is a stupid class put in for stupid reasons. If anyone is referring to this class, they can change to use the FieldCache.

          If you're not going to remove it, I will.

          Show
          Grant Ingersoll added a comment - Uwe, The ENTIRE point of this issue is to get rid of EFC. It is purposely breaking backwards compatibility. It is a stupid class put in for stupid reasons. If anyone is referring to this class, they can change to use the FieldCache. If you're not going to remove it, I will.
          Hide
          Yonik Seeley added a comment -

          Uwe is right - EFC has been around since 2.3, we should not delete it, but deprecate it.
          EFC's extra "stuff" should be moved to FieldCache.

          Show
          Yonik Seeley added a comment - Uwe is right - EFC has been around since 2.3, we should not delete it, but deprecate it. EFC's extra "stuff" should be moved to FieldCache.
          Hide
          Grant Ingersoll added a comment -

          Why?

          So much for case-by-case back compatibility.

          Show
          Grant Ingersoll added a comment - Why? So much for case-by-case back compatibility.
          Hide
          Grant Ingersoll added a comment -

          And Yonik, if you're argument is b/c Solr uses it, I will change it. It's like 5 lines of code.

          Show
          Grant Ingersoll added a comment - And Yonik, if you're argument is b/c Solr uses it, I will change it. It's like 5 lines of code.
          Hide
          Yonik Seeley added a comment -

          And Yonik, if you're argument is b/c Solr uses it, I will change it. It's like 5 lines of code.

          Not at all the issue - as you say, it's simple to change in Solr and doesn't represent a back compat issue to Solr users.

          So much for case-by-case back compatibility.

          This is entirely case-by-case:
          case #1: adding methods to FieldCache could technically be viewed as breaking back compat, but in this specific case it's OK since no one implements FieldCache.
          case #2: removing ExtendedFieldCache breaks all applications that refer to ExtendedFieldCache. it should be deprecated first.

          Show
          Yonik Seeley added a comment - And Yonik, if you're argument is b/c Solr uses it, I will change it. It's like 5 lines of code. Not at all the issue - as you say, it's simple to change in Solr and doesn't represent a back compat issue to Solr users. So much for case-by-case back compatibility. This is entirely case-by-case: case #1: adding methods to FieldCache could technically be viewed as breaking back compat, but in this specific case it's OK since no one implements FieldCache. case #2: removing ExtendedFieldCache breaks all applications that refer to ExtendedFieldCache. it should be deprecated first.
          Hide
          Uwe Schindler added a comment -

          I implemented it now, when removing EFC completely, a lot of tests from backwards 2.4 fail, especially TestSort and TestDateSort and FunctionQuery tests, because they:

          • reference ExtendedFieldCache for parsers
          • reference EXT_DEFAULT

          There are a lot of projects that use this references, e.g. Compass and many more.

          I left over a stub like noted above - and suddenly, all tests pass, only TestExtendedFieldCache, which is obsolete (will be removed from Tag). Nobody is hurted by this stub (it is just an empty declaration), all code is in FieldCache and FieldCacheImpl, as you suggested.

          In 3.0 we remove the stub and voila!

          Show
          Uwe Schindler added a comment - I implemented it now, when removing EFC completely, a lot of tests from backwards 2.4 fail, especially TestSort and TestDateSort and FunctionQuery tests, because they: reference ExtendedFieldCache for parsers reference EXT_DEFAULT There are a lot of projects that use this references, e.g. Compass and many more. I left over a stub like noted above - and suddenly, all tests pass, only TestExtendedFieldCache, which is obsolete (will be removed from Tag). Nobody is hurted by this stub (it is just an empty declaration), all code is in FieldCache and FieldCacheImpl, as you suggested. In 3.0 we remove the stub and voila!
          Hide
          Grant Ingersoll added a comment -

          OK

          Show
          Grant Ingersoll added a comment - OK
          Hide
          Uwe Schindler added a comment -

          Patch available in LUCENE-1701!

          I close this issue after 1701 is committed.

          Show
          Uwe Schindler added a comment - Patch available in LUCENE-1701 ! I close this issue after 1701 is committed.
          Hide
          Uwe Schindler added a comment -

          Committed together with LUCENE-1701 in revision 787723.

          Show
          Uwe Schindler added a comment - Committed together with LUCENE-1701 in revision 787723.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development