Lucene - Core
  1. Lucene - Core
  2. LUCENE-2057

TopDocsCollector should have bounded generic <T extends ScoreDoc>

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.0
    • Component/s: core/search
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      TopDocsCollector was changed to be TopDocsCollector<T>. However it has methods which specifically assume the PQ stores ScoreDoc. Therefore, if someone extends it and defines a type which is not ScoreDoc, things will break.

      We shouldn't put <T> on TopDocsCollector at all, but rather change its ctor to protected TopDocsCollector(PriorityQueue<? extends ScoreDoc> pq). TopDocsCollector should handle ScoreDoc types. If we do this, we'll need to change FieldValueHitQueue's Entry to extend ScoreDoc as well.

      1. scoredoc.patch
        6 kB
        Uwe Schindler

        Activity

        Hide
        Uwe Schindler added a comment -

        Committed revision: 835090

        Thanks Shai! As this is not a new bug, I added you to the changes entry for the Java 5 generics additions.

        Show
        Uwe Schindler added a comment - Committed revision: 835090 Thanks Shai! As this is not a new bug, I added you to the changes entry for the Java 5 generics additions.
        Hide
        Uwe Schindler added a comment -

        It's strange ... I'd thought that if I declare PQ<T>, and then from TFC for example init FVHQ, which extends PQ as <Entry>, then all who'll use FVHQ won't need to cast to Entry. I've tried it and indeed TSDC does not need to case to ScoreDoc, but TFC needs to case to Entry.

        Because of that it is implemented with a generic type param. This ensures that both the class implementation and the embedded PQ both work with the same data type and you never have to cast. The problem with the state before was that Entry did not extend ScoreDoc and because of that the generics were unbounded.

        I'll commit this soon (waiting for the backwards-tests).

        Show
        Uwe Schindler added a comment - It's strange ... I'd thought that if I declare PQ<T>, and then from TFC for example init FVHQ, which extends PQ as <Entry>, then all who'll use FVHQ won't need to cast to Entry. I've tried it and indeed TSDC does not need to case to ScoreDoc, but TFC needs to case to Entry. Because of that it is implemented with a generic type param. This ensures that both the class implementation and the embedded PQ both work with the same data type and you never have to cast. The problem with the state before was that Entry did not extend ScoreDoc and because of that the generics were unbounded. I'll commit this soon (waiting for the backwards-tests).
        Hide
        Shai Erera added a comment -

        It's strange ... I'd thought that if I declare PQ<T>, and then from TFC for example init FVHQ, which extends PQ as <Entry>, then all who'll use FVHQ won't need to cast to Entry. I've tried it and indeed TSDC does not need to case to ScoreDoc, but TFC needs to case to Entry.

        Patch looks good Uwe !

        Show
        Shai Erera added a comment - It's strange ... I'd thought that if I declare PQ<T>, and then from TFC for example init FVHQ, which extends PQ as <Entry>, then all who'll use FVHQ won't need to cast to Entry. I've tried it and indeed TSDC does not need to case to ScoreDoc, but TFC needs to case to Entry. Patch looks good Uwe !
        Hide
        Uwe Schindler added a comment -

        The generics should in all cases put on top of the whole class, because it is a wrapper around PQ. And PQ needs a generic type. By using a ?-like type you have to still cast in all sub-classes. See also other classes in oal.search (MultTermQueryWrapperFilter and so on).

        Show
        Uwe Schindler added a comment - The generics should in all cases put on top of the whole class, because it is a wrapper around PQ. And PQ needs a generic type. By using a ?-like type you have to still cast in all sub-classes. See also other classes in oal.search (MultTermQueryWrapperFilter and so on).
        Hide
        Uwe Schindler added a comment -

        The patch is already finished and ready to commit.

        Show
        Uwe Schindler added a comment - The patch is already finished and ready to commit.
        Hide
        Shai Erera added a comment -

        Uwe, let me know if you'd like me to work on a patch.

        Show
        Shai Erera added a comment - Uwe, let me know if you'd like me to work on a patch.

          People

          • Assignee:
            Uwe Schindler
            Reporter:
            Shai Erera
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development