Details

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

      Description

      TopDocCollector's members and constructor are declared either private or package visible. It makes it hard to extend it as if you want to extend it you can reuse its hq and totatlHits members, but need to define your own. It also forces you to override getTotalHits() and topDocs().
      By changing its members and constructor (the one that accepts a PQ) to protected, we allow users to extend it in order to get a different view of 'top docs' (like TopFieldCollector does), but still enjoy its getTotalHits() and topDocs() method implementations.

      1. 1356-2.patch
        2 kB
        Shai Erera
      2. 1356.patch
        0.8 kB
        Shai Erera

        Activity

        Hide
        shaie Shai Erera added a comment -

        The very trivial patch

        Show
        shaie Shai Erera added a comment - The very trivial patch
        Hide
        doronc Doron Cohen added a comment -

        Shai, Thanks for creating this issue and patch!

        I noticed you also modified the private reusableSD to be protected.

        This field is just for avoiding creating a new object at each insert to the priority queue.
        Note that TopFieldDocCollector maintains its own reusable object for this matter, and it of a 'slightly' different type.

        I am wondering if the right thing to do is to (1) leave that field private, or (2) make it protected but then make TopFieldDocCollector use it too.
        I'm inclined for option 1.

        What do you think? Others?

        Show
        doronc Doron Cohen added a comment - Shai, Thanks for creating this issue and patch! I noticed you also modified the private reusableSD to be protected. This field is just for avoiding creating a new object at each insert to the priority queue. Note that TopFieldDocCollector maintains its own reusable object for this matter, and it of a 'slightly' different type. I am wondering if the right thing to do is to (1) leave that field private, or (2) make it protected but then make TopFieldDocCollector use it too. I'm inclined for option 1. What do you think? Others?
        Hide
        shaie Shai Erera added a comment -

        IMO, TopFieldDocCollector should be changed to use reusableSD. FieldDoc extends ScoreDoc. That's the reason I modified it to protected - for extensions of TopDocCollector who maintain in PQ ScoreDoc types (either ScoreDoc or extensions).
        I don't see any advantage in marking it private, nor any disadvantage if any extension to TopDocCollector will maintain its own ScoreDoc instance.
        It's just that we have TopDocCollector, TopFieldDocCollector and my extension to TDC which insert ScoreDoc instances into PQ, so it made sense to me to change it to protected.

        Show
        shaie Shai Erera added a comment - IMO, TopFieldDocCollector should be changed to use reusableSD. FieldDoc extends ScoreDoc. That's the reason I modified it to protected - for extensions of TopDocCollector who maintain in PQ ScoreDoc types (either ScoreDoc or extensions). I don't see any advantage in marking it private, nor any disadvantage if any extension to TopDocCollector will maintain its own ScoreDoc instance. It's just that we have TopDocCollector, TopFieldDocCollector and my extension to TDC which insert ScoreDoc instances into PQ, so it made sense to me to change it to protected.
        Hide
        shaie Shai Erera added a comment -

        Re-thinking this - resuableSD should remain private to both TDC and TFDC. The reason is the two classes use it differently and don't share any implementation which involves this member (unlike totalHits and hq).

        I was in the middle of adding javadoc to the protected members and constructor (which accepts numHits and PQ) when I noticed that numHits is completely ignored by this constructor --> TopDocCollector(int numHits, PriortityQueue hq).
        The reason is that PQ is be probably configured to hold a maximum number of hits.

        What bothers me with this constructor is that it may falsely lead users of the API to think that it limits their PQ with a maximum number of hits. I think we should remove that parameter and expose two constructors:
        public TopDocCollector(int numHits) AND public TopDocCollector(PriorityQueue hq).

        If you agree, I'll reflect that in the 2nd patch I want to create (which adds javadoc).

        Show
        shaie Shai Erera added a comment - Re-thinking this - resuableSD should remain private to both TDC and TFDC. The reason is the two classes use it differently and don't share any implementation which involves this member (unlike totalHits and hq). I was in the middle of adding javadoc to the protected members and constructor (which accepts numHits and PQ) when I noticed that numHits is completely ignored by this constructor --> TopDocCollector(int numHits, PriortityQueue hq). The reason is that PQ is be probably configured to hold a maximum number of hits. What bothers me with this constructor is that it may falsely lead users of the API to think that it limits their PQ with a maximum number of hits. I think we should remove that parameter and expose two constructors: public TopDocCollector(int numHits) AND public TopDocCollector(PriorityQueue hq). If you agree, I'll reflect that in the 2nd patch I want to create (which adds javadoc).
        Hide
        doronc Doron Cohen added a comment -

        You're right, this is confusing indeed.
        Although it is not public or protected there may users code
        (residing in same package) relying on this method so it can't
        just be removed but rather just deprecated.

        If you agree, I'll reflect that in the 2nd patch I want to create (which adds javadoc).

        Yes thanks!

        Show
        doronc Doron Cohen added a comment - You're right, this is confusing indeed. Although it is not public or protected there may users code (residing in same package) relying on this method so it can't just be removed but rather just deprecated. If you agree, I'll reflect that in the 2nd patch I want to create (which adds javadoc). Yes thanks!
        Hide
        shaie Shai Erera added a comment -

        Marked the constructor as deprecated, created another one (protected) which accepts PQ only and modified TopFieldDocCollector to use the new c'tor instead of the deprecated one.
        Also added javadoc.

        Show
        shaie Shai Erera added a comment - Marked the constructor as deprecated, created another one (protected) which accepts PQ only and modified TopFieldDocCollector to use the new c'tor instead of the deprecated one. Also added javadoc.
        Hide
        mikemccand Michael McCandless added a comment -

        Doron is this one ready to go in?

        Show
        mikemccand Michael McCandless added a comment - Doron is this one ready to go in?
        Hide
        doronc Doron Cohen added a comment -

        It is, applies cleanly and seems correct.
        Will commit as soon as tests complete.

        Show
        doronc Doron Cohen added a comment - It is, applies cleanly and seems correct. Will commit as soon as tests complete.
        Hide
        doronc Doron Cohen added a comment -

        Thanks Shai !

        Show
        doronc Doron Cohen added a comment - Thanks Shai !

          People

          • Assignee:
            doronc Doron Cohen
            Reporter:
            shaie Shai Erera
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development