Open asfimport opened 11 years ago
Ryan McKinley (@ryantxu) (migrated from JIRA)
Earlier today, I committed a small change on #4868 but realize the change was based on not-understanding some assumptions.
We should clarify the assumptions in javadoc and/or the API.
My understanding now is that the API returned ConstantScoreQuery rather then Query to explicitly say the score is undefined (not necessarily distance/overlap/whatever).
I see two directions for this call:
#1
- Return a Query, and in the javadoc for SpatialStrategy say that the score value is undefined (but strategies may have a meaningful score)
#2
- return ConstantScore and make the javadoc explicit that the score is not meaningful
I vote for #1
since it is more general and lets applications/implementations work more easily
Ryan McKinley (@ryantxu) (migrated from JIRA)
FYI, earlier commits were:
David Smiley (@dsmiley) (migrated from JIRA)
So #2
was the behavior in place; somewhat altered now since your commit yesterday.
I can understand the draw to #1
but leaving it completely undefined doesn't feel right to me. How about this proposed definition:
By default, the score should be a spatial relevance metric between 0 and 1 where 1 is the most relevant. This might happen internally via makeRecipDistanceValueSource() or it might be another algorithm left up to the Strategy. If the Strategy or SpatialArgs is somehow configured to use something else, such as simply using the distance as the score, then that will override the default behavior.
Ryan McKinley (@ryantxu) (migrated from JIRA)
I don't think that saying the score should be between 0-1 helps anything. This seem to just overdefine things.
I think improving javadocs to say what is expected is the key thing, not forcing a unified meaning to the score.
Applications can sort out what the different things are possible, but Strategies will know (within themselves what is possible/relevant)
Chris Male (migrated from JIRA)
I agree with Ryan, we shouldn't try to over-define this. Returning Query gives the Strategies freedom to have a meaningful score if they support it. But we should just add a simple comment stating that the score from the Query may or may not be meaningful and the Strategy used should be checked for further details.
Chris Male (migrated from JIRA)
Another option, more big picture I guess, is to take this opportunity and remove the Strategy abstraction. We've touched upon this in other issues, but the fact is that each Strategy (including those not contributed) behaves differently and the notion of score is a big example of this. There is some consistently in the Prefix Strategies so having an abstraction there probably helps but otherwise I think we should just dump Strategy and let some Strategies return a Query with meaningful score and some return a CSQ showing that their score is meaningless.
David Smiley (@dsmiley) (migrated from JIRA)
The point of a 0-1 score is that it is usable for both sorting and relevancy boosting. And I don't have to look at the docs to know I can use the result of the query these ways.
David Smiley (@dsmiley) (migrated from JIRA)
FYI Solr's AbstractSpatialFieldType#getQueryFromSpatialArgs() has this excerpt:
String score = (localParams == null ? null : localParams.get(SCORE_PARAM));
if (score == null || "none".equals(score) || "".equals(score)) {
//FYI Solr FieldType doesn't have a getFilter(). We'll always grab
// getQuery() but it's possible a strategy has a more efficient getFilter
// that could be wrapped -- no way to know.
//See [SOLR-2883](https://issues.apache.org/jira/browse/SOLR-2883) needScore
return strategy.makeQuery(spatialArgs); //ConstantScoreQuery
}
With the change that Ryan proposes in this issue, that SpatialStrategy#makeQuery can return a scoring query if it chooses, this code should no longer return makeQuery(), it should wrap makeFilter() in a ConstantScoreQuery. Not a big deal but I just want to make note of it as it affects this issue.
SpatialStrategy#makeQuery() returns a Query, but the docs don't make it clear with the score value should be.
Migrated from LUCENE-4616 by Ryan McKinley (@ryantxu), updated Dec 28 2012