SOLR-16162: FilterQuery should implement DocSetProducer#814
SOLR-16162: FilterQuery should implement DocSetProducer#814magibney wants to merge 1 commit intoapache:mainfrom
Conversation
that's what it's doing anyway; but more important practically: this avoids the cloning of the backing query's DocSet (and the creation of superfluous DocSets).
|
As described in the jira and here, the description and fix seems reasonable to me. I just don't know the code well enough in this part to say if this should go in. |
| @Override | ||
| public DocSet createDocSet(SolrIndexSearcher searcher) throws IOException { | ||
| /* | ||
| There is a substantial practical reason for FilterQuery to implement DocSetProducer: |
There was a problem hiding this comment.
Well, you know, it might not be a good idea to open this can of worms.
There's org.apache.solr.search.SolrIndexSearcher#getDocSetNC, where NC implies NOcache, I suppose. That method hits org.apache.solr.search.DocSetUtil#createDocSet(), which checks for DocSetProducer. I'm not sure if reaching filtercache here is what getDocSetNC is plan to do.
There was a problem hiding this comment.
Yes, NC=no-cache; it's a little counterintuitive on the surface, but indeed that's the fundamental purpose of FilterQuery: it is used to consult the cache in contexts that otherwise would not cache, by reaching out to the filterCache "behind the scenes".
Often we would in fact expect to arrive at FilterQuery.createDocSet() via SolrIndexSearcher.getDocSetNC(), because FilterQuery.getCacche() now returns false. But what that really signifies is "don't cache the top level" ... "(because I handle reaching out the filterCache internally)". #624 fixed the fact that FilterQuery would previously have been cached twice.
This PR doesn't actually change whether FilterQuery hits the filterCache ... it always does, by definition. It just changes whether, having hit the cache, it forces the caller to build a clone of the cached DocSet (as it currently does), or supports a way to directly return the cached DocSet instance.
|
Pending feedback/objections, I plan to merge this in the coming week. |
|
ha, thanks for noticing this one. I guess I missed the "coming week" window by more than a year 😅 . I'll take care of this shortly. |
|
This PR had no visible activity in the past 60 days, labeling it as stale. Any new activity will remove the stale label. To attract more reviewers, please tag someone or notify the dev@solr.apache.org mailing list. Thank you for your contribution! |
|
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. |
|
@magibney did you have doubts on this one? |
|
No doubts at all, looks like I have some following up to do! |
github-actions
bot
added
cat:search
and removed
closed-stale
|
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 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. |
4 participants
See: SOLR-16162
"producing a DocSet" is what FilterQuery is actually doing anyway; but more important practically: this avoids the cloning of the backing query's DocSet (and the creation of superfluous DocSets).