[
https://issues.apache.org/jira/browse/SOLR-9660?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15609712#comment-15609712
]
Christine Poerschke commented on SOLR-9660:
-------------------------------------------
bq. ... 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|https://github.com/apache/lucene-solr/blob/master/solr/core/src/java/org/apache/solr/handler/component/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.
bq. ... rewrite the old ones in terms of the new ones ...
{code}
public SortSpec(Sort sort, List<SchemaField> fields)
{
this(sort, fields, num, offset);
}
{code}
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?
bq. ... the new weightSortSpec() function could be defined in terms of a
4-parameter version ...
Interesting idea. So if the 4-parameter version is
{code}
- public SortSpec weightSortSpec(SortSpec originalSortSpec, Sort nullEquivalent)
+ public SortSpec weightSortSpec(SortSpec originalSortSpec, Sort
nullEquivalent, int count, int offset)
{code}
then the implementation needs to decide between
{{originalSortSpec.getOffset()}} and {{offset}} alternatives, but
{code}
- public SortSpec weightSortSpec(SortSpec originalSortSpec, Sort nullEquivalent)
+ public SortSpec weightSortSpec(Sort originalSort, Sort nullEquivalent, int
count, int offset)
{code}
4-parameter variant would work well for {{sortSpecWithinGroup}} since the
{{sortSpecWithinGroup.set(Offset|Count)}} calls would disappear. However the
downside would be
{code}
- final SortSpec groupSortSpec = searcher.weightSortSpec(sortSpec,
Sort.RELEVANCE);
+ final SortSpec groupSortSpec = searcher.weightSortSpec(sortSpec.getSort(),
Sort.RELEVANCE, sortSpec.getCount(), sortSpec.getOffset());
{code}
and so the 2-parameter version seems neater.
bq. ... 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.
> in GroupingSpecification factor [group](sort|offset|limit) into
> [group](sortSpec)
> ---------------------------------------------------------------------------------
>
> Key: SOLR-9660
> URL: https://issues.apache.org/jira/browse/SOLR-9660
> Project: Solr
> Issue Type: Task
> Security Level: Public(Default Security Level. Issues are Public)
> Reporter: Christine Poerschke
> Assignee: Christine Poerschke
> Priority: Minor
> Attachments: SOLR-9660.patch, SOLR-9660.patch
>
>
> This is split out and adapted from and towards the SOLR-6203 changes.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
