ercsonusharma commented on code in PR #5119:
URL: https://github.com/apache/solr/pull/5119#discussion_r4306929638


##########
solr/core/src/java/org/apache/solr/handler/component/combine/ReciprocalRankFusion.java:
##########
@@ -128,25 +180,32 @@ public SimpleOrderedMap<Explanation> getExplanations(
       SolrParams solrParams) {
     int kVal = solrParams.getInt(CombinerParams.COMBINER_RRF_K, this.k);
     SimpleOrderedMap<Explanation> docIdsExplanations = new 
SimpleOrderedMap<>();
-    Map<String, Integer[]> docIdToRanks = getRanks(queriesDocMap.values(), 
combinedQueriesDocs);
+    Map<String, Integer[]> docIdToRanks = getRanks(queryKeys, queriesDocMap, 
combinedQueriesDocs);
     for (ShardDoc shardDoc : combinedQueriesDocs) {
       String docId = shardDoc.id.toString();
       Integer[] rankPerQuery = docIdToRanks.get(docId);
       Explanation fullDocIdExplanation =
           Explanation.match(
-              shardDoc.score, getReciprocalRankFusionExplain(queryKeys, 
rankPerQuery, kVal));
+              shardDoc.score,
+              getReciprocalRankFusionExplain(queryKeys, rankPerQuery, kVal, 
solrParams));
       docIdsExplanations.add(docId, fullDocIdExplanation);
     }
     return docIdsExplanations;
   }
 
   private String getReciprocalRankFusionExplain(
-      String[] queryKeys, Integer[] rankPerQuery, int kVal) {
+      String[] queryKeys, Integer[] rankPerQuery, int kVal, SolrParams 
solrParams) {
     StringBuilder reciprocalRankFusionExplain = new StringBuilder();
     StringJoiner scoreComponents = new StringJoiner(" + ");
-    for (Integer rank : rankPerQuery) {
+    for (int i = 0; i < queryKeys.length; i++) {
+      Integer rank = rankPerQuery[i];
       if (rank != null) {
-        scoreComponents.add("1/(" + kVal + "+" + rank + ")");
+        float weight = getWeight(solrParams, queryKeys[i]);

Review Comment:
   Nit: compute the weights once in combine and reuse them in explain instead 
of fetching it for each doc.  



##########
solr/core/src/java/org/apache/solr/handler/component/combine/ReciprocalRankFusion.java:
##########
@@ -54,32 +55,77 @@ public int getK() {
     return k;
   }
 
+  /**
+   * Reads the RRF weight for a query from {@code 
combiner.rrf.weight.<queryKey>}, defaulting to
+   * {@link CombinerParams#DEFAULT_COMBINER_RRF_WEIGHT}. A negative weight is 
rejected with a 400.
+   */
+  private float getWeight(SolrParams solrParams, String queryKey) {
+    float weight =
+        solrParams.getFloat(
+            CombinerParams.COMBINER_RRF_WEIGHT + "." + queryKey,
+            CombinerParams.DEFAULT_COMBINER_RRF_WEIGHT);
+    if (weight < 0) {

Review Comment:
   0 should also be rejected as well as infinity. ` (!(weight > 0) || 
!Float.isFinite(weight))` covers all of them.
   Also, instead of checking this condition separately in this method, it 
should be clubbed together with `validateWeightNames` method by renaming it.



##########
solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-11.adoc:
##########
@@ -53,3 +53,12 @@ A request with no other parameters returns every collection.
 `collection`, `shard`, and `_route_` narrow which part of the tree is returned.
 
 The v1 `CLUSTERSTATUS` command is unchanged, and `includeAll` still defaults 
to true.
+
+== New Features
+
+=== Weighted Reciprocal Rank Fusion

Review Comment:
   This section shouldn't be needed as this page is for breaking or major 
upgrade changes; the changelog and ref-guide section are enough for an opt-in 
parameter. 



##########
solr/solr-ref-guide/modules/query-guide/pages/json-combined-query-dsl.adoc:
##########
@@ -75,6 +75,11 @@ The query structure is similar to JSON Query DSL except for 
how multiple queries
    Any other algorithm can be configured using 
xref:json-combined-query-dsl.adoc#combiner-algorithm-plugin[plugin].
 `combiner.rrf.k` | Default: `60`::
    The k parameter in the RRF algorithm.
+`combiner.rrf.weight.<queryName>` | Default: `1.0`::
+   A non-negative multiplier applied to the reciprocal rank contribution of 
the query named `<queryName>` (where `<queryName>` is an entry in 
`combiner.query`).
+   Omit the parameter to give a query equal weight, which is the default 
behavior, so existing requests are unchanged.

Review Comment:
   "so existing requests are unchanged" is upgrade/changelog wording. In the 
ref guide, should just say: "If omitted, the query's weight is 1.0, so all 
queries contribute equally by default."



-- 
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]

Reply via email to