cpoerschke commented on a change in pull request #123:
URL: https://github.com/apache/solr/pull/123#discussion_r635980115
##########
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:
After multiple attempts I'm still struggling to understand and
appreciate how the `TestLTRReRankingPipeline` changes relate to and fit in with
the other changes. @tomglk would you have an extra perspective on this perhaps?
I'm speculating that the changes were required when the
`SolrDocumentFetcher` implementation was used but now with the
`TestFieldValueFeature` changes the picture has changed.
_If_ the `TestLTRReRankingPipeline` changes are no longer required and
relatively unrelated I would favour `git checkout origin/main --
TestLTRReRankingPipeline.java` removing them from the pull request scope. What
do you think?
##########
File path:
solr/contrib/ltr/src/test/org/apache/solr/ltr/feature/TestFieldValueFeature.java
##########
@@ -213,4 +349,69 @@ public void testParamsToMap() throws Exception {
doTestParamsToMap(FieldValueFeature.class.getName(), params);
}
+ @Test
+ public void testThatStringValuesAreCorrectlyParsed() throws Exception {
+ final String[][] inputsAndTests = {
+ new String[]{"T", "/response/docs/[0]/=={'[fv]':'" +
+ FeatureLoggerTestUtils.toFeatureVector("dvStrNumField", "1.0")+"'}"},
+ new String[]{"F", "/response/docs/[0]/=={'[fv]':'" +
+ FeatureLoggerTestUtils.toFeatureVector("dvStrNumField", "0.0")+"'}"},
+ new String[]{"-7324.427", "/response/docs/[0]/=={'[fv]':'" +
+ FeatureLoggerTestUtils.toFeatureVector("dvStrNumField",
"-7324.427")+"'}"},
+ new String[]{"532", "/response/docs/[0]/=={'[fv]':'" +
+ FeatureLoggerTestUtils.toFeatureVector("dvStrNumField",
"532.0")+"'}"},
+ new String[]{"notanumber",
"/error/msg/=='org.apache.solr.ltr.feature.FeatureException: " +
+ "Cannot parse value notanumber of field dvStrNumField to float.'"}
+ };
+
+ final String fstore = "testThatStringValuesAreCorrectlyParsed";
+ loadFeature("dvStrNumField", FieldValueFeature.class.getName(), fstore,
+ "{\"field\":\"" + "dvStrNumField" + "\"}");
+ loadModel("dvStrNumField-model", LinearModel.class.getName(),
+ new String[]{"dvStrNumField"}, fstore, "{\"weights\":{\"" +
"dvStrNumField" + "\":1.0}}");
+
+ for (String[] inputAndTest : inputsAndTests) {
+ assertU(adoc("id", "21", "dvStrNumField", inputAndTest[0]));
+ assertU(commit());
+
+ SolrQuery query = new SolrQuery();
+ query.setQuery("id:21");
+ query.add("rq", "{!ltr model=" + "dvStrNumField" + "-model
reRankDocs=4}");
+ query.add("fl", "[fv]");
+
+ assertJQ("/query" + query.toQueryString(), inputAndTest[1]);
+ }
+ }
+
+ /**
+ * This class is used to track which specific FieldValueFeature is used so
that we can test, whether the
+ * fallback mechanism works correctly.
+ */
+ public static class ObservingFieldValueFeature extends FieldValueFeature {
Review comment:
The selection of test coverage using this new
`ObservingFieldValueFeature` and its observing the scorer type is very
comprehensive and elegant, thanks for adding it!
##########
File path:
solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/FieldValueFeature.java
##########
@@ -23,24 +23,34 @@
import java.util.Set;
import org.apache.lucene.document.Document;
+import org.apache.lucene.index.DocValues;
+import org.apache.lucene.index.DocValuesType;
+import org.apache.lucene.index.FieldInfo;
import org.apache.lucene.index.IndexableField;
import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.index.NumericDocValues;
+import org.apache.lucene.index.SortedDocValues;
import org.apache.lucene.search.DocIdSetIterator;
import org.apache.lucene.search.IndexSearcher;
import org.apache.lucene.search.Query;
+import org.apache.lucene.util.BytesRef;
import org.apache.solr.request.SolrQueryRequest;
import org.apache.solr.schema.BoolField;
+import org.apache.solr.schema.NumberType;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrIndexSearcher;
/**
* This feature returns the value of a field in the current document
* Example configuration:
* <pre>{
- "name": "rawHits",
- "class": "org.apache.solr.ltr.feature.FieldValueFeature",
- "params": {
- "field": "hits"
- }
-}</pre>
+ * "name": "rawHits",
+ * "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ * "params": {
+ * "field": "hits",
+ * "defaultValue": -1
+ * }
+ * }</pre>
*/
Review comment:
Doing the _"a.k.a. pure DocValues"_ edit on the `solr/CHANGES.txt` made
me wonder, would it be helpful to have more detail here w.r.t. (stored and
docValues) field type attributes, what do you think?
--
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
