Lucene - Core
  1. Lucene - Core
  2. LUCENE-3483

Move Function grouping collectors from Solr to grouping module

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 4.0-ALPHA
    • Fix Version/s: 4.0-ALPHA
    • Component/s: modules/grouping
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Move the Function*Collectors from Solr (inside Grouping source file) to grouping module.

      1. LUCENE-3483.patch
        70 kB
        Martijn van Groningen
      2. LUCENE-3483.patch
        57 kB
        Martijn van Groningen
      3. LUCENE-3483.patch
        53 kB
        Martijn van Groningen

        Activity

        Hide
        Martijn van Groningen added a comment - - edited

        Attached patch.

        • I've put the collectors under o.a.s.grouping.function package.
        • The grouping module depends on queries module in this patch.
        • The tests inside grouping module randomly select a term based implementation or a ValueSource based implementation (Function*Collector implementation).
        Show
        Martijn van Groningen added a comment - - edited Attached patch. I've put the collectors under o.a.s.grouping.function package. The grouping module depends on queries module in this patch. The tests inside grouping module randomly select a term based implementation or a ValueSource based implementation (Function*Collector implementation).
        Hide
        Martijn van Groningen added a comment -

        Updated patch.

        • Added some more documentation
        • Added missing class: BytesRefFieldSource This class is now only used in the grouping test, but I think is usable outside this test case as well.
        Show
        Martijn van Groningen added a comment - Updated patch. Added some more documentation Added missing class: BytesRefFieldSource This class is now only used in the grouping test, but I think is usable outside this test case as well.
        Hide
        Martijn van Groningen added a comment - - edited

        Updated patch.

        • Updated to latest trunk changes.
        • Moved Term* collectors to term package.
        • Made SentinelIntSet public and added @lucene.internal to jdoc

        I think it is time to commit this patch.

        Show
        Martijn van Groningen added a comment - - edited Updated patch. Updated to latest trunk changes. Moved Term* collectors to term package. Made SentinelIntSet public and added @lucene.internal to jdoc I think it is time to commit this patch.
        Hide
        Yonik Seeley added a comment -

        Unfortunately, context diffs are hard to review in these cases (when things move as opposed to just change).
        Are the classes just being moved (with trivial changes such as package-private -> public, etc) or are there any additional changes/cleanups?

        Show
        Yonik Seeley added a comment - Unfortunately, context diffs are hard to review in these cases (when things move as opposed to just change). Are the classes just being moved (with trivial changes such as package-private -> public, etc) or are there any additional changes/cleanups?
        Hide
        Martijn van Groningen added a comment - - edited

        I only moved the Term* collectors. As a result of that I needed to make two explicit changes:

        • Changed visibility of SentinelIntSet from package protected to public.
        • Minor documentation change in package.html

        I could have moved SentinelIntSet to term package instead of changing visibility, but now other users can use this class as well now. Maybe one day SentinelIntSet can move to core in a util package.

        Show
        Martijn van Groningen added a comment - - edited I only moved the Term* collectors. As a result of that I needed to make two explicit changes: Changed visibility of SentinelIntSet from package protected to public. Minor documentation change in package.html I could have moved SentinelIntSet to term package instead of changing visibility, but now other users can use this class as well now. Maybe one day SentinelIntSet can move to core in a util package.
        Hide
        Yonik Seeley added a comment -

        Cool, thanks for the recap.

        Show
        Yonik Seeley added a comment - Cool, thanks for the recap.
        Hide
        Martijn van Groningen added a comment -

        Committed in r1179808

        Show
        Martijn van Groningen added a comment - Committed in r1179808

          People

          • Assignee:
            Martijn van Groningen
            Reporter:
            Martijn van Groningen
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development