Skip to content

[SOLR-15030] Add score() function that returns the score of the current hit #3340

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

[SOLR-15030] Add score() function that returns the score of the current hit #3340

wants to merge 11 commits into from

Conversation

AndreyBozhko
Copy link
Contributor

https://issues.apache.org/jira/browse/SOLR-15030

Description

Add score() function that can be used in the following contexts:

  • pseudo-fields
  • post-filter queries

Tests

Added tests to demonstrate the new behavior.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

@github-actions github-actions bot added documentation Improvements or additions to documentation tests cat:search labels Apr 29, 2025
@HoustonPutman
Copy link
Contributor

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.

@dsmiley dsmiley self-requested a review April 29, 2025 16:44
dsmiley
dsmiley approved these changes Apr 29, 2025
View reviewed changes
Copy link
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done!

solr/core/src/java/org/apache/solr/search/FunctionRangeQuery.java Outdated
@@ -113,4 +122,26 @@ public boolean equals(Object obj) {
public int hashCode() {
return 31 * classHash() + rangeFilt.hashCode();
}

private static class DelegatingCollectorWrapper extends Scorable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the name should reflect it's Scorable-ness. Maybe DelegatingCollectorScorable

@AndreyBozhko
Copy link
Contributor Author

Thanks for the reviews - I'll try to put together some test cases for distributed queries, to reproduce the behavior that @HoustonPutman mentioned.

AndreyBozhko
AndreyBozhko commented May 2, 2025
View reviewed changes
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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are three other ScoreAndDoc helper classes (with identical implementation) scattered throughout the codebase. Maybe it's worth consolidating into one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One top level class makes sense!

dsmiley
dsmiley reviewed May 6, 2025
View reviewed changes
solr/core/src/test/org/apache/solr/search/function/ScoreFunctionDistribTest.java Outdated

private static final String COLLECTION = "coll1";

private static SolrClient client;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Comment on lines +553 to +554
// At the same time, we don't set _wantScore = true because the field
// list must explicitly include 'score' field to enable scores
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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... :-/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dsmiley dsmiley mentioned this pull request May 6, 2025
Merged
6 tasks
@dsmiley
Copy link
Contributor

dsmiley commented May 15, 2025

Good stuff here; looking forward to it getting in.

@HoustonPutman HoustonPutman mentioned this pull request Jun 3, 2025
Merged
@dsmiley
Copy link
Contributor

dsmiley commented Jun 28, 2025

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().

@dsmiley
Copy link
Contributor

dsmiley commented Jul 9, 2025

Looks like you may be solving the same problem in ValueSourceAugmenter that I solved in SOLR-5707 #1244 which is ready to merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsmiley dsmiley dsmiley approved these changes

Assignees
No one assigned
Labels
cat:search documentation Improvements or additions to documentation tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AndreyBozhko @HoustonPutman @dsmiley