Re: [PR] [SOLR-15030] Add score() function that returns the score of the current hit [solr]
github-actions[bot] closed pull request #3340: [SOLR-15030] Add score() function that returns the score of the current hit URL: https://github.com/apache/solr/pull/3340 -- 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]
Re: [PR] [SOLR-15030] Add score() function that returns the score of the current hit [solr]
github-actions[bot] commented on PR #3340: URL: https://github.com/apache/solr/pull/3340#issuecomment-3499920461 This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again. -- 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]
Re: [PR] [SOLR-15030] Add score() function that returns the score of the current hit [solr]
github-actions[bot] commented on PR #3340: URL: https://github.com/apache/solr/pull/3340#issuecomment-3264181476 This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the [email protected] mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution! -- 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]
Re: [PR] [SOLR-15030] Add score() function that returns the score of the current hit [solr]
dsmiley commented on PR #3340: URL: https://github.com/apache/solr/pull/3340#issuecomment-3051126372 Looks like you may be solving the same problem in ValueSourceAugmenter that I solved in SOLR-5707 #1244 which is ready to merge -- 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]
Re: [PR] [SOLR-15030] Add score() function that returns the score of the current hit [solr]
dsmiley commented on PR #3340: URL: https://github.com/apache/solr/pull/3340#issuecomment-3015596495 For consistency with how the score is referenced in `fl` & `sort` and the upcoming ExpressionsValueSourceParser, I think simply `score` is how this should be referenced, not `score()`. -- 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]
Re: [PR] [SOLR-15030] Add score() function that returns the score of the current hit [solr]
AndreyBozhko commented on code in PR #3340:
URL: https://github.com/apache/solr/pull/3340#discussion_r2076124793
##
solr/core/src/java/org/apache/solr/search/SolrReturnFields.java:
##
@@ -547,6 +547,12 @@ private void addField(
String disp = (key == null) ? field : key;
augmenters.addTransformer(new MatchScoreAugmenter(disp));
scoreDependentFields.put(disp, disp.equals(MATCH_SCORE) ? "" :
MATCH_SCORE);
+} else if (key != null && isPseudoField) {
+ // SOLR-15030: a pseudo-field based on the function query may need
scores,
+ // so we consider all pseudo-fields as potentially requiring scores.
+ // At the same time, we don't set _wantScore = true because the field
+ // list must explicitly include 'score' field to enable scores
Review Comment:
What I thought was that for the query with `fl=id,a,b,s:sum(x,y)` - even
though it has a pseudo-field, that alone is not enough to conclude that the
query requires scores.
--
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]
Re: [PR] [SOLR-15030] Add score() function that returns the score of the current hit [solr]
dsmiley commented on code in PR #3340:
URL: https://github.com/apache/solr/pull/3340#discussion_r2075967221
##
solr/core/src/java/org/apache/solr/search/SolrReturnFields.java:
##
@@ -547,6 +547,12 @@ private void addField(
String disp = (key == null) ? field : key;
augmenters.addTransformer(new MatchScoreAugmenter(disp));
scoreDependentFields.put(disp, disp.equals(MATCH_SCORE) ? "" :
MATCH_SCORE);
+} else if (key != null && isPseudoField) {
+ // SOLR-15030: a pseudo-field based on the function query may need
scores,
+ // so we consider all pseudo-fields as potentially requiring scores.
+ // At the same time, we don't set _wantScore = true because the field
+ // list must explicitly include 'score' field to enable scores
Review Comment:
I assume that the work-around to `DocTransformer.needsScores` not existing
is that the user should explicitly add `score` to `fl`?
Lucene standardizes on `needsScores` terminology. Solr ReturnFields &
ResultContext has "wants" terminology.
--
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]
Re: [PR] [SOLR-15030] Add score() function that returns the score of the current hit [solr]
dsmiley commented on code in PR #3340:
URL: https://github.com/apache/solr/pull/3340#discussion_r2075937889
##
solr/core/src/java/org/apache/solr/search/SolrReturnFields.java:
##
@@ -547,6 +547,12 @@ private void addField(
String disp = (key == null) ? field : key;
augmenters.addTransformer(new MatchScoreAugmenter(disp));
scoreDependentFields.put(disp, disp.equals(MATCH_SCORE) ? "" :
MATCH_SCORE);
+} else if (key != null && isPseudoField) {
+ // SOLR-15030: a pseudo-field based on the function query may need
scores,
+ // so we consider all pseudo-fields as potentially requiring scores.
+ // At the same time, we don't set _wantScore = true because the field
+ // list must explicitly include 'score' field to enable scores
Review Comment:
nor does a `ValueSource` have `needsScores`. But `DoubleValuesSource`
(Lucene's modern replacement over legacy VS) _does_.
Addressing that is perhaps for another issue/PR. Progress not perfection.
--
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]
Re: [PR] [SOLR-15030] Add score() function that returns the score of the current hit [solr]
dsmiley commented on code in PR #3340:
URL: https://github.com/apache/solr/pull/3340#discussion_r2075758077
##
solr/core/src/test/org/apache/solr/search/function/ScoreFunctionDistribTest.java:
##
@@ -0,0 +1,188 @@
+package org.apache.solr.search.function;
+
+import java.util.List;
+import java.util.concurrent.TimeUnit;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.UpdateRequest;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.SolrException;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+public class ScoreFunctionDistribTest extends SolrCloudTestCase {
+
+ private static final String COLLECTION = "coll1";
+
+ private static SolrClient client;
Review Comment:
don't store a static field with a SolrClient; anyone can call
cluster.getSolrClient if needed
##
solr/core/src/test/org/apache/solr/search/function/ScoreFunctionDistribTest.java:
##
Review Comment:
I love this test -- using multi-line strings and assertJSONequals! (ugh that
capitalization choice is terrible though; for another PR)
##
solr/core/src/java/org/apache/solr/search/SolrReturnFields.java:
##
@@ -547,6 +547,12 @@ private void addField(
String disp = (key == null) ? field : key;
augmenters.addTransformer(new MatchScoreAugmenter(disp));
scoreDependentFields.put(disp, disp.equals(MATCH_SCORE) ? "" :
MATCH_SCORE);
+} else if (key != null && isPseudoField) {
+ // SOLR-15030: a pseudo-field based on the function query may need
scores,
+ // so we consider all pseudo-fields as potentially requiring scores.
+ // At the same time, we don't set _wantScore = true because the field
+ // list must explicitly include 'score' field to enable scores
Review Comment:
I'm confused or challenge what you say in this sentence. A DocTransformer
could want the score. Granted, a DocTransformer doesn't have a `wantsScore` /
`needsScore` so... :-/
--
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]
Re: [PR] [SOLR-15030] Add score() function that returns the score of the current hit [solr]
dsmiley commented on code in PR #3340:
URL: https://github.com/apache/solr/pull/3340#discussion_r2075750387
##
solr/core/src/java/org/apache/solr/response/transform/ValueSourceAugmenter.java:
##
@@ -101,4 +107,24 @@ protected void setValue(SolrDocument doc, Object val) {
doc.setField(name, val);
}
}
+
+ private static class ScoreAndDoc extends Scorable {
Review Comment:
One top level class makes sense!
--
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]
Re: [PR] [SOLR-15030] Add score() function that returns the score of the current hit [solr]
AndreyBozhko commented on code in PR #3340:
URL: https://github.com/apache/solr/pull/3340#discussion_r2072207579
##
solr/core/src/java/org/apache/solr/response/transform/ValueSourceAugmenter.java:
##
@@ -101,4 +107,24 @@ protected void setValue(SolrDocument doc, Object val) {
doc.setField(name, val);
}
}
+
+ private static class ScoreAndDoc extends Scorable {
Review Comment:
There are three other `ScoreAndDoc` helper classes (with identical
implementation) scattered throughout the codebase. Maybe it's worth
consolidating into one.
--
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]
Re: [PR] [SOLR-15030] Add score() function that returns the score of the current hit [solr]
AndreyBozhko commented on PR #3340: URL: https://github.com/apache/solr/pull/3340#issuecomment-2839600988 Thanks for the reviews - I'll try to put together some test cases for distributed queries, to reproduce the behavior that @HoustonPutman mentioned. -- 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]
Re: [PR] [SOLR-15030] Add score() function that returns the score of the current hit [solr]
dsmiley commented on code in PR #3340:
URL: https://github.com/apache/solr/pull/3340#discussion_r2066886029
##
solr/core/src/java/org/apache/solr/search/FunctionRangeQuery.java:
##
@@ -113,4 +122,26 @@ public boolean equals(Object obj) {
public int hashCode() {
return 31 * classHash() + rangeFilt.hashCode();
}
+
+ private static class DelegatingCollectorWrapper extends Scorable {
Review Comment:
the name should reflect it's Scorable-ness. Maybe
DelegatingCollectorScorable
--
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]
Re: [PR] [SOLR-15030] Add score() function that returns the score of the current hit [solr]
HoustonPutman commented on PR #3340: URL: https://github.com/apache/solr/pull/3340#issuecomment-2839314706 This is great, but in order to get this to work for distributed queries, we need to give `SolrReturnFields` the information that this Transformer/ValueSource requires scores. That way it is computed on the first pass `GET_TOP_IDS` distributed query, and not in `GET_FIELDS`, which won't have the score information anymore. I can help with this later this week maybe. -- 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]
