Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      Since caching is built into the public BitSet bits(IndexReader reader) method, I don't see a way to deprecate that, which means I'll just cut it out and document it in CHANGES.txt. Anyone who wants QueryFilter caching will be able to get the caching back by wrapping the QueryFilter in the CachingWrapperFilter.

      1. LUCENE-857.patch
        2 kB
        Otis Gospodnetic
      2. LUCENE-857.refactoring-approach.diff
        6 kB
        Hoss Man

        Activity

        Hide
        Otis Gospodnetic added a comment -

        QueryFilter without caching.
        I'll commit it tomorrow (Friday) if nobody complains.

        Show
        Otis Gospodnetic added a comment - QueryFilter without caching. I'll commit it tomorrow (Friday) if nobody complains.
        Hide
        Hoss Man added a comment -

        From email since i didn't notice Otis opened this issue already...

        Date: Thu, 5 Apr 2007 14:24:31 -0700 (PDT)
        To: java-dev@lucene.apache.org
        Subject: Re: Caching in QueryFilter - why?

        : Since caching is built into the public BitSet bits(IndexReader reader)
        : method, I don't see a way to deprecate that, which means I'll just cut
        : it out and document it in CHANGES.txt. Anyone who wants QueryFilter
        : caching will be able to get the caching back by wrapping the QueryFilter
        : in your CachingWrapperFilter.

        this seems like a potentially big surprise for people when upgrading ...
        old code will continue to work fine without warning, just get a lot less
        efficient.

        If the concern is duplicated code, perhaps QueryFilter should be
        deprecated and changed to be a subclass of CachingWrapperFilter, with a
        constructor that takes in the Query and wraps it in some new class
        (QueryWrapperFilter perhaps?) that does the meaty part (collecting the
        matches) ...

        @deprecated use CachingWrapperFilter and QueryWrapperFilter directly
        public class QueryFilter extends CachingWrapperFilter {
        public QueryFilter(Query query)

        { super(new QueryWrapperFilter(query)); }

        }

        public class QueryWrapperFilter extends Filter {
        private Query query;
        public QueryWrapperFilter(Query query)

        { this.query = query; }

        public BitSet bits(IndexReader reader) throws IOException {
        final BitSet bits = new BitSet(reader.maxDoc());
        new IndexSearcher(reader).search(query, new HitCollector() {
        public final void collect(int doc, float score)

        { bits.set(doc); // set bit for hit }

        });
        return bits;
        }
        }

        (obviously we need some toString, equals, and hashCode methods in here as well)

        Show
        Hoss Man added a comment - From email since i didn't notice Otis opened this issue already... Date: Thu, 5 Apr 2007 14:24:31 -0700 (PDT) To: java-dev@lucene.apache.org Subject: Re: Caching in QueryFilter - why? : Since caching is built into the public BitSet bits(IndexReader reader) : method, I don't see a way to deprecate that, which means I'll just cut : it out and document it in CHANGES.txt. Anyone who wants QueryFilter : caching will be able to get the caching back by wrapping the QueryFilter : in your CachingWrapperFilter. this seems like a potentially big surprise for people when upgrading ... old code will continue to work fine without warning, just get a lot less efficient. If the concern is duplicated code, perhaps QueryFilter should be deprecated and changed to be a subclass of CachingWrapperFilter, with a constructor that takes in the Query and wraps it in some new class (QueryWrapperFilter perhaps?) that does the meaty part (collecting the matches) ... @deprecated use CachingWrapperFilter and QueryWrapperFilter directly public class QueryFilter extends CachingWrapperFilter { public QueryFilter(Query query) { super(new QueryWrapperFilter(query)); } } public class QueryWrapperFilter extends Filter { private Query query; public QueryWrapperFilter(Query query) { this.query = query; } public BitSet bits(IndexReader reader) throws IOException { final BitSet bits = new BitSet(reader.maxDoc()); new IndexSearcher(reader).search(query, new HitCollector() { public final void collect(int doc, float score) { bits.set(doc); // set bit for hit } }); return bits; } } (obviously we need some toString, equals, and hashCode methods in here as well)
        Hide
        Otis Gospodnetic added a comment -

        I think this is where a javadoc and CHANGES.txt come in. People with serious production environments don't just throw new Lucene version on top of the old one without carefully reading CHANGES.txt. I do this every time I upgrade. If one is careless and doesn't read that, then, well...

        What you suggested is okay, but I didn't want to deprecate the whole class, just remove the caching concern from it, as that's what the CWF is concerned with already. With your suggestion one can't get a raw QueryFilter without getting it automatically cached. Isn't this inflexibility uncool?

        Show
        Otis Gospodnetic added a comment - I think this is where a javadoc and CHANGES.txt come in. People with serious production environments don't just throw new Lucene version on top of the old one without carefully reading CHANGES.txt. I do this every time I upgrade. If one is careless and doesn't read that, then, well... What you suggested is okay, but I didn't want to deprecate the whole class, just remove the caching concern from it, as that's what the CWF is concerned with already. With your suggestion one can't get a raw QueryFilter without getting it automatically cached. Isn't this inflexibility uncool?
        Hide
        Hoss Man added a comment -

        I don't think it's a question of being careless about reading the Changelog – I just think that when dealing with a point release, we shouldn't require people to make code changes just to get the same behavior as before ... if this was necessary to fix a bug it would be one thing, but really what we're talking about here is refactoring out a piece of functionality (using a Query as a Filter) so that it can be used independently from another piece of functionality (filter caching) ... since that can be done in a backwards compatible way, why not make it easy for people.

        > With your suggestion one can't get a raw QueryFilter without getting it
        > automatically cached. Isn't this inflexibility uncool?

        ...not quite, I'm suggesting that the "raw" QueryFilter behavior be extracted into a new class (QueryWrapperFilter) and the existing QueryFilter class continue to do exactly what it currently does - but refactored so that there is no duplicate code.

        Show
        Hoss Man added a comment - I don't think it's a question of being careless about reading the Changelog – I just think that when dealing with a point release, we shouldn't require people to make code changes just to get the same behavior as before ... if this was necessary to fix a bug it would be one thing, but really what we're talking about here is refactoring out a piece of functionality (using a Query as a Filter) so that it can be used independently from another piece of functionality (filter caching) ... since that can be done in a backwards compatible way, why not make it easy for people. > With your suggestion one can't get a raw QueryFilter without getting it > automatically cached. Isn't this inflexibility uncool? ...not quite, I'm suggesting that the "raw" QueryFilter behavior be extracted into a new class (QueryWrapperFilter) and the existing QueryFilter class continue to do exactly what it currently does - but refactored so that there is no duplicate code.
        Hide
        Hoss Man added a comment -

        An example of what I'm thinking would make sense from a backwards compatibility standpoint ... javadocs could still use some improvement.

        Show
        Hoss Man added a comment - An example of what I'm thinking would make sense from a backwards compatibility standpoint ... javadocs could still use some improvement.
        Hide
        Otis Gospodnetic added a comment -

        Thanks for the persistence and patience, Hoss. I see the light now! The patch wouldn't apply to QueryFilter, so I made changes manually.
        Committed.

        Show
        Otis Gospodnetic added a comment - Thanks for the persistence and patience, Hoss. I see the light now! The patch wouldn't apply to QueryFilter, so I made changes manually. Committed.
        Hide
        Hoss Man added a comment -

        Actually Otis: for the backwards compatibility to work, QueryFilter needs to extend CachingWrapperFilter with a constructor like...

        public QueryFilter(Query query)

        { super(new QueryWrapperFilter(query)); }

        ...what you've committed eliminates the caching from QueryFilter

        Show
        Hoss Man added a comment - Actually Otis: for the backwards compatibility to work, QueryFilter needs to extend CachingWrapperFilter with a constructor like... public QueryFilter(Query query) { super(new QueryWrapperFilter(query)); } ...what you've committed eliminates the caching from QueryFilter
        Hide
        Otis Gospodnetic added a comment -

        But of course. Thanks for the catch!

        Show
        Otis Gospodnetic added a comment - But of course. Thanks for the catch!

          People

          • Assignee:
            Otis Gospodnetic
            Reporter:
            Otis Gospodnetic
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development