cpoerschke commented on code in PR #2022:
URL: https://github.com/apache/solr/pull/2022#discussion_r1636944566
##########
solr/modules/ltr/src/java/org/apache/solr/ltr/interleaving/LTRInterleavingScoringQuery.java:
##########
@@ -45,4 +45,30 @@ public Set<Integer> getPickedInterleavingDocIds() {
public void setPickedInterleavingDocIds(Set<Integer>
pickedInterleavingDocIds) {
this.pickedInterleavingDocIds = pickedInterleavingDocIds;
}
+
+ @Override
+ public LTRScoringQuery clone() {
+ LTRInterleavingScoringQuery cloned =
+ new LTRInterleavingScoringQuery(
+ getScoringModel(), getExternalFeatureInfo(), getThreadModule());
+ cloned.setOriginalQuery(getOriginalQuery());
+ cloned.setFeatureLogger(getFeatureLogger());
+ cloned.setRequest(getRequest());
+ cloned.setPickedInterleavingDocIds(getPickedInterleavingDocIds());
+ return cloned;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ return sameClassAs(o) && equalsTo(getClass().cast(o));
+ }
+
+ private boolean equalsTo(LTRInterleavingScoringQuery other) {
+ boolean superSame = (super.equalsTo(other) &&
other.equalsTo((LTRScoringQuery) this));
+ if (this.getPickedInterleavingDocIds() != null &&
other.getPickedInterleavingDocIds() != null) {
Review Comment:
minor/style/subjective
```suggestion
if (this.pickedInterleavingDocIds != null &&
other.getPickedInterleavingDocIds() != null) {
```
##########
solr/modules/ltr/src/java/org/apache/solr/ltr/interleaving/LTRInterleavingScoringQuery.java:
##########
@@ -45,4 +45,30 @@ public Set<Integer> getPickedInterleavingDocIds() {
public void setPickedInterleavingDocIds(Set<Integer>
pickedInterleavingDocIds) {
this.pickedInterleavingDocIds = pickedInterleavingDocIds;
}
+
+ @Override
+ public LTRScoringQuery clone() {
+ LTRInterleavingScoringQuery cloned =
+ new LTRInterleavingScoringQuery(
+ getScoringModel(), getExternalFeatureInfo(), getThreadModule());
+ cloned.setOriginalQuery(getOriginalQuery());
+ cloned.setFeatureLogger(getFeatureLogger());
+ cloned.setRequest(getRequest());
+ cloned.setPickedInterleavingDocIds(getPickedInterleavingDocIds());
Review Comment:
wondering of the `efi` map and the `pickedDocIds` set should be cloned too?
##########
solr/modules/ltr/src/test/org/apache/solr/ltr/TestLTRQParserPlugin.java:
##########
@@ -78,6 +79,60 @@ public void ltrModelIsEmptyTest() throws Exception {
assertTrue(res.contains("the model 0 is empty"));
}
+ @Test
+ public void ltrRepeatedQueriesCachedTest() throws Exception {
+ final SolrQuery query = new SolrQuery();
+ query.setQuery("bloomberg baz buzz");
+ query.add("q", "bloomberg baz buzz");
+ query.add("qf", "title description");
+ query.add("defType", "edismax");
+ query.add("fl", "*, score");
+ query.add("rows", "4");
+ query.add("fv", "true");
+ query.add("rq", "{!ltr model=6029760550880411648 reRankDocs=3}");
+
+ // Different order for top 3 reranked, but last one is the same top
nonreranked doc
+ assertJQ(
+ "/query" + query.toQueryString(),
+ "/response/docs/[0]/id=='7'",
+ "/response/docs/[1]/id=='8'",
+ "/response/docs/[2]/id=='9'",
+ "/response/docs/[3]/id=='6'");
+
+ // to check caching
+ assertJQ(
+ "/query" + query.toQueryString(),
+ "/response/docs/[0]/id=='7'",
+ "/response/docs/[1]/id=='8'",
+ "/response/docs/[2]/id=='9'",
+ "/response/docs/[3]/id=='6'");
+
+ String resp =
+ restTestHarness.adminQuery(
+
"/admin/metrics?group=core&&prefix=CACHE.searcher.queryResultCache");
+ String error =
+ JSONTestUtil.match(
+ resp,
"/metrics/solr.core.collection1/CACHE.searcher.queryResultCache/hits==1");
Review Comment:
perhaps at the start of the test we could also test that the cache is empty
`hits==0` to start with?
##########
solr/modules/ltr/src/test/org/apache/solr/ltr/TestLTRQParserPlugin.java:
##########
@@ -78,6 +79,60 @@ public void ltrModelIsEmptyTest() throws Exception {
assertTrue(res.contains("the model 0 is empty"));
}
+ @Test
+ public void ltrRepeatedQueriesCachedTest() throws Exception {
+ final SolrQuery query = new SolrQuery();
+ query.setQuery("bloomberg baz buzz");
+ query.add("q", "bloomberg baz buzz");
+ query.add("qf", "title description");
+ query.add("defType", "edismax");
+ query.add("fl", "*, score");
+ query.add("rows", "4");
+ query.add("fv", "true");
+ query.add("rq", "{!ltr model=6029760550880411648 reRankDocs=3}");
Review Comment:
wondering how practical it would be to have test coverage for the
interleaving case too.
##########
solr/modules/ltr/src/test/org/apache/solr/ltr/TestLTRQParserPlugin.java:
##########
@@ -78,6 +79,60 @@ public void ltrModelIsEmptyTest() throws Exception {
assertTrue(res.contains("the model 0 is empty"));
}
+ @Test
+ public void ltrRepeatedQueriesCachedTest() throws Exception {
+ final SolrQuery query = new SolrQuery();
+ query.setQuery("bloomberg baz buzz");
+ query.add("q", "bloomberg baz buzz");
+ query.add("qf", "title description");
+ query.add("defType", "edismax");
+ query.add("fl", "*, score");
+ query.add("rows", "4");
+ query.add("fv", "true");
+ query.add("rq", "{!ltr model=6029760550880411648 reRankDocs=3}");
+
+ // Different order for top 3 reranked, but last one is the same top
nonreranked doc
+ assertJQ(
+ "/query" + query.toQueryString(),
+ "/response/docs/[0]/id=='7'",
+ "/response/docs/[1]/id=='8'",
+ "/response/docs/[2]/id=='9'",
+ "/response/docs/[3]/id=='6'");
+
+ // to check caching
+ assertJQ(
+ "/query" + query.toQueryString(),
+ "/response/docs/[0]/id=='7'",
+ "/response/docs/[1]/id=='8'",
+ "/response/docs/[2]/id=='9'",
+ "/response/docs/[3]/id=='6'");
+
+ String resp =
+ restTestHarness.adminQuery(
+
"/admin/metrics?group=core&&prefix=CACHE.searcher.queryResultCache");
Review Comment:
```suggestion
"/admin/metrics?group=core&prefix=CACHE.searcher.queryResultCache");
```
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
