-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch) #300
Open
mjosephidou
wants to merge
23
commits into
apache:master
Choose a base branch
from
bloomberg:SOLR-11831
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
80b3092
SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Ve…
c2f6303
miscellaneous tweaks and suggestions
cpoerschke 23f6386
initial (partial) Solr Ref Guide content for group.skip.second.step
cpoerschke a7e9151
class-level javadocs for SkipSecondStepSearchResultResultTransformer
cpoerschke 281def0
method-level javadocs for [SkipSecondStep]SearchGroupsContainer
cpoerschke 905325e
make SkipSecondStepSearchResultResultTransformer not-an-inner-class
cpoerschke 95236f8
Remove inhearitDoc (can't be used in a class javadoc)
diegoceccarelli 609b6e5
Add group.skip.second.step documentation
diegoceccarelli a3fb5ca
Use diamond operator
diegoceccarelli 1aed3d6
Test group.skip.second.step with 0 results
diegoceccarelli 6d21a93
Remove call to super.addSearchGroupToShards (not needed)
diegoceccarelli 98f4ce7
solr-ref-guide: minor edits in the group.skip.second.step section
cpoerschke 04c97eb
(maybe) disallow group.ngroups=true when group.skip.second.step=true
cpoerschke 4e40419
TestDistributedGrouping: doTestGroupSkipSecondStepAlt sketch, doTestG…
cpoerschke c706d79
In progress
diegoceccarelli bac35c9
Add more tests for distribute grouping + minor fixes
diegoceccarelli c4df27a
Manage NaN
diegoceccarelli 8cecc96
Fix maxScore response
diegoceccarelli 60567ff
Fix topDocScore, used to compute maxScore
diegoceccarelli c8c0fe2
Add test checks on numFound
diegoceccarelli b5d2b8c
Add documentation
diegoceccarelli 548a04b
Check for group.func
diegoceccarelli 27c4355
Forbid group.query and group.func
diegoceccarelli File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,9 +48,25 @@ public class SearchGroup<T> { | |
* been passed to {@link FirstPassGroupingCollector#getTopGroups} */ | ||
public Object[] sortValues; | ||
|
||
/** The top doc of this group: we track the Lucene id, | ||
* the Solr id and the score of the document */ | ||
public Object topDocSolrId; | ||
public float topDocScore; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering about additional object/GC footprint (for anyone who does not benefit from that) associated with the addition of three members here and opened https://issues.apache.org/jira/browse/LUCENE-8728 to explore further around that. |
||
/** The topDocLuceneId will be null at the federator level because it is unique only at the shard level. | ||
* It is used by the shard to get the corresponding solr id when serializing the search group to send to the federator | ||
*/ | ||
public int topDocLuceneId; | ||
|
||
@Override | ||
public String toString() { | ||
return("SearchGroup(groupValue=" + groupValue + " sortValues=" + Arrays.toString(sortValues) + ")"); | ||
return "SearchGroup{" + | ||
"groupValue=" + groupValue + | ||
", sortValues=" + Arrays.toString(sortValues) + | ||
", topDocSolrId=" + topDocSolrId + | ||
", topDocScore=" + topDocScore + | ||
", topDocLuceneId=" + topDocLuceneId + | ||
'}'; | ||
} | ||
|
||
@Override | ||
|
@@ -113,6 +129,11 @@ private static class MergedGroup<T> { | |
public boolean processed; | ||
public boolean inQueue; | ||
|
||
/** The top doc of this group: | ||
* the Solr id and the score of the document */ | ||
public float topDocScore; | ||
public Object topDocSolrId; | ||
|
||
public MergedGroup(T groupValue) { | ||
this.groupValue = groupValue; | ||
} | ||
|
@@ -225,6 +246,8 @@ private void updateNextGroup(int topN, ShardIter<T> shard) { | |
// Start a new group: | ||
//System.out.println(" new"); | ||
mergedGroup = new MergedGroup<>(group.groupValue); | ||
mergedGroup.topDocSolrId = group.topDocSolrId; | ||
mergedGroup.topDocScore = group.topDocScore; | ||
mergedGroup.minShardIndex = shard.shardIndex; | ||
assert group.sortValues != null; | ||
mergedGroup.topValues = group.sortValues; | ||
|
@@ -262,6 +285,8 @@ private void updateNextGroup(int topN, ShardIter<T> shard) { | |
if (mergedGroup.inQueue) { | ||
queue.remove(mergedGroup); | ||
} | ||
mergedGroup.topDocScore = group.topDocScore; | ||
mergedGroup.topDocSolrId = group.topDocSolrId; | ||
mergedGroup.topValues = group.sortValues; | ||
mergedGroup.minShardIndex = shard.shardIndex; | ||
queue.add(mergedGroup); | ||
|
@@ -308,6 +333,8 @@ public Collection<SearchGroup<T>> merge(List<Collection<SearchGroup<T>>> shards, | |
final SearchGroup<T> newGroup = new SearchGroup<>(); | ||
newGroup.groupValue = group.groupValue; | ||
newGroup.sortValues = group.topValues; | ||
newGroup.topDocSolrId = group.topDocSolrId; | ||
newGroup.topDocScore = group.topDocScore; | ||
newTopGroups.add(newGroup); | ||
if (newTopGroups.size() == topN) { | ||
break; | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
searchGroup.topDocScore
being filled in conditionally here i.e. only if it's a field comparator 'jumps out' a little.Have not yet looked into what other comparators there are and if removing of the conditional might be practical.
But this also re-surfaced the
SearchGroup.java
question around adding or not adding additional fields and what (if any) alternative might be possible.With this in mind https://github.com/cpoerschke/lucene-solr/commits/github-bloomberg-SOLR-11831-cpoerschke-13 explores the 'extend FirstPassGroupingCollector and SearchGroup' route a little further:
group.skip.second.step
code paths always have SolrSearchGroup instead of SearchGroupWhat do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking the conversation back from the commits on the above branch to here: bloomberg#232 sketches extending Lucene's FirstPassGroupingCollector and SearchGroup for Solr's group.skip.second.step use as outlined above.
Flagged up by that sketching, and not yet fully explored, is the question of whether or not the exclusion of group.func is actually needed, superficially it seemed it might not be i.e. the federator is just merging groups and sorting by id(s) and where the ids came from might not matter.