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

in GroupingSpecification factor [group](sort|offset|limit) into [group](sortSpec)

    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

      This is split out and adapted from and towards the SOLR-6203 changes.

      1. SOLR-9660.patch
        16 kB
        Christine Poerschke
      2. SOLR-9660.patch
        11 kB
        Christine Poerschke
      3. SOLR-9660.patch
        11 kB
        Christine Poerschke
      4. SOLR-9660.patch
        11 kB
        Christine Poerschke

        Issue Links

          Activity

          Hide
          cpoerschke Christine Poerschke added a comment -

          Attaching work-in-progress patch, it inexplicably fails one of the TestDistributedGrouping tests (details below) with visually the difference apparently only in the 'start' element of the response but not the response elements themselves. SOLR-9649 was also unexpected and could be related.

          > Throwable #1: junit.framework.AssertionFailedError: .grouped[a_i1].doclist.start:5!=0
          >        at __randomizedtesting.SeedInfo.seed([E195797B46E2FF35:69C146A1E81E92CD]:0)
          >        at junit.framework.Assert.fail(Assert.java:50)
          >        at org.apache.solr.BaseDistributedSearchTestCase.compareSolrResponses(BaseDistributedSearchTestCase.java:913)
          >        at org.apache.solr.BaseDistributedSearchTestCase.compareResponses(BaseDistributedSearchTestCase.java:932)
          >        at org.apache.solr.BaseDistributedSearchTestCase.query(BaseDistributedSearchTestCase.java:607)
          >        at org.apache.solr.BaseDistributedSearchTestCase.query(BaseDistributedSearchTestCase.java:587)
          >        at org.apache.solr.BaseDistributedSearchTestCase.query(BaseDistributedSearchTestCase.java:566)
          >        at org.apache.solr.TestDistributedGrouping.test(TestDistributedGrouping.java:170)
          >        at org.apache.solr.BaseDistributedSearchTestCase$ShardsRepeatRule$ShardsRepeatStatement.callStatement(BaseDistributedSearchTestCase.java:1011)
          >        at org.apache.solr.BaseDistributedSearchTestCase$ShardsRepeatRule$ShardsStatement.evaluate(BaseDistributedSearchTestCase.java:960)
          >        at java.lang.Thread.run(Thread.java:745)
          
          Show
          cpoerschke Christine Poerschke added a comment - Attaching work-in-progress patch, it inexplicably fails one of the TestDistributedGrouping tests (details below) with visually the difference apparently only in the 'start' element of the response but not the response elements themselves. SOLR-9649 was also unexpected and could be related. > Throwable #1: junit.framework.AssertionFailedError: .grouped[a_i1].doclist.start:5!=0 > at __randomizedtesting.SeedInfo.seed([E195797B46E2FF35:69C146A1E81E92CD]:0) > at junit.framework.Assert.fail(Assert.java:50) > at org.apache.solr.BaseDistributedSearchTestCase.compareSolrResponses(BaseDistributedSearchTestCase.java:913) > at org.apache.solr.BaseDistributedSearchTestCase.compareResponses(BaseDistributedSearchTestCase.java:932) > at org.apache.solr.BaseDistributedSearchTestCase.query(BaseDistributedSearchTestCase.java:607) > at org.apache.solr.BaseDistributedSearchTestCase.query(BaseDistributedSearchTestCase.java:587) > at org.apache.solr.BaseDistributedSearchTestCase.query(BaseDistributedSearchTestCase.java:566) > at org.apache.solr.TestDistributedGrouping.test(TestDistributedGrouping.java:170) > at org.apache.solr.BaseDistributedSearchTestCase$ShardsRepeatRule$ShardsRepeatStatement.callStatement(BaseDistributedSearchTestCase.java:1011) > at org.apache.solr.BaseDistributedSearchTestCase$ShardsRepeatRule$ShardsStatement.evaluate(BaseDistributedSearchTestCase.java:960) > at java.lang. Thread .run( Thread .java:745)
          Hide
          Judith Judith Silverman added a comment -

          Christine, I have no suggestions about the failing test but here are a couple of questions. Is it necessary to deprecate the GroupingSpecification accessors like getGroupOffset(), rather than simply modifying the definitions as you did? They could still be useful as wrappers. Not that I feel strongly about it, but your answer could tell me something about Solr philosophy.

          On a related note: now that you have added new SortSpec constructors, could you rewrite the old ones in terms of the new ones using the initial values of num and offset as the last two arguments:

          public SortSpec(Sort sort, List<SchemaField> fields)

          { this(sort, fields, num, offset ); }

          ? I missed that in my SOLR-6203 patch but it jumped out at me now. Similarly, the new weightSortSpec() function could be defined in terms of a 4-parameter version: weightSortSpec(SortSpec originalSortSpec, Sort nullEquivalent, int count, int offset) to make it more self-contained.

          Thanks,
          Judith

          Show
          Judith Judith Silverman added a comment - Christine, I have no suggestions about the failing test but here are a couple of questions. Is it necessary to deprecate the GroupingSpecification accessors like getGroupOffset(), rather than simply modifying the definitions as you did? They could still be useful as wrappers. Not that I feel strongly about it, but your answer could tell me something about Solr philosophy. On a related note: now that you have added new SortSpec constructors, could you rewrite the old ones in terms of the new ones using the initial values of num and offset as the last two arguments: public SortSpec(Sort sort, List<SchemaField> fields) { this(sort, fields, num, offset ); } ? I missed that in my SOLR-6203 patch but it jumped out at me now. Similarly, the new weightSortSpec() function could be defined in terms of a 4-parameter version: weightSortSpec(SortSpec originalSortSpec, Sort nullEquivalent, int count, int offset) to make it more self-contained. Thanks, Judith
          Hide
          cpoerschke Christine Poerschke added a comment -

          Test failure mystery solved: in the original patch there is a

            sortSpecWithinGroup = groupSortSpec;
          

          which needed to be

            sortSpecWithinGroup = new SortSpec(
              groupSortSpec.getSort(),
              groupSortSpec.getSchemaFields(),
              groupSortSpec.getCount(),
              groupSortSpec.getOffset());
          

          so that subsequent

            sortSpecWithinGroup.set(Count|Offset) = ...
          

          change sortSpecWithinGroup only and not groupSortSpec also.

          Show
          cpoerschke Christine Poerschke added a comment - Test failure mystery solved: in the original patch there is a sortSpecWithinGroup = groupSortSpec; which needed to be sortSpecWithinGroup = new SortSpec( groupSortSpec.getSort(), groupSortSpec.getSchemaFields(), groupSortSpec.getCount(), groupSortSpec.getOffset()); so that subsequent sortSpecWithinGroup.set(Count|Offset) = ... change sortSpecWithinGroup only and not groupSortSpec also.
          Hide
          cpoerschke Christine Poerschke added a comment -

          Attaching revised patch, yet to check if ant precommit and cd solr/core ; ant test still pass with it.

          (Observation: The rewritten...Fields logic in SolrIndexSearcher.weightSortSpec seems a bit awkward, though unless we go and assume that SortField[] from the original and from the rewritten sort are identical i cannot see a away around that logic.)

          Show
          cpoerschke Christine Poerschke added a comment - Attaching revised patch, yet to check if ant precommit and cd solr/core ; ant test still pass with it. (Observation: The rewritten...Fields logic in SolrIndexSearcher.weightSortSpec seems a bit awkward, though unless we go and assume that SortField[] from the original and from the rewritten sort are identical i cannot see a away around that logic.)
          Hide
          cpoerschke Christine Poerschke added a comment -

          Hi Judith, good couple of questions and suggestions, thanks for asking them. I would reply in more detail later on, got a couple of other things and stuff to take care of this morning/today and want to give proper time and consideration to the reply rather than just quickly rush it.

          Show
          cpoerschke Christine Poerschke added a comment - Hi Judith, good couple of questions and suggestions, thanks for asking them. I would reply in more detail later on, got a couple of other things and stuff to take care of this morning/today and want to give proper time and consideration to the reply rather than just quickly rush it.
          Hide
          Judith Judith Silverman added a comment -

          Ah, glad you found that bug.

          While I was looking for it, I noticed that in at least three different files (Grouping.java, TopGroupsShardRequestFactory.java, TopGroupsShardResponseProcessor.java), we set something related to groupOffset to 0 if group.format == simple or groupingSpec.isMain(). I certainly don't see the whole picture, but other things being equal I would think it cleaner and safer to do that check as far upstream and in as few places as possible.

          Is there any advantage to having so many '5's in the unit tests, by the way? Seems to me that two of them could get swapped and the test rig would never know.

          Thanks,
          Judith

          Show
          Judith Judith Silverman added a comment - Ah, glad you found that bug. While I was looking for it, I noticed that in at least three different files (Grouping.java, TopGroupsShardRequestFactory.java, TopGroupsShardResponseProcessor.java), we set something related to groupOffset to 0 if group.format == simple or groupingSpec.isMain(). I certainly don't see the whole picture, but other things being equal I would think it cleaner and safer to do that check as far upstream and in as few places as possible. Is there any advantage to having so many '5's in the unit tests, by the way? Seems to me that two of them could get swapped and the test rig would never know. Thanks, Judith
          Hide
          cpoerschke Christine Poerschke added a comment -

          ... to deprecate the GroupingSpecification accessors like getGroupOffset() ...

          That is a good question. Okay, so there's actually three possibilities:

          1. Just remove the accessor and change all the accessor's callers to match.

          • GroupingSpecification has public visibility as do the accessors.
          • We can change all the accessor's callers in the Apache codebase but anyone out there who might have something custom calling the accessors concerned, their build will break when they upgrade.
          • Having said that, the GroupingSpecification is marked as @lucene.experimental and (as i understand that annotation) that would actually make it okay to remove the (considered experimental) accessor.
          • So my thinking behind not choosing this option #1 here is to reduce the scope of the changes i.e. changing all the accessor's callers increases the scope and complexity of the patch.

          2. Keep the accessors, only changing their implementation.

          • The advantage here is not having to change the accessor's callers.
          • A nice-but-not-required side effect is that non-Apache codebases' build will not break. Nice since strictly not required given the marked @lucene.experimental nature of the class.
          • A disadvantage is that present and future calling code now has two choices (getOffset() and getGroupSortSpec().getOffset()) to achieve the same thing.
          • A related aspect is that whereas getGroupSortSpec().getSort() and getGroupSortSpec().getCount() and getGroupSortSpec().getOffset() very apparently use the same underlying SortSpec that sameness is hidden in the implementation if the getGroupSort() and getLimit() and getOffset() wrappers are used instead. Making it very apparent that sort/count/offset are all part of SortSpec could then mean that the calling code passes one SortSpec object to whereever it needs to go whereas otherwise three distinct objects might be passed instead.

          3. Mark the accessors deprecated, change their implementation, plan to remove them later.

          • No need to change the accessor's callers now.
          • @Deprecated annotation reduces the two choices to one non-deprecated choice.
          • @Deprecated annotation helps when it is time to remove the accessor.
          • Actual removal of the accessor (separate patch, likely separate ticket):
            • Identify callers and change them, then remove the deprecated accessors.
            • Alternatively first remove the deprecated accessors and let the compiler tell you where the callers are
            • Along the way it might become apparent that further refactoring could be done e.g. from QueryComponent.java#L472-L481 it seems that Grouping.set(WithinGroupSort|DocsPerGroupDefault|GroupOffsetDefault) can be factored into just Grouping.setWithinGroupSortSpec() once SOLR-9660 here is complete.

          Oops, that was a rather long analysis of the options. In practice, option #3 was just my intuitive choice.

          ... rewrite the old ones in terms of the new ones ...

            public SortSpec(Sort sort, List<SchemaField> fields)
            {
              this(sort, fields, num, offset);
            }
          

          Yes, I normally also favour constructor's using the this(...) approach and didn't realise that the initial values of num and offset could be passed as args in this(...), thank you for sharing that. On balance my preference in this scenario would be to not rewrite the old constructors since the constructor initialising num and offset members based on the initial values of those very members seems unusual?

          ... the new weightSortSpec() function could be defined in terms of a 4-parameter version ...

          Interesting idea. So if the 4-parameter version is

          - public SortSpec weightSortSpec(SortSpec originalSortSpec, Sort nullEquivalent)
          + public SortSpec weightSortSpec(SortSpec originalSortSpec, Sort nullEquivalent, int count, int offset)
          

          then the implementation needs to decide between originalSortSpec.getOffset() and offset alternatives, but

          - public SortSpec weightSortSpec(SortSpec originalSortSpec, Sort nullEquivalent)
          + public SortSpec weightSortSpec(Sort originalSort, Sort nullEquivalent, int count, int offset)
          

          4-parameter variant would work well for sortSpecWithinGroup since the sortSpecWithinGroup.set(Offset|Count) calls would disappear. However the downside would be

          - final SortSpec groupSortSpec = searcher.weightSortSpec(sortSpec, Sort.RELEVANCE);
          + final SortSpec groupSortSpec = searcher.weightSortSpec(sortSpec.getSort(), Sort.RELEVANCE, sortSpec.getCount(), sortSpec.getOffset());
          

          and so the 2-parameter version seems neater.

          ... so many '5's in the unit tests ... two of them could get swapped and the test rig would never know.

          Indeed, fewer '5's would be preferable. I'm also a fan of randomization, in SOLR-9412 there's a link to an interesting talk on that from a while back.

          Show
          cpoerschke Christine Poerschke added a comment - ... to deprecate the GroupingSpecification accessors like getGroupOffset() ... That is a good question. Okay, so there's actually three possibilities: 1. Just remove the accessor and change all the accessor's callers to match. GroupingSpecification has public visibility as do the accessors. We can change all the accessor's callers in the Apache codebase but anyone out there who might have something custom calling the accessors concerned, their build will break when they upgrade. Having said that, the GroupingSpecification is marked as @lucene.experimental and (as i understand that annotation) that would actually make it okay to remove the (considered experimental) accessor. So my thinking behind not choosing this option #1 here is to reduce the scope of the changes i.e. changing all the accessor's callers increases the scope and complexity of the patch. 2. Keep the accessors, only changing their implementation. The advantage here is not having to change the accessor's callers. A nice-but-not-required side effect is that non-Apache codebases' build will not break. Nice since strictly not required given the marked @lucene.experimental nature of the class. A disadvantage is that present and future calling code now has two choices ( getOffset() and getGroupSortSpec().getOffset() ) to achieve the same thing. A related aspect is that whereas getGroupSortSpec().getSort() and getGroupSortSpec().getCount() and getGroupSortSpec().getOffset() very apparently use the same underlying SortSpec that sameness is hidden in the implementation if the getGroupSort() and getLimit() and getOffset() wrappers are used instead. Making it very apparent that sort/count/offset are all part of SortSpec could then mean that the calling code passes one SortSpec object to whereever it needs to go whereas otherwise three distinct objects might be passed instead. 3. Mark the accessors deprecated, change their implementation, plan to remove them later. No need to change the accessor's callers now. @Deprecated annotation reduces the two choices to one non-deprecated choice. @Deprecated annotation helps when it is time to remove the accessor. Actual removal of the accessor (separate patch, likely separate ticket): Identify callers and change them, then remove the deprecated accessors. Alternatively first remove the deprecated accessors and let the compiler tell you where the callers are Along the way it might become apparent that further refactoring could be done e.g. from QueryComponent.java#L472-L481 it seems that Grouping.set(WithinGroupSort|DocsPerGroupDefault|GroupOffsetDefault) can be factored into just Grouping.setWithinGroupSortSpec() once SOLR-9660 here is complete. Oops, that was a rather long analysis of the options. In practice, option #3 was just my intuitive choice. ... rewrite the old ones in terms of the new ones ... public SortSpec(Sort sort, List<SchemaField> fields) { this (sort, fields, num, offset); } Yes, I normally also favour constructor's using the this(...) approach and didn't realise that the initial values of num and offset could be passed as args in this(...) , thank you for sharing that. On balance my preference in this scenario would be to not rewrite the old constructors since the constructor initialising num and offset members based on the initial values of those very members seems unusual? ... the new weightSortSpec() function could be defined in terms of a 4-parameter version ... Interesting idea. So if the 4-parameter version is - public SortSpec weightSortSpec(SortSpec originalSortSpec, Sort nullEquivalent) + public SortSpec weightSortSpec(SortSpec originalSortSpec, Sort nullEquivalent, int count, int offset) then the implementation needs to decide between originalSortSpec.getOffset() and offset alternatives, but - public SortSpec weightSortSpec(SortSpec originalSortSpec, Sort nullEquivalent) + public SortSpec weightSortSpec(Sort originalSort, Sort nullEquivalent, int count, int offset) 4-parameter variant would work well for sortSpecWithinGroup since the sortSpecWithinGroup.set(Offset|Count) calls would disappear. However the downside would be - final SortSpec groupSortSpec = searcher.weightSortSpec(sortSpec, Sort.RELEVANCE); + final SortSpec groupSortSpec = searcher.weightSortSpec(sortSpec.getSort(), Sort.RELEVANCE, sortSpec.getCount(), sortSpec.getOffset()); and so the 2-parameter version seems neater. ... so many '5's in the unit tests ... two of them could get swapped and the test rig would never know. Indeed, fewer '5's would be preferable. I'm also a fan of randomization, in SOLR-9412 there's a link to an interesting talk on that from a while back.
          Hide
          Judith Judith Silverman added a comment -

          Christine, thanks for the detailed explanations.

          > On balance my preference in this scenario would be to not rewrite the old constructors since the constructor initialising num and offset members based on the initial values of those very members seems unusual?

          Indeed. We could instead define default values as consts and do this:

          public SortSpec(Sort sort, List<SchemaField> fields)

          { this(sort, fields, DEFAULT_NUM, DEFAULT_OFFSET); }

          Re a 4-parameter weightSortSpec(): I didn't mean to get rid of the 2-parameter version but to rewrite it in terms of a 4-parameter one:

          public SortSpec weightSortSpec(SortSpec originalSortSpec, Sort nullEquivalent)

          { return weightSortSpec( originalSortSpec, nullEquivalent, sortSpec.getCount(), sortSpec.getOffset() ); }

          Thanks for the link to SOLR-9412; will check it out.
          Judith

          Show
          Judith Judith Silverman added a comment - Christine, thanks for the detailed explanations. > On balance my preference in this scenario would be to not rewrite the old constructors since the constructor initialising num and offset members based on the initial values of those very members seems unusual? Indeed. We could instead define default values as consts and do this: public SortSpec(Sort sort, List<SchemaField> fields) { this(sort, fields, DEFAULT_NUM, DEFAULT_OFFSET); } Re a 4-parameter weightSortSpec(): I didn't mean to get rid of the 2-parameter version but to rewrite it in terms of a 4-parameter one: public SortSpec weightSortSpec(SortSpec originalSortSpec, Sort nullEquivalent) { return weightSortSpec( originalSortSpec, nullEquivalent, sortSpec.getCount(), sortSpec.getOffset() ); } Thanks for the link to SOLR-9412 ; will check it out. Judith
          Hide
          cpoerschke Christine Poerschke added a comment -

          ... this(sort, fields, DEFAULT_NUM, DEFAULT_OFFSET); ...

          Usually I am a big fan of static constants like DEFAULT_NUM. However, here the existing default is 10 which to me at least seems a bit unexpected (why 10 and not 5 or 20 say?) and so I'd rather not introduce a DEFAULT_NUM=10 which would make the 10 more 'baked in' than it already is. (As a side note, the default value I would have expected is 0 because that clearly leaves it up to the constructing code to set a meaningful value. In my mind SortSpec is a 'library' data structure class and only 'application' classes would contain DEFAULT_NUM constants, and different 'application' classes might even contain different defaults.)

          ... 2-parameter version ... rewrite it in terms of a 4-parameter one ...

          Good idea, I rebased and updated the patch to include a private implWeightSortSpec method and its logic is clearer since the three originalSortSpec.get... calls are pushed up into its caller.

          Show
          cpoerschke Christine Poerschke added a comment - ... this(sort, fields, DEFAULT_NUM, DEFAULT_OFFSET); ... Usually I am a big fan of static constants like DEFAULT_NUM . However, here the existing default is 10 which to me at least seems a bit unexpected (why 10 and not 5 or 20 say?) and so I'd rather not introduce a DEFAULT_NUM=10 which would make the 10 more 'baked in' than it already is. (As a side note, the default value I would have expected is 0 because that clearly leaves it up to the constructing code to set a meaningful value. In my mind SortSpec is a 'library' data structure class and only 'application' classes would contain DEFAULT_NUM constants, and different 'application' classes might even contain different defaults.) ... 2-parameter version ... rewrite it in terms of a 4-parameter one ... Good idea, I rebased and updated the patch to include a private implWeightSortSpec method and its logic is clearer since the three originalSortSpec.get... calls are pushed up into its caller.
          Hide
          Judith Judith Silverman added a comment -

          Hi, Christine. I finally have a chunk of time and would like to help out here, but I don't want to get in the way of your program. Is there something in particular you would like me to look into? BTW, not sure whether you saw my question in this thread about changes to groupOffset from 26Oct16; maybe it should be asked/addressed separately.
          Thanks,
          Judith

          Show
          Judith Judith Silverman added a comment - Hi, Christine. I finally have a chunk of time and would like to help out here, but I don't want to get in the way of your program. Is there something in particular you would like me to look into? BTW, not sure whether you saw my question in this thread about changes to groupOffset from 26Oct16; maybe it should be asked/addressed separately. Thanks, Judith
          Hide
          cpoerschke Christine Poerschke added a comment -

          ... we set something related to groupOffset to 0 if group.format == simple or groupingSpec.isMain() ...

          Hi Judith, sorry I missed this part of your comment at the time. I also don't quite see the whole picture w.r.t. how the group.format==simple is implemented. It seemed quite subtle though i.e. maybe it would be trickier to happen elsewhere and in fewer places. Drilling into the history of the code might help understand the implementation but I haven't gone looking and so can't say.

          Show
          cpoerschke Christine Poerschke added a comment - ... we set something related to groupOffset to 0 if group.format == simple or groupingSpec.isMain() ... Hi Judith, sorry I missed this part of your comment at the time. I also don't quite see the whole picture w.r.t. how the group.format==simple is implemented. It seemed quite subtle though i.e. maybe it would be trickier to happen elsewhere and in fewer places. Drilling into the history of the code might help understand the implementation but I haven't gone looking and so can't say.
          Hide
          cpoerschke Christine Poerschke added a comment -

          Attaching final patch, precommit and solr/core tests pass, commit to follow shortly.

          Compared to the previous patch this patch pushes the new accessors to the end of the file to make for an easier-to-read diff and also having yet again gotten myself confused by getGroupLimit (is it "within group limit" or "between groups limit"?) and its colleague getGroupOffset - the two methods have been renamed to getWithinGroup... to better reflect their meaning.

          Show
          cpoerschke Christine Poerschke added a comment - Attaching final patch, precommit and solr/core tests pass, commit to follow shortly. Compared to the previous patch this patch pushes the new accessors to the end of the file to make for an easier-to-read diff and also having yet again gotten myself confused by getGroupLimit (is it "within group limit" or "between groups limit"?) and its colleague getGroupOffset - the two methods have been renamed to getWithinGroup... to better reflect their meaning.
          Hide
          jira-bot ASF subversion and git services added a comment -

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

          SOLR-9660: in GroupingSpecification factor [group](sort|offset|limit) into [group](sortSpec) (Judith Silverman, Christine Poerschke)

          Show
          jira-bot ASF subversion and git services added a comment - Commit a7fa920b52febb80be70210caad7db1eeaf0f97a in lucene-solr's branch refs/heads/master from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=a7fa920 ] SOLR-9660 : in GroupingSpecification factor [group] (sort|offset|limit) into [group] (sortSpec) (Judith Silverman, Christine Poerschke)
          Hide
          cpoerschke Christine Poerschke added a comment -

          Hi Judith. Thanks for indirectly reminding me to return to this ticket here. I've committed to master branch and will backport to branch_6x tomorrow or so, SOLR-9783 is also committed, they both hopefully will help with SOLR-6203 itself. Replied to your 26Oct16 question above, also will add note to SOLR-6203 next.

          Show
          cpoerschke Christine Poerschke added a comment - Hi Judith. Thanks for indirectly reminding me to return to this ticket here. I've committed to master branch and will backport to branch_6x tomorrow or so, SOLR-9783 is also committed, they both hopefully will help with SOLR-6203 itself. Replied to your 26Oct16 question above, also will add note to SOLR-6203 next.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit cf8d0e1ccbb06edc8830b7c270b90984c1e287af 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=cf8d0e1 ]

          SOLR-9660: in GroupingSpecification factor [group](sort|offset|limit) into [group](sortSpec) (Judith Silverman, Christine Poerschke)

          Show
          jira-bot ASF subversion and git services added a comment - Commit cf8d0e1ccbb06edc8830b7c270b90984c1e287af 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=cf8d0e1 ] SOLR-9660 : in GroupingSpecification factor [group] (sort|offset|limit) into [group] (sortSpec) (Judith Silverman, Christine Poerschke)
          Hide
          jira-bot ASF subversion and git services added a comment -

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

          SOLR-9660: rename GroupSpecification's sortSpecWithinGroup to withinGroupSortSpec (Judith Silverman via Christine Poerschke)

          Show
          jira-bot ASF subversion and git services added a comment - Commit bc8936a567e40eef3b70665fd8838548350c9aaa in lucene-solr's branch refs/heads/master from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bc8936a ] SOLR-9660 : rename GroupSpecification's sortSpecWithinGroup to withinGroupSortSpec (Judith Silverman via Christine Poerschke)
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 038d4514a787d4137c59322dd38218eaa5be291a 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=038d451 ]

          SOLR-9660: rename GroupSpecification's sortSpecWithinGroup to withinGroupSortSpec (Judith Silverman via Christine Poerschke)

          Show
          jira-bot ASF subversion and git services added a comment - Commit 038d4514a787d4137c59322dd38218eaa5be291a 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=038d451 ] SOLR-9660 : rename GroupSpecification's sortSpecWithinGroup to withinGroupSortSpec (Judith Silverman via Christine Poerschke)
          Hide
          cpoerschke Christine Poerschke added a comment -

          Thanks Judith!

          Show
          cpoerschke Christine Poerschke added a comment - Thanks Judith!
          Hide
          Judith Judith Silverman added a comment -

          Hi, Christine, it's good to see all the updates! I haven't had time to
          do more than skim but I see that SOLR-9660 has been resolved with no
          new patches since 29Nov and I wanted to make sure that you saw my
          comment from 02Dec under SOLR-6203 about one of the patches for
          SOLR-9660. Maybe that's a non-issue or you are addressing it separately.
          Thanks,
          Judith

          Show
          Judith Judith Silverman added a comment - Hi, Christine, it's good to see all the updates! I haven't had time to do more than skim but I see that SOLR-9660 has been resolved with no new patches since 29Nov and I wanted to make sure that you saw my comment from 02Dec under SOLR-6203 about one of the patches for SOLR-9660 . Maybe that's a non-issue or you are addressing it separately. Thanks, Judith

            People

            • Assignee:
              cpoerschke Christine Poerschke
              Reporter:
              cpoerschke Christine Poerschke
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development