Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-9783

remove no-longer-needed sortWithinGroup null checks in (Search|Top)Group[s]ShardResponseProcessor

    Details

    • Type: Task
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.4, 7.0
    • Component/s: None
    • Security Level: Public (Default Security Level. Issues are Public)
    • Labels:
      None

      Description

      Why this, why now? I was looking some more at SOLR-6203 and what the next sub-step after the SOLR-9660 sub-step might be. Revisiting Judith Silverman's SOLR-6203 README file, the step (1) is included in SOLR-9660 and step (2) mentions passing around SortSpecs rather than plain Sorts, with Search and TopGroups ShardResponseProcessor amongst the files affected. In principle the change for those two files should be straightforward i.e.

        ...
      - Sort sortWithinGroup = rb.getGroupingSpec().getSortWithinGroup();
      + SortSpec sortSpecWithinGroup = rb.getGroupingSpec().getSortSpecWithinGroup();
        ...
      

      except that both starting points are

        Sort sortWithinGroup = rb.getGroupingSpec().getSortWithinGroup();
        if (sortWithinGroup == null) { // TODO prevent it from being null in the first place
          sortWithinGroup = Sort.RELEVANCE;
        }
      

      and so this ticket here aims to get rid of the two 'TODO' statements. The statements were added as part of LUCENE-6900's https://svn.apache.org/viewvc?view=revision&revision=1716569 in November 2015 and Judith's original SOLR-6203.patch is from October 2015 i.e. slightly before then.

      David Smiley - do you recall anything re: when/how sortWithinGroup could be null back then? From my reading of the current (master) code the sortWithinGroup would never be null now. solr/core tests pass when the if statements are removed (will attach patch and also run the non-core solr tests) but that could of course just be due to lacking test coverage.

      And unrelated but noticed whilst in the code area, the patch includes a

      + ... || sort == Sort.RELEVANCE) {
      - ... || sort.equals(Sort.RELEVANCE)) {
      

      tweak to QueryCommand.java also.

        Attachments

        1. SOLR-9783.patch
          3 kB
          Christine Poerschke

          Issue Links

            Activity

              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: