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.

      1. SOLR-9783.patch
        3 kB
        Christine Poerschke

        Issue Links

          Activity

          Hide
          dsmiley David Smiley added a comment -

          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

          No I don't recall if it could be null. I recall I was trying to be a bit cautious in my changes to Solr as part of that issue... it's possible I felt I needed to add that because the code prior had null checks; I don't remember. If you don't think it can be null, then I suggest removing them and add asserts.

          Show
          dsmiley David Smiley added a comment - 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 No I don't recall if it could be null. I recall I was trying to be a bit cautious in my changes to Solr as part of that issue... it's possible I felt I needed to add that because the code prior had null checks; I don't remember. If you don't think it can be null, then I suggest removing them and add asserts.
          Hide
          jira-bot ASF subversion and git services added a comment -

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

          SOLR-9783: (Search|Top)Group[s]ShardResponseProcessor.process: turned sortWithinGroup null check into assert.
          Also sort.equals tweak in (grouping) QueryCommand.create method.

          Show
          jira-bot ASF subversion and git services added a comment - Commit 02c687758e904ab92c2b766b2ec837bcb99f484f in lucene-solr's branch refs/heads/master from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=02c6877 ] SOLR-9783 : (Search|Top)Group [s] ShardResponseProcessor.process: turned sortWithinGroup null check into assert. Also sort.equals tweak in (grouping) QueryCommand.create method.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 0ef423674541a6e3c620a8b0029391ee7aca63be 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=0ef4236 ]

          SOLR-9783: (Search|Top)Group[s]ShardResponseProcessor.process: turned sortWithinGroup null check into assert.
          Also sort.equals tweak in (grouping) QueryCommand.create method.

          Show
          jira-bot ASF subversion and git services added a comment - Commit 0ef423674541a6e3c620a8b0029391ee7aca63be 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=0ef4236 ] SOLR-9783 : (Search|Top)Group [s] ShardResponseProcessor.process: turned sortWithinGroup null check into assert. Also sort.equals tweak in (grouping) QueryCommand.create method.
          Hide
          Judith Judith Silverman added a comment -

          Hello, is there any context in which we don't want to interpret a null Sort value in a SortSpec as Sort.RELEVANCE? I ask because I just had a test fail because sortWithinGroup was null. (My code is highly experimental, so this failure doesn't prove anything about the master.) My investigation led me to SortSpecParsing's newEmptySortSpec(), which feeds a null Sort value to a SortSpec constructor. I'm wondering whether newEmptySortSpec() could send Sort.RELEVANCE instead and/or the SortSpec constructors could be modified to replace nulls with Sort.RELEVANCE (and to adjust whatever else would need adjusting). Apologies if this idea is impractical or has already been considered and dismissed.

          Show
          Judith Judith Silverman added a comment - Hello, is there any context in which we don't want to interpret a null Sort value in a SortSpec as Sort.RELEVANCE? I ask because I just had a test fail because sortWithinGroup was null. (My code is highly experimental, so this failure doesn't prove anything about the master.) My investigation led me to SortSpecParsing's newEmptySortSpec(), which feeds a null Sort value to a SortSpec constructor. I'm wondering whether newEmptySortSpec() could send Sort.RELEVANCE instead and/or the SortSpec constructors could be modified to replace nulls with Sort.RELEVANCE (and to adjust whatever else would need adjusting). Apologies if this idea is impractical or has already been considered and dismissed.
          Hide
          cpoerschke Christine Poerschke added a comment -

          ... to interpret a null Sort value in a SortSpec as ...

          Yes, that's a good question as to if/how/why/when the Sort value in a SortSpec might be null and if it is null, how to interpret that.

          Within this change/ticket here I was just specifically looking at whether or not the sortWithinGroup within a GroupingSpecification could ever be null at the point at which the ...ShardResponseProcessor is using it. From my reading of the code I concluded that it would never be null because of the QueryComponent.java#L249-L269 logic.

          ... I'm wondering whether newEmptySortSpec() could send Sort.RELEVANCE instead and/or the SortSpec constructors could be modified to replace nulls with Sort.RELEVANCE ...

          newEmptySortSpec() sending Sort.RELEVANCE instead of null sounds an interesting idea. I wonder where else apart from QueryComponent.java#L249-L269 there is "null means Sort.RELEVANCE" logic (or indeed "null means SomethingElse" logic) that would go away if newEmptySortSpec() stopped sending null.

          Show
          cpoerschke Christine Poerschke added a comment - ... to interpret a null Sort value in a SortSpec as ... Yes, that's a good question as to if/how/why/when the Sort value in a SortSpec might be null and if it is null, how to interpret that. Within this change/ticket here I was just specifically looking at whether or not the sortWithinGroup within a GroupingSpecification could ever be null at the point at which the ...ShardResponseProcessor is using it. From my reading of the code I concluded that it would never be null because of the QueryComponent.java#L249-L269 logic. ... I'm wondering whether newEmptySortSpec() could send Sort.RELEVANCE instead and/or the SortSpec constructors could be modified to replace nulls with Sort.RELEVANCE ... newEmptySortSpec() sending Sort.RELEVANCE instead of null sounds an interesting idea. I wonder where else apart from QueryComponent.java#L249-L269 there is "null means Sort.RELEVANCE" logic (or indeed "null means SomethingElse" logic) that would go away if newEmptySortSpec() stopped sending null.
          Hide
          cpoerschke Christine Poerschke added a comment -

          Thanks David for the suggestion to use asserts - that being an alternative to removal of the check had not occurred to me.

          Show
          cpoerschke Christine Poerschke added a comment - Thanks David for the suggestion to use asserts - that being an alternative to removal of the check had not occurred to me.
          Hide
          cpoerschke Christine Poerschke added a comment -

          "remove no-longer-needed sortWithinGroup null checks in ..." here is done, SOLR-9660 and SOLR-6203 continue.

          Show
          cpoerschke Christine Poerschke added a comment - "remove no-longer-needed sortWithinGroup null checks in ..." here is done, SOLR-9660 and SOLR-6203 continue.

            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