SOLR-17037 - Clone LTRScoringQuery on rewriting to preserve queryResultCache key #2022
Conversation
softwaredoug
changed the title
|
Looks like I broke some interleaving test, looking into that... |
|
Fixed the interleaving tests :) |
.gitignore
Outdated
| @@ -43,3 +43,5 @@ __pycache__ | |||
| # Ignore gradle wrapper | |||
| gradle/wrapper/gradle-wrapper.jar | |||
| .gradletasknamecache | |||
|
|
|||
| .tool-versions | |||
There was a problem hiding this comment.
anything to do with the fix? is this intended?
There was a problem hiding this comment.
Oh you're right, this is my asdf environment. Will remove it.
|
Thanks @softwaredoug for the contribution! |
|
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! |
|
|
||
| private boolean equalsTo(LTRInterleavingScoringQuery other) { | ||
| boolean superSame = (super.equalsTo(other) && other.equalsTo((LTRScoringQuery) this)); | ||
| if (this.getPickedInterleavingDocIds() != null && other.getPickedInterleavingDocIds() != null) { |
There was a problem hiding this comment.
minor/style/subjective
| if (this.getPickedInterleavingDocIds() != null && other.getPickedInterleavingDocIds() != null) { | |
| if (this.pickedInterleavingDocIds != null && other.getPickedInterleavingDocIds() != null) { |
| getScoringModel(), getExternalFeatureInfo(), getThreadModule()); | ||
| cloned.setOriginalQuery(getOriginalQuery()); | ||
| cloned.setFeatureLogger(getFeatureLogger()); | ||
| cloned.setRequest(getRequest()); | ||
| cloned.setPickedInterleavingDocIds(getPickedInterleavingDocIds()); |
There was a problem hiding this comment.
wondering if the efi map and the pickedDocIds set should be cloned too?
|
|
||
| String resp = | ||
| restTestHarness.adminQuery( | ||
| "/admin/metrics?group=core&&prefix=CACHE.searcher.queryResultCache"); |
There was a problem hiding this comment.
| "/admin/metrics?group=core&&prefix=CACHE.searcher.queryResultCache"); | |
| "/admin/metrics?group=core&prefix=CACHE.searcher.queryResultCache"); |
| "/admin/metrics?group=core&&prefix=CACHE.searcher.queryResultCache"); | ||
| String error = | ||
| JSONTestUtil.match( | ||
| resp, "/metrics/solr.core.collection1/CACHE.searcher.queryResultCache/hits==1"); |
There was a problem hiding this comment.
perhaps at the start of the test we could also test that the cache is empty hits==0 to start with?
| query.add("fl", "*, score"); | ||
| query.add("rows", "4"); | ||
| query.add("fv", "true"); | ||
| query.add("rq", "{!ltr model=6029760550880411648 reRankDocs=3}"); |
There was a problem hiding this comment.
wondering how practical it would be to have test coverage for the interleaving case too.
https://issues.apache.org/jira/browse/SOLR-17037
Description
Clone the LTRScoringQuery during rewriting so it benefits from the cache
Solution
Clone the LTRScoringQuery during rewriting so it benefits from the cache
Tests
See added test that repros, which is now fixed.
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.