[GitHub] [solr] cpoerschke commented on a change in pull request #13: SOLR-14690: QueryResultKey.[nc_]minExactCount

[GitBox <gi...@apache.org>]

cpoerschke commented on a change in pull request #13:
URL: https://github.com/apache/solr/pull/13#discussion_r671387779



##########
File path: solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java
##########
@@ -1304,7 +1314,14 @@ private void getDocListC(QueryResult qr, QueryCommand cmd) throws IOException {
       // so set all of them on the cache key.
       key = new QueryResultKey(q, cmd.getFilterList(), cmd.getSort(), flags, cmd.getMinExactCount());
       if ((flags & NO_CHECK_QCACHE) == 0) {
-        superset = queryResultCache.get(key);
+        final QueryResultCacheEntry qrce = queryResultCache.get(key);
+        if (qrce != null &&
+            qrce.docList.hitCountRelation() == TotalHits.Relation.EQUAL_TO &&
+            qrce.minExactCount >= key.nc_minExactCount) {

Review comment:
       So the
   
   ```
   qrce.docList.hitCountRelation() == TotalHits.Relation.EQUAL_TO &&
   qrce.minExactCount >= key.nc_minExactCount) {
   ```
   
   logic here matches the _"a query with a minExactCount restriction being able to use a cache entry from a query without the restriction."_ goal on the JIRA ticket (because queries without a restriction will result in a `EQUAL_TO` hit count relation and because queries without a restriction have a `Integer.MAX_VALUE` min exact count value).
   
   For the _"a minExactCount=100 query being able to use a minExactCount=1000 cache entry"_ goal on the JIRA ticket
   
   ```
   qrce.docList.hitCountRelation() == TotalHits.Relation.GREATER_THAN_OR_EQUAL_TO &&
   qrce.minExactCount >= key.nc_minExactCount) {
   ```
   
   seems intuitively right.
   
   But upon closer consideration I think this would mean that a query without a minExactCount restriction could use a cache entry with a `GREATER_THAN_OR_EQUAL_TO` hit count relation (if `qrce.minExactCount` and `key.nc_minExactCount` are both `Integer.MAX_VALUE`) -- could such a cache entry exist though?
   * I note that `nDocs` requested is an `int` but that `TotalHits.value` is a `long` as per https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.9.0/lucene/core/src/java/org/apache/lucene/search/TotalHits.java#L47 since 7.0 via https://issues.apache.org/jira/browse/LUCENE-7872 ticket.
   * Logically even if Solr requests an `int` quantity of results then Lucene could count up a `long` quantity of matches, so yes non-EQUAL_TO hit count relation cache entry use for a query without a minExactCount restriction looks to be a possibility theoretically.
   * Practically the numHits and totalHitsThreshold in Lucene's TopFieldCollector are currently `int` and so there's nothing to worry about here I think and it should be possible to simplify the logic to
   
   ```
   (qrce.docList.hitCountRelation() == TotalHits.Relation.EQUAL_TO ||
    qrce.docList.hitCountRelation() == TotalHits.Relation.GREATER_THAN_OR_EQUAL_TO) &&
   qrce.minExactCount >= key.nc_minExactCount) {
   ```
   
   or
   
   ```
   qrce.minExactCount >= key.nc_minExactCount) {
   ```
   
   with suitable test coverage of course.




-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org