You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "risdenk (via GitHub)" <gi...@apache.org> on 2025/06/30 12:51:46 UTC
[PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
risdenk opened a new pull request, #2221:
URL: https://github.com/apache/solr/pull/2221
https://issues.apache.org/jira/browse/SOLR-17131
# Description
If a query has `rows=0` Solr does not need to sort the results or even compute the score. This means the filterCache can be more efficient to compute the result and Solr doesn't spend time on unnecessarily sorting and scoring large result sets for it to be thrown away.
There is one subtle difference from this change - `maxScore` is no longer returned if `rows=0` which seems like a reasonable tradeoff. If `maxScore` is needed then set `rows=1`. `maxScore` can't reliably be compared across searches anyway so no reason to have it when `rows=0`.
This was found while looking at https://issues.apache.org/jira/browse/SOLR-14765 and trying to reason through the case of `rows=0` that are sometimes used to just determine count of matching results. This change adds the check for `rows=0` before other checks to ensure that we can short circuit scoring even if there is a scoring query. We also don't add the query to the filter cache since that could result in filter cache thrashing.
In some tests, we found that queries with `rows=0` p99 improved by 50% and CPU dropped by 5%.
# Tests
Updated tests in TestMainQueryCaching to account for this optimization.
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the `main` branch.
- [x] I have run `./gradlew check`.
- [x] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-1915232387
Hi,
I already responded on Slack, but I now found this PR and wanted to say some additional things:
There is also a separate collector and on top of that a separate method in `IndexSearcher`, which automatically does a lot of the top-docs related stuff. The problem with a normal `TopDocs` or `TopFieldDocs` collector is that the empty PQ has some additional overhead (although the score is not calculated and returned as 0).
First of all: SolrIndexSearcher should also implement support for using the optimized methods and collectors, unless a `DocSet` is needed (for facets/aggs). Using the `TotalHitCountCollector` is one thing to do (this needs to be done more down the line in the search code of Solr, but it's worth to do. But as there is now support to short-circuit count queries on the Query level, you should also think of using `IndexSearcher#count()` directly: E.g., this one returns `IndexReader#numDocs()` for `MatchAllDocsQuery` (thats a stupid simple case), but it can also return quick counts when there are no deleted docs in a segment and the statistics in index can answer the query.
The changes here look fine (although I can't follow the discussion about those crazy flags true/false/whatever), but in addition to the code here, in case of no TopDocs and no facets, the code should call `IndexSearcher#count()`. In the facet case, it should at least use the `TotalHitCountCollector` as the final sink.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-2063882591
@risdenk are there any loose ends here or it mergeable as-is, notwithstanding a CHANGES.txt entry?
Proposed CHANGES.txt under 9.6 optimizations:
> SOLR-17131: Queries: rows=0, can skip sorting and doc collection. (Kevin Risden, David Smiley)
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #2221: SOLR-17131: Optimize rows=0 since score/sort isn't necessary
URL: https://github.com/apache/solr/pull/2221
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-3514075692
I wonder if it'd be possible to handle this all at the shard level. If the request is really constant-score, then in principle it should be possible to populate `maxScore` (and pseudo-`score` fields) by scoring just a single doc. This would address the backcompat concerns that @gus-asf raised.
I came around to this thought because `rows=0` isn't the only case that it makes sense to optimize here. As David suggests, "explicitly set "fl" to something that does not include score" is indeed a workaround for the `rows=0` case -- but for sort by score alone (and MatchAllDocs or constant-score query) `rows > 0` can also skip sort and score evaluation (simply returning a DocList in index order), which would be _way_ faster than any "workaround" sort, for the `rows > 0` case.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-2070929291
I think the change is "safe" from a test stability standpoint.
There is one change to be mentioned in CHANGES.txt and probably in release notes. If rows=0 then maxScore will no longer be returned. The user must ask for at least one row if the score is desired.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #2221:
URL: https://github.com/apache/solr/pull/2221#discussion_r1471790378
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -1682,7 +1684,6 @@ private void getDocListC(QueryResult qr, QueryCommand cmd) throws IOException {
}
}
} else {
- fullSortCount.increment();
Review Comment:
Addressed in 9bc979a
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-1913695210
I also think a `queryResultWindowSize` of 1 (which is the default if it isn't specified in solrconfig and isn't documented AFAICT) should not bump up a request for no rows to a window of a paltry 1 doc. I don't think that was intended; I think what was intended was to effectively have no window if it isn't specified. I can see using a default of 1 _seems_ to convey that (for typical queries that have results) but not for rows=0 case. Likely an oversight long ago.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-3543266038
After discussion with @risdenk, just pushed 3 commits here:
1. 91e9395bf93396fde39deb751b3bf36b6c7687c8 merges in current `apache-solr/main`
2. 471f1279428cdab5e6d42a908ebacec5857867f8 reverts the special param manipulation for `rows=0` (the root of the back-compat concerns, iiuc)
3. 99411d2c34a4720802a8a933dd7aaaf7f12e61c2 a first pass at implementing the approach [suggested above](https://github.com/apache/solr/pull/2221#issuecomment-3514075692).
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-1913296690
> Is there a unit test you can recommend that I might run in a debugger looking at key spots, that would show that a score is being computed wastefully?
`TestMainQueryCaching#testConstantScoreFlScore` and put a breakpoint at `SolrIndexSearcher` line 1620
If there is `rows=0` and one of three cases, the scores are computed and sorted even though its not needed:
* `fl` includes `score`
* `sort` includes `score`
* `q` is a non constant query
If you walk through the existing if checks in `SolrIndexSearcher` line 1620, the first case isn't chosen since scores are requested. The second case doesn't match since its not a `*:*` or `MatchAllDocsQuery` - even if its a constant score query `useFilterForSortedQuery` is false and we don't really want to enable (and its false by default) that case (puts the `q` in the filter cache - related to your other question I'll answer below). The final case is `useFilterCacheForDynamicScoreQuery` which if `useFilterForSortedQuery` is false (which it is by default) we skip optimizations.
The changes to `TestMainQueryCaching` also show the other cases where the `rows=0` check improves things. Avoids a full sort.
> This seems contradictory. If the filterCache can compute the result, then wouldn't disabling the filterCache for the query (as I see you do in the PR) not use the filter cache then? I'm referring to the query, not the filters (which are processed the same either way).
So the check I added makes sure we don't add the `q` query to the `filterCache`. We want to use the filterCache to compute the result, but don't want to store the `q` in the `filterCache` after its computed. This is similar to `useFilterForSortedQuery` but this is specifically the case of a `rows=0` query.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-1914999026
> > getDocSetNC uses the filterCache docset
>
> Can you elaborate where this is so?
This existed prior to my change due to SOLR-14765 - `SolrIndexSearcher` before my PR lines 1658-1660 and with my PR lines 1673-1675.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2221:
URL: https://github.com/apache/solr/pull/2221#discussion_r1574044713
##########
solr/benchmark/src/java/org/apache/solr/bench/MiniClusterState.java:
##########
Review Comment:
Oh I see now, there's _also_ a setter. Shouldn't we be setting this system property in the setter then?
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-1914960070
You claim that your approach to this uses the filter cache (for `q`). Yet it does not because it calls `getDocSetNC`. I feel compelled to -1 this because that's what `useFilterCache` means. But we agree on the goals -- don't compute the score!
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-1915205945
Let's use GH links to ensure we understand each other. You are referring to [this](https://github.com/apache/solr/blob/e53bf109ba2d35e1aff6f9a5c428fff5eff63f7b/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java#L1658)?
```
List<Query> filterList = cmd.getFilterList();
if (filterList != null && !filterList.isEmpty()) {
out.docSet = DocSetUtil.getDocSet(out.docSet.intersection(getDocSet(filterList)), this);
}
```
This shows that the list of filter queries goes through the filter cache. But that's true no matter what (other code paths ultimately lead to the same but in different methods deeper). The question at hand is how is the `q` param processed, which in this code gets turned into a DocSet saved to `out.docSet`. Immediately before these lines, it's populated via `getDocSet`. This is what `useFilterCache` *means*. In your PR, you added a condition to not do this, thus not use the filter cache for `q`.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-3762293705
This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the dev@solr.apache.org mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution!
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #2221: SOLR-17131: Optimize rows=0 since score/sort isn't necessary
URL: https://github.com/apache/solr/pull/2221
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-1915447012
@uschindler See [getDocListNC](https://github.com/apache/solr/blob/326afddeddaa2092086e6f6b27f8bc35bf6d79cf/solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java#L1812) which already seems to be pretty well optimized when `lastDocRequested <= 0` (i.e. `rows==0`), and in conjunction also when `!needScores`. The reason some of the Lucene convenience collectors & IndexSearcher.count aren't used is because this code is juggling a multitude of scenarios, such as when there might be a so-called `PostFilter`, thus leading to another Collector to add to the head of the collectors. Care must be taken before adding yet another optimization -- does it *actually* add value or it just to feel good to use another Lucene class. `getDocListAndSetNC` follows this method and is similar.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2221:
URL: https://github.com/apache/solr/pull/2221#discussion_r1470537795
##########
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java:
##########
@@ -169,6 +160,20 @@ public void prepare(ResponseBuilder rb) throws IOException {
try {
QParser parser = QParser.getParser(rb.getQueryString(), defType, req);
+
+ SortSpec sortSpec = parser.getSortSpec(true);
+ rb.setSortSpec(sortSpec);
+ boolean someDocs = sortSpec == null || sortSpec.getCount() != 0;
+
+ // Set field flags
Review Comment:
The try-catch is about query parsing / syntax errors; sortSpec & returnFields is separate; perhaps should go after.
##########
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java:
##########
@@ -147,15 +147,6 @@ public void prepare(ResponseBuilder rb) throws IOException {
}
}
- // Set field flags
Review Comment:
BTW, I cannot figure out why "field" is used to describe/qualify the flags. If you can't either, I recommend as you move this comment to simply omit the "field" word.
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -1682,7 +1684,6 @@ private void getDocListC(QueryResult qr, QueryCommand cmd) throws IOException {
}
}
} else {
- fullSortCount.increment();
Review Comment:
nocommit -- to move elsewhere still
##########
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java:
##########
@@ -169,6 +160,20 @@ public void prepare(ResponseBuilder rb) throws IOException {
try {
QParser parser = QParser.getParser(rb.getQueryString(), defType, req);
+
+ SortSpec sortSpec = parser.getSortSpec(true);
+ rb.setSortSpec(sortSpec);
+ boolean someDocs = sortSpec == null || sortSpec.getCount() != 0;
+
+ // Set field flags
Review Comment:
I was anticipating seeing a rows check in changes to QueryComponent but don't see it yet
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-1914730907
> The other code path is actually already optimized for rows==0 and no scores
Which code path are you talking about?
> I think the solution is to not save the results into queryResultCache when rows==0; thus the window won't be applied either.
Not sure I agree with that. We should be able to store the result in the queryResultCache so that we don't have to recompute for the same exact query a second time?
> I also think a queryResultWindowSize of 1 (which is the default if it isn't specified in solrconfig and isn't documented AFAICT)
Its documented here https://solr.apache.org/guide/solr/latest/configuration-guide/caches-warming.html#queryresultwindowsize-element
I know in our case we do set `queryResultWindowSize` in `solrconfig.xml`.
Either way I'll look into QueryComponent some. @dsmiley as far as you are aware this change even though maybe not the best place to put it - doesn't have any negative side effects right?
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on code in PR #2221:
URL: https://github.com/apache/solr/pull/2221#discussion_r2535092354
##########
solr/core/src/java/org/apache/solr/search/QueryUtils.java:
##########
@@ -53,34 +53,44 @@ public static boolean isNegative(Query q) {
return true;
}
+ /** Use instead: {@link #getConstantScore(Query)} */
+ @Deprecated
+ public static boolean isConstantScoreQuery(Query q) {
+ return getConstantScore(q) != null;
+ }
+
/**
* Recursively unwraps the specified query to determine whether it is capable of producing a score
- * that varies across different documents. Returns true if this query is not capable of producing
- * a varying score (i.e., it is a constant score query).
+ * that varies across different documents. Returns the constant score if this query is not capable
+ * of producing a varying score (i.e., it is a constant score query), else returns null.
Review Comment:
This is the main change. I _think_ that so long as we can recognize a constant-score query, we can also determine the particular constant score, and use it to provide scores (and maxScore) without actually sorting/scoring anything.
I'd appreciate other eyes on this, in case there are edge cases that I'm missing. Not sure what the best way is to directly test this ... open to ideas.
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -1710,14 +1710,15 @@ private QueryResult getDocListC(QueryResult qr, QueryCommand cmd) throws IOExcep
// - the sort doesn't contain score
// - we don't want score returned.
- // check if we should try and use the filter cache
+ // check if we should try and use the filter cache for the query itself
final boolean needSort;
- final boolean useFilterCache;
- if ((flags & (GET_SCORES | NO_CHECK_FILTERCACHE)) != 0 || filterCache == null) {
- needSort = true; // this value should be irrelevant when `useFilterCache=false`
- useFilterCache = false;
+ final boolean useFilterCacheForQuery;
+ Float constantScore = 1.0f;
+ if ((flags & (NO_CHECK_FILTERCACHE)) != 0 || filterCache == null) {
Review Comment:
I think we can still optimize here even if scores are requested -- we just need to supply the scores without actually running the query/scoring.
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -1759,13 +1760,16 @@ private QueryResult getDocListC(QueryResult qr, QueryCommand cmd) throws IOExcep
// todo: there could be a sortDocSet that could take a list of
// the filters instead of anding them first...
// perhaps there should be a multi-docset-iterator
+ if ((flags & GET_SCORES) == 0) {
+ constantScore = null;
+ }
Review Comment:
`constantScore ?= null` determines whether scores are added, so if the client hasn't asked for scores, it doesn't matter whether we could provide them ... set `constantScore = null` so that we don't add scores to the response.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-2525348954
This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the dev@solr.apache.org mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution!
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2221:
URL: https://github.com/apache/solr/pull/2221#discussion_r1574045448
##########
solr/benchmark/src/java/org/apache/solr/bench/MiniClusterState.java:
##########
Review Comment:
CC @tflobbe ; you added the setUseHttp1 call 1 year ago.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #2221:
URL: https://github.com/apache/solr/pull/2221#discussion_r1471252293
##########
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java:
##########
@@ -169,6 +160,20 @@ public void prepare(ResponseBuilder rb) throws IOException {
try {
QParser parser = QParser.getParser(rb.getQueryString(), defType, req);
+
+ SortSpec sortSpec = parser.getSortSpec(true);
+ rb.setSortSpec(sortSpec);
+ boolean someDocs = sortSpec == null || sortSpec.getCount() != 0;
+
+ // Set field flags
Review Comment:
`sortSpec.getCount()` is the rows for the query. SortSpec encapsulates the pulling out of sort details and rows for the query. The parser fills in SortSpec so wanted to reuse that logic instead of explicitly trying to parse rows myself.
I have a separate commit coming soon once tests pass to actually make SortSpec and SolrReturnFields a bit smarter about rows=0.
There is some weird intermingling of `GET_SCORES` where it only applies to the `fl` but not to the sort. So if `score` is in the `sort` (which it is by default) then you still calculate scores in a lot of cases.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #2221:
URL: https://github.com/apache/solr/pull/2221#discussion_r1471406109
##########
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java:
##########
@@ -147,15 +147,6 @@ public void prepare(ResponseBuilder rb) throws IOException {
}
}
- // Set field flags
Review Comment:
Removed the comment in [5d842c6](https://github.com/apache/solr/pull/2221/commits/5d842c6bb7683346a5d1b575a9134e72702ddc10)
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-1913487230
I see what's happening. The `queryResultCache`'s `queryResultWindowSize` is forcing the actual number of documents to be raised from 0 to that window (which is 1 in this test config), thus we have to score docs to find the one best doc.
I don't agree with your solution because it confuses what `useFilterCache` fundamentally means (use the filter cache for `q`). The other code path is actually already optimized for `rows==0` and no scores; it should be quite fast!
I think the solution is to not save the results into `queryResultCache` when rows==0; thus the window won't be applied either. But I don't think SolrIndexSearcher should set this, I think it should be in QueryComponent by setting `NO_SET_QCACHE` so that some plugin/customization can easily make this decision for itself. Additionally, QueryComponent is setting `GET_SCORES` if `fl` contains the score but I think this should be further guarded by `rows != 0`.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-1914809682
> (Me) The other code path is actually already optimized for rows==0 and no scores
The method you are attempting to modify calls `getDocListAndSetNC` when `useFilterCache=false`, which is well optimized for this.
> Not sure I agree with that. We should be able to store the result in the queryResultCache so that we don't have to recompute for the same exact query a second time?
Okay; I sympathize. Another alteration to my suggestion, is that `queryResultWindowSize` should be ignored when rows=0 to allow us to run this query as fast as possible, even if this means not caching `queryResultWindowSize` docs -- because we'd rather not compute their scores merely to cache something in hopes it might be used.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #2221:
URL: https://github.com/apache/solr/pull/2221#discussion_r1471253926
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -1682,7 +1684,6 @@ private void getDocListC(QueryResult qr, QueryCommand cmd) throws IOException {
}
}
} else {
- fullSortCount.increment();
Review Comment:
I looked at moving this to a better place inside `getDocListAndSetNC` as well as `getDocListNC` but it gets really complicated figuring out if a fullsortcount should be incremented or not.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-1914972100
> You claim that your approach to this uses the filter cache (for q)
With my change, for the case of `rows=0`, the filtercache docset will be used to compute the result using q.
I don't think I understand your concerns. `getDocSetNC` uses the `filterCache` docset and does not insert the query result into the filter cache. If `getDocSetC` is used, the query result will be inserted into the filter cache - which we don't want.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-1914939631
I addressed the queryResultWindowSize with 326afdd when rows=0.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-2016351884
There is great work here; looking forward to it making 9.6 :-)
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2221:
URL: https://github.com/apache/solr/pull/2221#discussion_r1571706033
##########
solr/benchmark/src/java/org/apache/solr/bench/MiniClusterState.java:
##########
Review Comment:
The field `useHttp1` is initialized with `Boolean.getBoolean("solr.http1")` so what's the point of the changes you are adding here?
##########
solr/core/src/test/org/apache/solr/search/ReturnFieldsTest.java:
##########
@@ -395,6 +395,16 @@ public void testFunkyFieldNames() {
"//arr[@name='#foo_s']/str[.='how now brown cow']");
}
+ @Test
+ public void testRowsZeroNoScore() {
+ ReturnFields rf = new SolrReturnFields(req("fl", "id,score", "rows", "0"));
+ assertFalse(rf.wantsScore());
+ assertFalse(rf.wantsField("score"));
+ assertTrue(rf.wantsField("id"));
Review Comment:
A TODO is fine.
##########
solr/core/src/java/org/apache/solr/search/SolrReturnFields.java:
##########
@@ -532,10 +540,18 @@ private void addField(
okFieldNames.add(key);
// a valid field name
if (SCORE.equals(field)) {
- _wantsScore = true;
+ if (_noRows) {
Review Comment:
At least add a TODO to not even get to this point if noRows is true; we shouldn't even parse "fl". As a reader of this code, it's odd to see special handling on SCORE in particular just because of noRows.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-2070806945
Shall I just merge this for 9.6? I think it's embarrassing we've had this perf issue for so long without it being noticed. match-all-docs is so common, like in ping queries for one.
One test failed, TestPrepRecovery.testLeaderNotResponding but [it's a common flapper](https://ge.apache.org/scans/tests?search.relativeStartTime=P28D&search.rootProjectNames=solr-root&search.timeZoneId=America%2FNew_York&tests.container=org.apache.solr.cloud.TestPrepRecovery)
CC @gus-asf (9.6 RM)
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "gus-asf (via GitHub)" <gi...@apache.org>.
gus-asf commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-2071000102
So code that does something with that maxScore value could hit
null/undefined? That's sort of iffy for back compatibility...
On Mon, Apr 22, 2024 at 4:51 PM David Smiley ***@***.***>
wrote:
> I think the change is "safe" from a test stability standpoint.
>
> There is one change to be mentioned in CHANGES.txt and probably in release
> notes. If rows=0 then maxScore will no longer be returned. The user must
> ask for at least one row if the score is desired.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/solr/pull/2221#issuecomment-2070929291>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ALF2L3KHFCKC2CFZ6OHG3FTY6VZZXAVCNFSM6AAAAABCJOYDZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZQHEZDSMRZGE>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
http://www.needhamsoftware.com (work)
https://a.co/d/b2sZLD9 (my fantasy fiction book)
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-1914900925
> is that queryResultWindowSize should be ignored when rows=0 to allow us to run this query as fast as possible, even if this means not caching queryResultWindowSize docs -- because we'd rather not compute their scores merely to cache something in hopes it might be used.
100% agree with this idea. I'm not sure what is happening with `queryResultWindowSize` docs with my change. In theory we definitely shouldn't be computing them.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-1912952744
> does not need to sort the results or even compute the score
Is there a unit test you can recommend that I might run in a debugger looking at key spots, that would show that a score is being computed wastefully? I can't imagine that there is time wasted sorting (as you claim) because there is nothing to sort.
> This means the filterCache can be more efficient to compute the result and Solr doesn't spend time on unnecessarily sorting and scoring large result sets for it to be thrown away.
This seems contradictory. If the filterCache can compute the result, then wouldn't disabling the filterCache for the query (as I see you do in the PR) *not* use the filter cache then? I'm referring to the query, not the filters (which are processed the same either way).
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #2221:
URL: https://github.com/apache/solr/pull/2221#discussion_r1471409812
##########
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java:
##########
@@ -169,6 +160,20 @@ public void prepare(ResponseBuilder rb) throws IOException {
try {
QParser parser = QParser.getParser(rb.getQueryString(), defType, req);
+
+ SortSpec sortSpec = parser.getSortSpec(true);
+ rb.setSortSpec(sortSpec);
+ boolean someDocs = sortSpec == null || sortSpec.getCount() != 0;
+
+ // Set field flags
Review Comment:
I changed this impl in 23cca04 which makes `SortSpec#includeScore` and `SolrReturnFields` smarter about `rows=0` so they never indicate they need scores. This flows into the `GET_SCORES` logic as well.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #2221:
URL: https://github.com/apache/solr/pull/2221#discussion_r1469808152
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -1818,7 +1820,7 @@ private void getDocListNC(QueryResult qr, QueryCommand cmd) throws IOException {
int[] ids;
float[] scores;
- boolean needScores = (cmd.getFlags() & GET_SCORES) != 0;
+ boolean needScores = ((cmd.getFlags() & GET_SCORES) != 0) && (cmd.getLen() != 0);
Review Comment:
I just haven't gotten back to doing that change yet. I will make another iteration.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-1919850528
I pushed 80f11d5 which has a benchmark for checking a bunch of this. I haven't compared against main yet, but on my machine:
```
Benchmark (flScore) (matchAllDocs) (noRowsQuery) (singleShard) (sortScore) Mode Cnt Score Error Units
SearchOptimizations.query false false false false false thrpt 9 14117.797 ± 667.524 ops/s
SearchOptimizations.query false false false false true thrpt 9 12798.032 ± 664.581 ops/s
SearchOptimizations.query false false false true false thrpt 9 41903.764 ± 3499.993 ops/s
SearchOptimizations.query false false false true true thrpt 9 41875.468 ± 1274.265 ops/s
SearchOptimizations.query false false true false false thrpt 9 14165.919 ± 1275.410 ops/s
SearchOptimizations.query false false true false true thrpt 9 14658.515 ± 711.654 ops/s
SearchOptimizations.query false false true true false thrpt 9 46095.168 ± 2424.954 ops/s
SearchOptimizations.query false false true true true thrpt 9 45676.193 ± 2169.222 ops/s
SearchOptimizations.query false true false false false thrpt 9 12980.789 ± 379.131 ops/s
SearchOptimizations.query false true false false true thrpt 9 10658.171 ± 1223.202 ops/s
SearchOptimizations.query false true false true false thrpt 9 36985.569 ± 1949.741 ops/s
SearchOptimizations.query false true false true true thrpt 9 32990.086 ± 1658.271 ops/s
SearchOptimizations.query false true true false false thrpt 9 15341.764 ± 345.964 ops/s
SearchOptimizations.query false true true false true thrpt 9 14952.378 ± 812.817 ops/s
SearchOptimizations.query false true true true false thrpt 9 51607.454 ± 2001.172 ops/s
SearchOptimizations.query false true true true true thrpt 9 51498.706 ± 1305.913 ops/s
SearchOptimizations.query true false false false false thrpt 9 12525.537 ± 1012.328 ops/s
SearchOptimizations.query true false false false true thrpt 9 12463.681 ± 670.962 ops/s
SearchOptimizations.query true false false true false thrpt 9 40158.078 ± 2073.773 ops/s
SearchOptimizations.query true false false true true thrpt 9 38621.333 ± 2662.662 ops/s
SearchOptimizations.query true false true false false thrpt 9 14370.718 ± 1075.079 ops/s
SearchOptimizations.query true false true false true thrpt 9 14066.580 ± 376.911 ops/s
SearchOptimizations.query true false true true false thrpt 9 45014.503 ± 2190.095 ops/s
SearchOptimizations.query true false true true true thrpt 9 45708.595 ± 978.899 ops/s
SearchOptimizations.query true true false false false thrpt 9 12078.353 ± 221.499 ops/s
SearchOptimizations.query true true false false true thrpt 9 11039.752 ± 132.751 ops/s
SearchOptimizations.query true true false true false thrpt 9 33369.947 ± 448.742 ops/s
SearchOptimizations.query true true false true true thrpt 9 26665.048 ± 740.376 ops/s
SearchOptimizations.query true true true false false thrpt 9 15575.767 ± 398.453 ops/s
SearchOptimizations.query true true true false true thrpt 9 15068.308 ± 1095.587 ops/s
SearchOptimizations.query true true true true false thrpt 9 52237.705 ± 2442.391 ops/s
SearchOptimizations.query true true true true true thrpt 9 52827.441 ± 1061.696 ops/s
```
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-1914981099
> getDocSetNC uses the filterCache docset
Can you elaborate where this is so?
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-1914878120
I made a slight change to improve the variable name I added - c6618d6
> The method you are attempting to modify calls getDocListAndSetNC when useFilterCache=false, which is well optimized for this.
Right but in that case - assuming you are referring to prior to my PR line 1703 / 1708, we don't get the optimizations from SOLR-14765. This means in the case of rows=0, we don't reuse the filter cache dotset and have to recompute it.
With my change, we never get down to `getDocListAndSetNC` for the case of `rows=0` and can build the results right from the filterCache docset.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2221:
URL: https://github.com/apache/solr/pull/2221#discussion_r1471266067
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -1682,7 +1684,6 @@ private void getDocListC(QueryResult qr, QueryCommand cmd) throws IOException {
}
}
} else {
- fullSortCount.increment();
Review Comment:
In those methods, I think it goes first on the `else` side of the condition with comment `// handle zero case...` thus when we need to return a doc list.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-2638303410
This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-3543342822
[Test failure](https://github.com/apache/solr/actions/runs/19439745944/job/55619834113) hasn't reproduced for me locally:
```
./gradlew :solr:core:test --tests "org.apache.solr.search.TestQueryLimits.testAdjustShardRequestLimits" "-Ptests.jvmargs=-XX:TieredStopAtLevel=1 -XX:+UseParallelGC -XX:ActiveProcessorCount=1 -XX:ReservedCodeCacheSize=120m" -Ptests.seed=9EF16791021EE82D -Ptests.timeoutSuite=600000! -Ptests.useSecurityManager=true -Ptests.file.encoding=US-ASCII
```
```
2> 34795 INFO (TEST-TestQueryLimits.testAdjustShardRequestLimits-seed#[9EF16791021EE82D]) [n: c: s: r: x: t:] o.a.s.SolrTestCaseJ4 ###Ending testAdjustShardRequestLimits
> java.lang.AssertionError: should have full results: {
> "responseHeader":{
> "zkConnected":true,
> "partialResultsDetails":"[Shard:http://127.0.0.1:34045/solr/test_shard1_replica_n11]Limits exceeded! ([process] Limit(s) exceeded prior to facet in [delayingSearchComponent, query, facet, facet_module, mlt, highlight, stats, expand, terms, debug]): Query limits: [TimeAllowedLimit:LIMIT EXCEEDED]",
> "partialResults":true,
> "cpuTime":86,
> "status":0,
> "QTime":373},
> "grouped":{
> "val_i":{
> "matches":100,
> "groups":[
...
> {
> "groupValue":"2",
> "doclist":{
> "numFound":0,
> "start":0,
> "numFoundExact":true,
> "docs":[]}}]}}} expected null, but was:<true>
> at __randomizedtesting.SeedInfo.seed([9EF16791021EE82D:316796918915397C]:0)
> at org.junit.Assert.fail(Assert.java:89)
> at org.junit.Assert.failNotNull(Assert.java:756)
> at org.junit.Assert.assertNull(Assert.java:738)
> at org.apache.solr.search.TestQueryLimits.testAdjustShardRequestLimits(TestQueryLimits.java:133)
```
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "gus-asf (via GitHub)" <gi...@apache.org>.
gus-asf commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-2070903192
I guess the question is more a matter of how safe you think this change
is... We all know that there are some flaky tests. The test doesn't sound
related (being admin/recovery related not query related) doesn't matter
much assuming it doesn't reproduce with the seed, or seem to fail more
frequently.
I haven't published the RC yet, though I have already run tests locally and
should re-do that if we are going to add new commits.
-Gus
On Mon, Apr 22, 2024 at 3:44 PM David Smiley ***@***.***>
wrote:
> Shall I just merge this for 9.6? I think it's embarrassing we've had this
> perf issue for so long without it being noticed. match-all-docs is so
> common, like in ping queries for one.
>
> One test failed, TestPrepRecovery.testLeaderNotResponding but it's a
> common flapper
> <https://ge.apache.org/scans/tests?search.relativeStartTime=P28D&search.rootProjectNames=solr-root&search.timeZoneId=America%2FNew_York&tests.container=org.apache.solr.cloud.TestPrepRecovery>
>
> CC @gus-asf <https://github.com/gus-asf> (9.6 RM)
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/solr/pull/2221#issuecomment-2070806945>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ALF2L3IFCJ6F3JJSIZGGKKLY6VR73AVCNFSM6AAAAABCJOYDZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZQHAYDMOJUGU>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
http://www.needhamsoftware.com (work)
https://a.co/d/b2sZLD9 (my fantasy fiction book)
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-2071123760
**I'll not push for this in 9.6.** I confused it in part for the important work that made it to 9.1 [SOLR-17131](https://issues.apache.org/jira/browse/SOLR-14765).
Kevin, I suppose an easy work-around for users is to explicitly set "fl" to something that does *not* include score. If fl doesn't include scores, then maxScore won't be included either.
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #2221:
URL: https://github.com/apache/solr/pull/2221#discussion_r1471411181
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -1818,7 +1820,7 @@ private void getDocListNC(QueryResult qr, QueryCommand cmd) throws IOException {
int[] ids;
float[] scores;
- boolean needScores = (cmd.getFlags() & GET_SCORES) != 0;
+ boolean needScores = ((cmd.getFlags() & GET_SCORES) != 0) && (cmd.getLen() != 0);
Review Comment:
Continuing here https://github.com/apache/solr/pull/2221#discussion_r1471409812
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2221:
URL: https://github.com/apache/solr/pull/2221#discussion_r1469804423
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -1818,7 +1820,7 @@ private void getDocListNC(QueryResult qr, QueryCommand cmd) throws IOException {
int[] ids;
float[] scores;
- boolean needScores = (cmd.getFlags() & GET_SCORES) != 0;
+ boolean needScores = ((cmd.getFlags() & GET_SCORES) != 0) && (cmd.getLen() != 0);
Review Comment:
Arguably, the caller shouldn't be applying GET_SCORES in the first place -- which is QueryComponent. I suggested this idea yesterday; do you not like it?
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-1915662767
Updated this based on talking w/ @dsmiley. the `useFilterCache` part I was confused on that it always applies to `q` so my change didn't make sense. I pushed a change that made it clear what `useFilterCache` is always used for `q`.
I also made the change to `QueryComponent` to make sure `scores` aren't calculated if `rows=0`. I think there is more here potentially.
TODO:
* Look at adding some more tests
* Try adding a benchmark for this
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-3238743734
This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the dev@solr.apache.org mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution!
--
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
Re: [PR] SOLR-17131: Optimize rows=0 since score/sort isn't necessary [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2221:
URL: https://github.com/apache/solr/pull/2221#issuecomment-3459140119
This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again.
--
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