cpoerschke commented on a change in pull request #123:
URL: https://github.com/apache/solr/pull/123#discussion_r636516706
##########
File path:
solr/contrib/ltr/src/test/org/apache/solr/ltr/TestLTRReRankingPipeline.java
##########
@@ -41,25 +37,33 @@
import org.apache.lucene.search.ScoreMode;
import org.apache.lucene.search.TermQuery;
import org.apache.lucene.search.TopDocs;
-import org.apache.lucene.store.Directory;
-import org.apache.solr.SolrTestCase;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.core.SolrResourceLoader;
import org.apache.solr.ltr.feature.Feature;
import org.apache.solr.ltr.feature.FieldValueFeature;
import org.apache.solr.ltr.model.LTRScoringModel;
import org.apache.solr.ltr.model.TestLinearModel;
import org.apache.solr.ltr.norm.IdentityNormalizer;
import org.apache.solr.ltr.norm.Normalizer;
+import org.apache.solr.request.LocalSolrQueryRequest;
+import org.apache.solr.request.SolrQueryRequest;
+import org.junit.BeforeClass;
import org.junit.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Review comment:
Thanks for this! Yes, once the "where is the pure docValues connection?"
question is gone and the mental focus is on "code comprehension and
readability" it is clear that this is a refactoring change that is indeed very
beneficial.
And when considering if the refactoring changes inadvertently changes any
existing behaviour the `@AwaitsFix(bugUrl =
"https://issues.apache.org/jira/browse/SOLR-11134";)` caught my eye and I think
I managed to diagnose this long-standing issue:
* the problem was that `new FloatDocValuesField("final-score", ...)`
resulted in a `stored=false` field (which was not very apparent) and its
`schema.xml` replacement named `finalScore` is `stored=true` i.e. immediate
readability and comprehension benefit there.
* then next the `testDifferentTopN` turned out to have an incorrect
expectation w.r.t. what the score should be (addition of an alternative
`TestLinearModel.makeFeatureWeights` variant tries to make that clearer).
* and lastly once the `testDifferentTopN` test passed it interacted with the
`testRescorer` test i.e. if the former ran first then the latter would see not
only its own 2 documents but also the documents added by the former test, this
is easily fixed by everyone deleting all documents before adding their
documents.
> ... So I would like to keep it in this PR to make it more accessible and
easier to built up upon in the future.
I agree we should keep these very beneficial `TestLTRReRankingPipeline`
changes.
If (and that is an if) the SOLR-11134 fix (or something along those lines)
makes sense, then how would you feel about us still removing the
`TestLTRReRankingPipeline` change from this PR and instead to have it in a
separate PR and to "book it" only under SOLR-11134 or jointly under SOLR-11134
and SOLR-12697 tickets?
--
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.
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
