dsmiley commented on code in PR #2348:
URL: https://github.com/apache/solr/pull/2348#discussion_r1527010537
##########
solr/core/src/java/org/apache/solr/search/ReRankCollector.java:
##########
@@ -129,21 +129,25 @@ public TopDocs topDocs(int start, int howMany) {
ScoreDoc[] mainScoreDocs = mainDocs.scoreDocs;
ScoreDoc[] mainScoreDocsClone =
- (reRankScaler != null && reRankScaler.scaleScores())
- ? deepCloneAndZeroOut(mainScoreDocs)
- : null;
+ deepClone(mainScoreDocs, reRankScaler != null &&
reRankScaler.scaleScores());
Review Comment:
boolean args can be unclear at the call site. To mitigate that, please
declare a local var zeroOutScores initialized to this and pass in.
##########
solr/modules/ltr/src/java/org/apache/solr/ltr/LTRRescorer.java:
##########
@@ -233,7 +235,14 @@ protected static boolean scoreSingleHit(
boolean logHit = false;
scorer.getDocInfo().setOriginalDocScore(hit.score);
+ QueryLimits queryLimits = QueryLimits.getCurrentLimits();
Review Comment:
Ah; I see you do this here so that LTRInterleavingRescorer will incorporate
this, which overrides `scoreFeatures` completely yet calls `scoreSingleHit`.
Okay fine. But note `scoreFeatures` has lots of code duplication; it'd
benefit from a refactor.
If you want to keep queryLimits check in `scoreSingleHit` then you needn't
declare queryLimits to only then call a method on it 2 lines later (kinda
weird; why score in-between?). Just do
`QueryLimits.getCurrentLimits().maybeExitWithPartialResults(...)`
##########
solr/modules/ltr/src/java/org/apache/solr/ltr/LTRRescorer.java:
##########
@@ -233,7 +235,14 @@ protected static boolean scoreSingleHit(
boolean logHit = false;
scorer.getDocInfo().setOriginalDocScore(hit.score);
+ QueryLimits queryLimits = QueryLimits.getCurrentLimits();
Review Comment:
IMO it's better do do such checks at the spot where there is a loop over
something like docs. This also allows fetching the QueryLimits outside the
loop. Thus I suggest modifying `scoreFeatures`. You could do the
`queryLimits.maybeExitWithPartialResults` check at the start of the `while`
loop.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
