Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-7372

factor out a org.apache.lucene.search.FilterWeight class

    Details

    • Type: Task
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.x, 7.0, 6.2
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      • FilterWeight to delegate method implementations to the Weight that it wraps
      • exception: no delegating for the bulkScorer method implementation since currently not all FilterWeights implement/override that default method
      1. LUCENE-7372.patch
        21 kB
        Christine Poerschke
      2. LUCENE-7372.patch
        23 kB
        Christine Poerschke
      3. LUCENE-7372.patch
        22 kB
        Christine Poerschke
      4. LUCENE-7372.patch
        22 kB
        Christine Poerschke

        Activity

        Hide
        jpountz Adrien Grand added a comment -

        This should probably be called FilterWeight to be consistent with the rest of the code base? Also, why does its constructor take a Query object too? I think it should only take a Weight object and use the query returned by Weight.getQuery() to pass to the parent constructor? Otherwise +1.

        Show
        jpountz Adrien Grand added a comment - This should probably be called FilterWeight to be consistent with the rest of the code base? Also, why does its constructor take a Query object too? I think it should only take a Weight object and use the query returned by Weight.getQuery() to pass to the parent constructor? Otherwise +1.
        Hide
        cpoerschke Christine Poerschke added a comment -

        Thanks Adrien Grand for the quick review! Added extra constructor

        protected FilterWeight(Weight weight) {
          super(weight.getQuery());
          this.in = weight;
        }
        

        as you suggested. It seems though that the FilterWeight(Query query, Weight weight) constructor variant is still needed to cater for BlockJoinWeight and ToChildBlockJoinWeight usage?

        Show
        cpoerschke Christine Poerschke added a comment - Thanks Adrien Grand for the quick review! Added extra constructor protected FilterWeight(Weight weight) { super (weight.getQuery()); this .in = weight; } as you suggested. It seems though that the FilterWeight(Query query, Weight weight) constructor variant is still needed to cater for BlockJoinWeight and ToChildBlockJoinWeight usage?
        Hide
        dsmiley David Smiley added a comment -

        +1 nice. Remember to add at least a one liner javadoc at the class level; precommit will complain if you don't.

        Show
        dsmiley David Smiley added a comment - +1 nice. Remember to add at least a one liner javadoc at the class level; precommit will complain if you don't.
        Hide
        dsmiley David Smiley added a comment -

        Oh, one more thing – make FilterWeight abstract to clarify it's use.

        Show
        dsmiley David Smiley added a comment - Oh, one more thing – make FilterWeight abstract to clarify it's use.
        Hide
        cpoerschke Christine Poerschke added a comment -

        Thanks David Smiley for your comments, both included in revised patch.

        Show
        cpoerschke Christine Poerschke added a comment - Thanks David Smiley for your comments, both included in revised patch.
        Hide
        jpountz Adrien Grand added a comment -

        Can you add @lucene.internal to the class javadocs and add a comment to the constructor that takes a Query parameter to be explicit about the fact that this query should be rewritten? Otherwise +1 to push.

        Show
        jpountz Adrien Grand added a comment - Can you add @lucene.internal to the class javadocs and add a comment to the constructor that takes a Query parameter to be explicit about the fact that this query should be rewritten? Otherwise +1 to push.
        Hide
        cpoerschke Christine Poerschke added a comment - - edited

        Can you add @lucene.internal to the class javadocs ...

        Just to confirm, @lucene.internal or @lucene.experimental or perhaps both?

        I notice that FilterCollector and FilterLeafCollector are marked @lucene.experimental but other org.apache.lucene.search Filter classes are unmarked.

        Should they (FilteredDocIdSetIterator, FilterScorer, FilterSpans) be marked also and if so (outside the scope of this ticket) as what?

        Similar question would apply to org.apache.lucene.index Filter classes e.g. FilterCodecReader, FilterDirectoryReader, FilteredTermsEnum, FilterLeafReader and possibly other classes too.

        Show
        cpoerschke Christine Poerschke added a comment - - edited Can you add @lucene.internal to the class javadocs ... Just to confirm, @lucene.internal or @lucene.experimental or perhaps both? I notice that FilterCollector and FilterLeafCollector are marked @lucene.experimental but other org.apache.lucene.search Filter classes are unmarked. Should they ( FilteredDocIdSetIterator , FilterScorer , FilterSpans ) be marked also and if so (outside the scope of this ticket) as what? Similar question would apply to org.apache.lucene.index Filter classes e.g. FilterCodecReader , FilterDirectoryReader , FilteredTermsEnum , FilterLeafReader and possibly other classes too.
        Hide
        jpountz Adrien Grand added a comment -

        Building your own query/weight is expert, so I'm more in favour of internal. While we want the API to consume queries to be stable, since this is what Lucene is about, I don't see it as a goal as far as implementing custom queries/weights is concerned.

        Regarding the other Filter* classes, the annotation is indeed not used consistently. See also this comment: https://issues.apache.org/jira/browse/LUCENE-7123?focusedCommentId=15204103&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15204103.

        Show
        jpountz Adrien Grand added a comment - Building your own query/weight is expert, so I'm more in favour of internal. While we want the API to consume queries to be stable, since this is what Lucene is about, I don't see it as a goal as far as implementing custom queries/weights is concerned. Regarding the other Filter* classes, the annotation is indeed not used consistently. See also this comment: https://issues.apache.org/jira/browse/LUCENE-7123?focusedCommentId=15204103&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15204103 .
        Hide
        cpoerschke Christine Poerschke added a comment -
        • rebased for LUCENE-7368 changes and conflicts resolved
        • @lucene.internal annotation
        • javadocs for the two constructors
        Show
        cpoerschke Christine Poerschke added a comment - rebased for LUCENE-7368 changes and conflicts resolved @lucene.internal annotation javadocs for the two constructors
        Hide
        jpountz Adrien Grand added a comment -

        +1

        Show
        jpountz Adrien Grand added a comment - +1
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 15e8719b8aa80b1e7e8deeba6bf8bec99f663ac8 in lucene-solr's branch refs/heads/master from Christine Poerschke
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=15e8719 ]

        LUCENE-7372: Factor out an org.apache.lucene.search.FilterWeight class.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 15e8719b8aa80b1e7e8deeba6bf8bec99f663ac8 in lucene-solr's branch refs/heads/master from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=15e8719 ] LUCENE-7372 : Factor out an org.apache.lucene.search.FilterWeight class.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 625979113f8f1c5e33ed342645aff0594245ca46 in lucene-solr's branch refs/heads/branch_6x from Christine Poerschke
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6259791 ]

        LUCENE-7372: Factor out an org.apache.lucene.search.FilterWeight class.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 625979113f8f1c5e33ed342645aff0594245ca46 in lucene-solr's branch refs/heads/branch_6x from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6259791 ] LUCENE-7372 : Factor out an org.apache.lucene.search.FilterWeight class.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 5c178196184f2bd0e57da510869e44d3177ac679 in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5c17819 ]

        LUCENE-7372: Add org.apache.lucene.search.FilterWeight class + test.

        (This adds the class and test only, as a partial cherry-pick of the branch_5x commit.)

        Show
        jira-bot ASF subversion and git services added a comment - Commit 5c178196184f2bd0e57da510869e44d3177ac679 in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5c17819 ] LUCENE-7372 : Add org.apache.lucene.search.FilterWeight class + test. (This adds the class and test only, as a partial cherry-pick of the branch_5x commit.)

          People

          • Assignee:
            cpoerschke Christine Poerschke
            Reporter:
            cpoerschke Christine Poerschke
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development