You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/06/05 15:28:52 UTC
[GitHub] [solr] tomglk opened a new pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
tomglk opened a new pull request #166:
URL: https://github.com/apache/solr/pull/166
# Description
This PR refers to the issue [SOLR-15450](https://issues.apache.org/jira/browse/SOLR-15450).
It introduces a new `PrefetchingFieldValueFeature` which can be used for stored fields without docValues.
This feature prefetches all stored fields that itself and other `PrefetchingFieldValueFeatures` use so that they are available in the cache.
This improves the performance because we only need to access the index during the first loading of all fields that should be prefetched instead of reading the fields used by the features separately.
# Solution
The `PrefetchingFieldValueFeature` extends the `FieldValueFeature`.
It only is used to get the values for fields which are stored and have no docValues. For all other fields it delegates the work to the `FieldValueFeature` because in such cases we want to make use of the docValues.
The `PrefetchingFieldValueFeature` keeps a list of all fields that should be fetched and uses these fields when requesting the document from the docFetcher.
```java
private Set<String> prefetchFields;
final Document document = docFetcher.doc(context.docBase + itr.docID(), prefetchFields);
```
The `prefetchFields` are updated after all features were loaded. To do so, it keeps track of the feature stores that were updated, iterates over all `PrefetchingFieldValueFeatures` per store, collects the fields and then updates them.
```java
Set<String> updatedFeatureStores = new HashSet<>();
preparePrefetchingFieldValueFeatures(updatedFeatureStores);
private void preparePrefetchingFieldValueFeatures(Set<String> updatedFeatureStores) {
for(String featureStoreName: updatedFeatureStores) {
final Set<PrefetchingFieldValueFeature> prefetchingFieldValueFeatures = getFeatureStore(featureStoreName)
.getFeatures().stream()
.filter(feature -> feature instanceof PrefetchingFieldValueFeature)
.map(feature -> ((PrefetchingFieldValueFeature) feature))
.collect(Collectors.toSet());
final Set<String> prefetchFields = new HashSet<>();
for (PrefetchingFieldValueFeature feature : prefetchingFieldValueFeatures) {
prefetchFields.add(feature.getField());
}
for (PrefetchingFieldValueFeature feature : prefetchingFieldValueFeatures) {
feature.setPrefetchFields(prefetchFields);
}
}
}
```
# Tests
**TestPrefetchingFieldValueFeature** tests that
* the `prefetchFields` are only updated after all features were loaded and the basic setting works
* adding fields to an existing feature works
**TestLTROnSolrCloudWithPrefetchingFieldValueFeature** tests that
* the basic reRanking works
* the `PrefetchingFieldValueFeatureScorers` correctly prefetch the fields (using assertions on the *hasBeenLoaded*-property of LazyFields and checking the StoredFields of the Document)
# Checklist
Please review the following and check all that apply:
- [X] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) 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 test`.
- [ ] I have run `./gradlew check`.
- [X] I have run the tests in `org.apache.solr.ltr.test`.
- [X] I have added tests for my changes.
- [X] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide) **NOTE:** very sparsely, I was not sure how much detail I should provide
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r654630147
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/LTRScoringQuery.java
##########
@@ -223,6 +223,7 @@ public ModelWeight createWeight(IndexSearcher searcher, ScoreMode scoreMode, flo
else{
createWeightsParallel(searcher, scoreMode.needsScores(), featureWeights, features);
}
+ // TODO: potential prefetchFields computation site?
Review comment:
> ... My only concern is the re-computation per query. ...
Excellent point! And it got me thinking about additional alternative approaches and so here's another one for the list, one _without_ the per-query re-computation but _with_ the per-query precision.
LTRScoringModel (also) computes joint `prefetchFields` approach:
* (+) the feature store content provides visibility into the feature store's overall prefetch fields set
* (+) the used prefetch fields used is 'precise' i.e. all features in the store vs. all features used by the model
* (+) thrown exceptions can provide visibility into the 'precise' prefetch fields used
* (+/-) concurrency needs to be considered when updating the prefetch fields of actively used features
partial code illustration: https://github.com/cpoerschke/solr/commit/de4e32fa21b9daba34b6b297e770e1c2c29362b0
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r655924437
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/LTRScoringQuery.java
##########
@@ -223,6 +223,7 @@ public ModelWeight createWeight(IndexSearcher searcher, ScoreMode scoreMode, flo
else{
createWeightsParallel(searcher, scoreMode.needsScores(), featureWeights, features);
}
+ // TODO: potential prefetchFields computation site?
Review comment:
Hi @cpoerschke ,
from the approaches that we discussed until now, I like this one best.
I committed your implementation in 7ac6e35.
(Also, I did a few small changes to fix the tests and some renaming because the terms `frozen` and `freeze` made it a bit hard to understand.)
Tests for that are missing. Yesterday evening felt a bit too late to start with that.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r650443475
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+
+ public void setField(String field) {
+ this.field = field;
+ }
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @Override
+ public LinkedHashMap<String,Object> paramsToMap() {
+ final LinkedHashMap<String,Object> params = defaultParamsToMap();
+ params.put("field", field);
+ return params;
+ }
+
+ @Override
+ protected void validate() throws FeatureException {
+ if (field == null || field.isEmpty()) {
+ throw new FeatureException(getClass().getSimpleName()+
+ ": field must be provided");
+ }
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SchemaField schemaField;
+ private final SolrDocumentFetcher docFetcher;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+ if (searcher instanceof SolrIndexSearcher) {
+ schemaField = ((SolrIndexSearcher) searcher).getSchema().getFieldOrNull(field);
+ } else {
+ schemaField = null;
+ }
+ this.docFetcher = request.getSearcher().getDocFetcher();
+ }
+
+ /**
+ * Return a FeatureScorer that work with stored fields and makes use of the cache if the configured field is stored
+ * and has no docValues.
+ * Otherwise, delegate the work to the FieldValueFeature.
+ *
+ * @param context the segment this FeatureScorer is working with
+ * @return FeatureScorer for the current segment and field
+ * @throws IOException as defined by abstract class Feature
+ */
+ @Override
+ public FeatureScorer scorer(LeafReaderContext context) throws IOException {
+ if (schemaField != null && !schemaField.stored() && schemaField.hasDocValues()) {
+ return super.scorer(context);
+ }
+ return new PrefetchingFieldValueFeatureScorer(this, context,
+ DocIdSetIterator.all(DocIdSetIterator.NO_MORE_DOCS));
+ }
+
+ /**
+ * A FeatureScorer that reads the stored value for a field
+ * docFetcher does not request a single field but all the prefetchFields to improve performance through caching
+ */
+ public class PrefetchingFieldValueFeatureScorer extends FieldValueFeatureScorer {
+
+ public PrefetchingFieldValueFeatureScorer(FeatureWeight weight,
+ LeafReaderContext context, DocIdSetIterator itr) {
+ super(weight, context, itr);
+ }
+
+ @Override
+ public float score() throws IOException {
+ try {
+ final Document document = docFetcher.doc(context.docBase + itr.docID(), prefetchFields);
+
+ return super.parseStoredFieldValue(document.getField(field));
+ } catch (final IOException e) {
+ throw new FeatureException(
+ e.toString() + ": " +
+ "Unable to extract feature for "
+ + name, e);
Review comment:
Hi @tomglk! I really like your idea of including the prefetchFields in the stored representation that one gets when downloading from the feature store or when inspecting in ZooKeeper.
In https://github.com/cpoerschke/solr/commit/df2864a95e1a83ee446781042fe119981551a932 I explored splitting PrefetchingFieldValueFeature.prefetchFields into three (configured, computed, effective) and 'self' not being part of the params-to-map prefetch fields. But I am not yet 100% convinced by that splitting approach and the commit also includes some small others bits mixed in. What do you think?
On your (not) overriding `equals` point, that's a good one. I think it's okay to map `null` to `empty set` in `paramsToMap` like you did.
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.PrefetchingFieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+ // can be used for debugging to only fetch the field this features uses
+ public static final String DISABLE_PREFETCHING_FIELD_VALUE_FEATURE = "disablePrefetchingFieldValueFeature";
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SolrDocumentFetcher docFetcher;
+ private final Boolean disablePrefetching;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+
+ disablePrefetching = request.getParams().getBool(DISABLE_PREFETCHING_FIELD_VALUE_FEATURE, false);
Review comment:
Hi @tomglk,
All good points here re: the benefits of a disable prefetching mechanism and the disadvantages of try-catch-retry logic. Reading your arguments here (thanks for the details and structure!) and the configured/computed/effective work on https://github.com/cpoerschke/solr/commit/df2864a95e1a83ee446781042fe119981551a932 generated some more insights and ideas, I will try to organise them into logical chunks below but in short: I agree and can see now that and how a prefetch disabling mechanism will be useful.
> ... Your intention is something like this (in a subclass of PFVF) ...
Yes, "in a subclass of PFVF" was the intention and then the details of the subclass could suit the particular use case i.e. could be boolean or could be non-boolean in some cases. But (spoiler alert) re: the next 'logical chunk' updates below, it seems to me now that there is a general (non-subclass) case for a non-boolean parameter.
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.PrefetchingFieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+ // can be used for debugging to only fetch the field this features uses
+ public static final String DISABLE_PREFETCHING_FIELD_VALUE_FEATURE = "disablePrefetchingFieldValueFeature";
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SolrDocumentFetcher docFetcher;
+ private final Boolean disablePrefetching;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+
+ disablePrefetching = request.getParams().getBool(DISABLE_PREFETCHING_FIELD_VALUE_FEATURE, false);
Review comment:
Here's the 'logical chunks' mentioned above:
* index corruption is (hopefully) a rare source of errors, and in such a scenario rethrowing the exception with the details would seem most useful.
* missing-ness is a probably more likely potential source of errors: how would the document fetcher behave if one of the fields does not exist in the schema or it exists but is not stored and not docValues? or if the field exists but is optional and the document being scored has no value for the field? perhaps the test coverage already has the answers to this, i haven't looked, just thinking out aloud here.
* next then moving on to potential sources of missing-ness:
* `[effective]prefetchFields` is computed from the PrefetchingFieldValueFeature itself and its fellow PrefetchingFieldValueFeature in the same feature store
* _if_ a feature is removed from the store and _if_ that did not propagate the new joint prefetchFields to the features remaining in the store then that would potentially be inefficient i.e. a field is unnecessarily prefetched but it would not be a problem as such since the prefetch is for the underlying field rather than the removed feature
* aside: from what i recall we don't support removal of features from a store
* aside: re-computation of the joint prefetchFields happens when a feature is uploaded to a feature store, the upload could be for an established actively used feature store. concurrent read-use of `PrefetchingFieldValueFeature.prefetchFields` is not a problem but one thread reading and another updating seems potentially problematic
* when a feature is added to the store, at that point missing-ness could arise e.g. due to a typo in the field name or due to destination mistakes i.e. upload to storeA for collectionA was intended but upload to storeB for collectionB happened
* if/since removal of features is not supported there is no obviously quick remedy here (probably deleting the feature store and re-uploading it with only the "good" features should work though I'm not entirely sure how any models using the store's "good" features would behave in that scenario, it probably would work whilst the models are loaded in memory but would not work across a reload or restart)
* if a PrefetchingFieldValueFeature with a field named "bad1" was uploaded then operationally a precise "oops, sorry, please ignore that 'bad1' field" mechanism would be more targetted than a broader "ouch, let's temporarily turn off all prefetching" mechanism
* the exception thrown could guide towards a (say) `ltr.PrefetchingFieldValueFeature.exclusions` string parameter (instead of the `disablePrefetchingFieldValueFeature` boolean parameter) and iteratively the user can then reduce `ltr.PrefetchingFieldValueFeature.exclusions=bad1,bar,foo,good1` down to `ltr.PrefetchingFieldValueFeature.exclusions=bad1` meaning that the features using the `bar,foo,good1` fields continue to benefit from prefetching
* aside: when shrinking the `[effective]prefetchFields` based on `ltr.PrefetchingFieldValueFeature.exclusions=bad1,bar,foo,good1` the implementation should not exclude itself e.g. features using the `good1` field must disregard the `good1` element on the `ltr.PrefetchingFieldValueFeature.exclusions` list
* Might `ltr.PrefetchingFieldValueFeature.exclusions=aaa,bbb,ccc,ddd,eee,fff` have other uses too?
* The effects of a feature store update could be somewhere in between 'good' and 'bad' e.g.
* `model1` uses features `aaa,bbb,ccc`
* we add `fl=[fv]` to extract feature values for old and new features in order to build `model2`
* `fl=[fv]` was previously fine but with the new features latency is more problematic
* we complete feature extraction and stop using `fl=[fv]`
* `model1` does not use features `ddd,eee,fff` but since all features are in the same store `model1`'s features will prefetch fields for `ddd,eee,fff` even if `fl=[fv]` is not requested
* `ltr.PrefetchingFieldValueFeature.exclusions=ddd,eee,fff` can eliminate the prefetch
* we deploy `model2` and if it only uses the `ddd,eee,fff` features then `ltr.PrefetchingFieldValueFeature.exclusions=aaa,bbb,ccc` can eliminate unnecessary prefetching of the old features' fields
(I don't know if practically additional prefetch fields could work out as costly enough to justify the operational efforts of the outlined `ltr.PrefetchingFieldValueFeature.exclusions` parameter use but in principle the parameter could be used in that way.)
----
Bit of a long write-up that, sorry, did it sort of get the ideas across or maybe some bits could be clarified somehow, what do you think?
Oh and of course `ltr.PrefetchingFieldValueFeature.exclusions` is just a tentative name i.e. we can call it something else also and `ltr.PrefetchingFieldValueFeature.exclusions=*` equivalent to `disablePrefetchingFieldValueFeature=true` may also be convenient.
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/LTRScoringQuery.java
##########
@@ -223,6 +223,7 @@ public ModelWeight createWeight(IndexSearcher searcher, ScoreMode scoreMode, flo
else{
createWeightsParallel(searcher, scoreMode.needsScores(), featureWeights, features);
}
+ // TODO: potential prefetchFields computation site?
Review comment:
Hi @tomglk. I had another idea! This pull request is fun :-)
The idea unexpectedly appeared triggered by the earlier insights:
> ... re-computation of the joint prefetchFields ... could be for an established actively used feature store. concurrent ...
> ... Might `ltr.PrefetchingFieldValueFeature.exclusions=aaa,bbb,ccc,ddd,eee,fff` have other uses too? ...
> ... `disablePrefetchingFieldValueFeature` ... `ltr.PrefetchingFieldValueFeature.exclusions` ...
ManagedFeatureStore computes the joint `prefetchFields` approach:
* (+) the feature store content provides visibility into the feature store's overall prefetch fields set
* (+/-) concurrency needs to be considered when updating the prefetch fields of actively used features
* (-) the prefetch fields used may be 'too wide' i.e. all features in the store vs. all features used by the model
LTRScoringQuery computes the joint `prefetchFields` approach -- the new idea:
* (-) or (+/-) less or different visibility into the overall prefetch fields set (no visibility via the feature store but still visibility when an exception is thrown)
* (+) no concurrency concerns since the joint prefetch fields set is propagated to not-yet-used PrefetchingFieldValueFeatureWeight objects
* (+) the used prefetch fields used is 'precise' i.e. all features in the store vs. all features used by the model
pseudo code:
```
for (FeatureWeight fw : featureWeights) {
if (fw instanceof PrefetchingFieldValueFeatureWeight) {
PrefetchingFieldValueFeatureWeight pfvfw = (PrefetchingFieldValueFeatureWeight)fw;
prefetchingFeatureWeights.add(pfvfw);
prefetchFields.add(pfvfw.getField());
}
}
for (PrefetchingFieldValueFeatureWeight pfvfw : prefetchingFeatureWeights) {
prefetchFields.addPrefetchFields(prefetchFields);
}
```
What do you think?
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/LTRScoringQuery.java
##########
@@ -223,6 +223,7 @@ public ModelWeight createWeight(IndexSearcher searcher, ScoreMode scoreMode, flo
else{
createWeightsParallel(searcher, scoreMode.needsScores(), featureWeights, features);
}
+ // TODO: potential prefetchFields computation site?
Review comment:
Hi @tomglk. I had another idea! This pull request is fun :-)
The idea unexpectedly appeared triggered by the earlier insights:
> ... re-computation of the joint prefetchFields ... could be for an established actively used feature store. concurrent ...
> ... Might `ltr.PrefetchingFieldValueFeature.exclusions=aaa,bbb,ccc,ddd,eee,fff` have other uses too? ...
> ... `disablePrefetchingFieldValueFeature` ... `ltr.PrefetchingFieldValueFeature.exclusions` ...
ManagedFeatureStore computes the joint `prefetchFields` approach:
* (+) the feature store content provides visibility into the feature store's overall prefetch fields set
* (+/-) concurrency needs to be considered when updating the prefetch fields of actively used features
* (-) the prefetch fields used may be 'too wide' i.e. all features in the store vs. all features used by the model
LTRScoringQuery computes the joint `prefetchFields` approach -- the new idea:
* (-) or (+/-) less or different visibility into the overall prefetch fields set (no visibility via the feature store but still visibility when an exception is thrown)
* (+) no concurrency concerns since the joint prefetch fields set is propagated to not-yet-used PrefetchingFieldValueFeatureWeight objects
* (+) the used prefetch fields used is 'precise' i.e. all features in the store vs. all features used by the model
pseudo code:
```
for (FeatureWeight fw : featureWeights) {
if (fw instanceof PrefetchingFieldValueFeatureWeight) {
PrefetchingFieldValueFeatureWeight pfvfw = (PrefetchingFieldValueFeatureWeight)fw;
prefetchingFeatureWeights.add(pfvfw);
prefetchFields.add(pfvfw.getField());
}
}
for (PrefetchingFieldValueFeatureWeight pfvfw : prefetchingFeatureWeights) {
pfvfw.addPrefetchFields(prefetchFields);
}
```
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r646156988
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/FieldValueFeature.java
##########
@@ -166,33 +166,36 @@ public float score() throws IOException {
try {
final Document document = context.reader().document(itr.docID(),
fieldAsSet);
- final IndexableField indexableField = document.getField(field);
- if (indexableField == null) {
- return getDefaultValue();
- }
- final Number number = indexableField.numericValue();
- if (number != null) {
- return number.floatValue();
- } else {
- final String string = indexableField.stringValue();
- if (string.length() == 1) {
- // boolean values in the index are encoded with the
- // a single char contained in TRUE_TOKEN or FALSE_TOKEN
- // (see BoolField)
- if (string.charAt(0) == BoolField.TRUE_TOKEN[0]) {
- return 1;
- }
- if (string.charAt(0) == BoolField.FALSE_TOKEN[0]) {
- return 0;
- }
- }
- }
+ return parseStoredFieldValue(document.getField(field));
} catch (final IOException e) {
throw new FeatureException(
e.toString() + ": " +
"Unable to extract feature for "
+ name, e);
}
+ }
+
+ float parseStoredFieldValue(IndexableField indexableField) throws IOException {
Review comment:
ab81d8b
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r648547301
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+
+ public void setField(String field) {
+ this.field = field;
+ }
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @Override
+ public LinkedHashMap<String,Object> paramsToMap() {
+ final LinkedHashMap<String,Object> params = defaultParamsToMap();
+ params.put("field", field);
+ return params;
+ }
+
+ @Override
+ protected void validate() throws FeatureException {
+ if (field == null || field.isEmpty()) {
+ throw new FeatureException(getClass().getSimpleName()+
+ ": field must be provided");
+ }
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SchemaField schemaField;
+ private final SolrDocumentFetcher docFetcher;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+ if (searcher instanceof SolrIndexSearcher) {
+ schemaField = ((SolrIndexSearcher) searcher).getSchema().getFieldOrNull(field);
+ } else {
+ schemaField = null;
+ }
+ this.docFetcher = request.getSearcher().getDocFetcher();
+ }
+
+ /**
+ * Return a FeatureScorer that work with stored fields and makes use of the cache if the configured field is stored
+ * and has no docValues.
+ * Otherwise, delegate the work to the FieldValueFeature.
+ *
+ * @param context the segment this FeatureScorer is working with
+ * @return FeatureScorer for the current segment and field
+ * @throws IOException as defined by abstract class Feature
+ */
+ @Override
+ public FeatureScorer scorer(LeafReaderContext context) throws IOException {
+ if (schemaField != null && !schemaField.stored() && schemaField.hasDocValues()) {
+ return super.scorer(context);
+ }
+ return new PrefetchingFieldValueFeatureScorer(this, context,
+ DocIdSetIterator.all(DocIdSetIterator.NO_MORE_DOCS));
+ }
+
+ /**
+ * A FeatureScorer that reads the stored value for a field
+ * docFetcher does not request a single field but all the prefetchFields to improve performance through caching
+ */
+ public class PrefetchingFieldValueFeatureScorer extends FieldValueFeatureScorer {
+
+ public PrefetchingFieldValueFeatureScorer(FeatureWeight weight,
+ LeafReaderContext context, DocIdSetIterator itr) {
+ super(weight, context, itr);
+ }
+
+ @Override
+ public float score() throws IOException {
+ try {
+ final Document document = docFetcher.doc(context.docBase + itr.docID(), prefetchFields);
+
+ return super.parseStoredFieldValue(document.getField(field));
+ } catch (final IOException e) {
+ throw new FeatureException(
+ e.toString() + ": " +
+ "Unable to extract feature for "
+ + name, e);
Review comment:
> > overriding toString(String) or toString() might be something to consider
>
> I am wondering whether it could also be an option to add the prefetchFields to `paramsToMap()`. I did not look into the side effects / benefits this might have yet.
`paramsToMap()` is also used as part of storage e.g. https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.8.2/solr/contrib/ltr/src/java/org/apache/solr/ltr/store/rest/ManagedFeatureStore.java#L196 neighbourhood. I suspect it will be interesting to look into that and what the side effects / benefits would be (and if there's perhaps a way for it to behave one way when converting to a map for storage and another way when converting to a map for logging purposes).
Another semi-related pointer into the same code neighbourhood would be https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.8.2/solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/Feature.java#L83 i.e. the setter mechanism.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r650366702
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+
+ public void setField(String field) {
+ this.field = field;
+ }
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @Override
+ public LinkedHashMap<String,Object> paramsToMap() {
+ final LinkedHashMap<String,Object> params = defaultParamsToMap();
+ params.put("field", field);
+ return params;
+ }
+
+ @Override
+ protected void validate() throws FeatureException {
+ if (field == null || field.isEmpty()) {
+ throw new FeatureException(getClass().getSimpleName()+
+ ": field must be provided");
+ }
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SchemaField schemaField;
+ private final SolrDocumentFetcher docFetcher;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+ if (searcher instanceof SolrIndexSearcher) {
+ schemaField = ((SolrIndexSearcher) searcher).getSchema().getFieldOrNull(field);
+ } else {
+ schemaField = null;
+ }
+ this.docFetcher = request.getSearcher().getDocFetcher();
+ }
+
+ /**
+ * Return a FeatureScorer that work with stored fields and makes use of the cache if the configured field is stored
+ * and has no docValues.
+ * Otherwise, delegate the work to the FieldValueFeature.
+ *
+ * @param context the segment this FeatureScorer is working with
+ * @return FeatureScorer for the current segment and field
+ * @throws IOException as defined by abstract class Feature
+ */
+ @Override
+ public FeatureScorer scorer(LeafReaderContext context) throws IOException {
+ if (schemaField != null && !schemaField.stored() && schemaField.hasDocValues()) {
+ return super.scorer(context);
+ }
+ return new PrefetchingFieldValueFeatureScorer(this, context,
+ DocIdSetIterator.all(DocIdSetIterator.NO_MORE_DOCS));
+ }
+
+ /**
+ * A FeatureScorer that reads the stored value for a field
+ * docFetcher does not request a single field but all the prefetchFields to improve performance through caching
+ */
+ public class PrefetchingFieldValueFeatureScorer extends FieldValueFeatureScorer {
+
+ public PrefetchingFieldValueFeatureScorer(FeatureWeight weight,
+ LeafReaderContext context, DocIdSetIterator itr) {
+ super(weight, context, itr);
+ }
+
+ @Override
+ public float score() throws IOException {
+ try {
+ final Document document = docFetcher.doc(context.docBase + itr.docID(), prefetchFields);
+
+ return super.parseStoredFieldValue(document.getField(field));
+ } catch (final IOException e) {
+ throw new FeatureException(
+ e.toString() + ": " +
+ "Unable to extract feature for "
+ + name, e);
Review comment:
Thank you for the pointers. :)
After trying `paramsToMap()` and `toString()` I opted for overriding `paramsToMap()` because I think it is very useful to see the prefetch fields when performing the get request on the feature store. ( fa1fb82 )
I had to do an adjustment in `paramsToMap()`: If there are no prefetch fields then we have to return an empty set instead of null to prevent a NPE further down the line.
While adding tests for `paramsToMap()` I realized, that the `doTestParamsToMap()` test did not work.
Two `PrefetchingFieldValueFeatures` are not equal, if one of them is initialized with a params map that is missing the prefetch fields and the other one is initialized with am empty set for the prefetchFields. (Because their params differ and therefore one has prefetchFields = null, the other one has an empty set.)
However, the prefetchFields should never be empty / null, because `featuresAsManagedResources()` is called after `preparePrefetchingFieldValueFeatures` where the prefetchFields are initialized.
So I would opt to ignore this.
If you disagree on ignoring that we could fix it by a) overriding `equals()` (quite a lot of code) or b) always initializing prefetchFields with an empty set.
I had to add another setter for the prefetchFields because when the params are loaded from the storage ( https://github.com/apache/lucene-solr/blob/a92a05e195b775b30ca410bc0a26e8e79e7b3bfb/solr/core/src/java/org/apache/solr/rest/ManagedResourceStorage.java#L489 ) the information that the prefetchFields are a Set is lost.
##########
File path: solr/contrib/ltr/src/test/org/apache/solr/ltr/TestCacheInteractionOfPrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,530 @@
+/* * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr;
+
+import org.apache.lucene.document.Document;
+import org.apache.lucene.document.Field;
+import org.apache.lucene.document.StoredField;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.misc.document.LazyDocument;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.client.solrj.SolrQuery;
+import org.apache.solr.client.solrj.response.QueryResponse;
+import org.apache.solr.common.SolrDocument;
+import org.apache.solr.common.SolrInputDocument;
+import org.apache.solr.ltr.feature.FeatureException;
+import org.apache.solr.ltr.feature.PrefetchingFieldValueFeature;
+import org.apache.solr.ltr.model.LinearModel;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.solr.ltr.feature.PrefetchingFieldValueFeature.DISABLE_PREFETCHING_FIELD_VALUE_FEATURE;
+
+public class TestCacheInteractionOfPrefetchingFieldValueFeature extends TestLTROnSolrCloudBase {
+ private final String LAZY_FIELD_LOADING_CONFIG_KEY = "solr.query.enableLazyFieldLoading";
+
+ private static final String FEATURE_STORE_NAME = "test";
+ private static final int NUM_FEATURES = 6;
+ private static final String[] FIELD_NAMES = new String[]{"storedIntField", "storedLongField",
+ "storedFloatField", "storedDoubleField", "storedStrNumField", "storedStrBoolField"};
+ private static final String[] FEATURE_NAMES = new String[]{"storedIntFieldFeature", "storedLongFieldFeature",
+ "storedFloatFieldFeature", "storedDoubleFieldFeature", "storedStrNumFieldFeature", "storedStrBoolFieldFeature"};
+ private static final String MODEL_WEIGHTS = "{\"weights\":{\"storedIntFieldFeature\":0.1,\"storedLongFieldFeature\":0.1," +
+ "\"storedFloatFieldFeature\":0.1,\"storedDoubleFieldFeature\":0.1," +
+ "\"storedStrNumFieldFeature\":0.1,\"storedStrBoolFieldFeature\":0.1}}";
+
+ @Override
+ void setupSolrCluster(int numShards, int numReplicas) throws Exception {
+ // we do not want to test the scoring / ranking but the interaction with the cache
+ // because the scoring itself behaves just like the FieldValueFeature
+ // so just one shard, replica and node serve the purpose
+ setupSolrCluster(1, 1, 1);
+ }
+
+ @Test
+ public void testSimpleQuery() throws Exception {
+ ObservingPrefetchingFieldValueFeature.setBreakPrefetching(false);
+ ObservingPrefetchingFieldValueFeature.loadedFields = new HashMap<>();
+ System.setProperty(LAZY_FIELD_LOADING_CONFIG_KEY, "false");
+ // needed to clear cache because we make assertions on its content
+ reloadCollection(COLLECTION);
+
+ SolrQuery query = new SolrQuery("{!func}sub(8,field(popularity))");
+ query.setRequestHandler("/query");
+ query.setParam("rows", "8");
+ query.setFields("id,features:[fv]");
+ query.add("rq", "{!ltr model=powpularityS-model reRankDocs=8}");
+
+ QueryResponse queryResponse = solrCluster.getSolrClient().query(COLLECTION,query);
+
+ Map<String, List<List<String>>> loadedFields = ObservingPrefetchingFieldValueFeature.loadedFields;
+
+ assertEquals(loadedFields.size(), queryResponse.getResults().size());
+ for (SolrDocument doc : queryResponse.getResults()) {
+ String docId = (String) doc.getFirstValue("id");
+ if (docId.equals("1")) {
+ assertEquals(NUM_FEATURES, loadedFields.get(docId).stream()
+ // doc with id 1 has no values set for 3 of the 6 feature fields
+ .filter(fieldLoadedList -> fieldLoadedList.size() == NUM_FEATURES - 3)
+ .count());
+ } else {
+ // all the fields were loaded at once
+ assertEquals(NUM_FEATURES, loadedFields.get(docId).stream()
+ .filter(fieldLoadedList -> fieldLoadedList.size() == NUM_FEATURES)
+ .count());
+ }
+ }
+ assertTheResponse(queryResponse);
+ }
+
+ @Test
+ public void testSimpleQueryLazy() throws Exception {
+ ObservingPrefetchingFieldValueFeature.setBreakPrefetching(false);
+ ObservingPrefetchingFieldValueFeature.loadedFields = new HashMap<>();
+ System.setProperty(LAZY_FIELD_LOADING_CONFIG_KEY, "true");
+ // needed to clear cache because we make assertions on its content
+ reloadCollection(COLLECTION);
+
+ SolrQuery query = new SolrQuery("{!func}sub(8,field(popularity))");
+ query.setRequestHandler("/query");
+ query.setParam("rows", "8");
+ query.setFields("id,features:[fv]");
+ query.add("rq", "{!ltr model=powpularityS-model reRankDocs=8}");
+
+ QueryResponse queryResponse = solrCluster.getSolrClient().query(COLLECTION,query);
+
+ Map<String, List<List<String>>> loadedFields = ObservingPrefetchingFieldValueFeature.loadedFields;
+
+ for (SolrDocument doc : queryResponse.getResults()) {
+ String docId = (String) doc.getFirstValue("id");
+ if (docId.equals("1")) {
+ assertEquals(NUM_FEATURES, loadedFields.get(docId).stream()
+ // doc with id 1 has no values set for 3 of the 6 feature fields
+ .filter(fieldLoadedList -> fieldLoadedList.size() == NUM_FEATURES - 3)
+ .count());
+ } else {
+ // all the fields were loaded at once
+ assertEquals(NUM_FEATURES, loadedFields.get(docId).stream()
+ .filter(fieldLoadedList -> fieldLoadedList.size() == NUM_FEATURES)
+ .count());
+ }
+ }
+ assertTheResponse(queryResponse);
+ }
+
+ @Test
+ public void testSimpleQueryBreakPrefetching() throws Exception {
+ ObservingPrefetchingFieldValueFeature.setBreakPrefetching(true);
+ ObservingPrefetchingFieldValueFeature.loadedFields = new HashMap<>();
+ System.setProperty(LAZY_FIELD_LOADING_CONFIG_KEY, "false");
+ // needed to clear cache because we make assertions on its content
+ reloadCollection(COLLECTION);
+
+ SolrQuery query = new SolrQuery("{!func}sub(8,field(popularity))");
+ query.setRequestHandler("/query");
+ query.setParam("rows", "8");
+ query.setFields("id,features:[fv]");
+ query.add("rq", "{!ltr model=powpularityS-model reRankDocs=8}");
+
+ QueryResponse queryResponse =
+ solrCluster.getSolrClient().query(COLLECTION,query);
+
+ Map<String, List<List<String>>> loadedFields = ObservingPrefetchingFieldValueFeature.loadedFields;
+
+ assertEquals(loadedFields.size(), queryResponse.getResults().size());
+ for (SolrDocument doc : queryResponse.getResults()) {
+ String docId = (String) doc.getFirstValue("id");
+ if (docId.equals("1")) {
+ // doc with id 1 has no values set for 3 of the 6 feature fields
+ assertEquals(NUM_FEATURES - 3, loadedFields.get(docId).stream()
+ .filter(fieldLoadedList -> fieldLoadedList.size() == 1)
+ .count());
+ } else {
+ // each single field used for a feature gets loaded separately
+ assertEquals(NUM_FEATURES, loadedFields.get(docId).stream()
+ .filter(fieldLoadedList -> fieldLoadedList.size() == 1)
+ .count());
+ }
+ }
+ assertTheResponse(queryResponse);
+ }
+
+ @Test
+ public void testSimpleQueryLazyBreakPrefetching() throws Exception {
Review comment:
I extracted some more code and added explanations for the thoughts behind the different tests ( 8d0ab32 ).
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.PrefetchingFieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+ // can be used for debugging to only fetch the field this features uses
+ public static final String DISABLE_PREFETCHING_FIELD_VALUE_FEATURE = "disablePrefetchingFieldValueFeature";
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SolrDocumentFetcher docFetcher;
+ private final Boolean disablePrefetching;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+
+ disablePrefetching = request.getParams().getBool(DISABLE_PREFETCHING_FIELD_VALUE_FEATURE, false);
Review comment:
Hi @cpoerschke ,
after thinking about your suggestion (in the comment below this) for quite some time, I am still not sure what to make from it.
To make my thoughts transparent:
* yes, the parameter and the switch add some complexity to the code
* however, the default user will most likely just stumble upon the param in the exception for the PFVF
* in the exception it is explained what to use the param for
* the "fallback-fetch" also adds quite some complexity to `score()` because of the try-catch-chain which I find harder to read than the current status
* downside of status quo: although being less complex inside `score()`, the parameter distributes its complexity through the whole feature all the way to the query
* I like about your idea, that one problematic field does not break all other prefetchFields
* this happens in the status quo
* however, this fallback means that if one field is broken, we basically have no benefit from the PFVF anymore. If users do only look at errors, they wouldn't even realize it (Sure, you shouldn't do that, but I cannot say how users handle their logs. If an exception is thrown, they at least are quickly aware of the problem)
* it bugs me that each PFVFS would try to fetch the broken field (all fields) before falling back to only getting its field. This does not feel efficient
* > If we had a fetchDocument method here for this block of code **and did not provide** a disablePrefetching option then the fetchDocument method would provide a hook
* I cannot follow this comment of yours. Couldn't a subclass just ignore the `disablePrefetching` when implementing an own `score()` or `fetchDocument()`? (own `fetchDocument()` method was added in 8c5071b )
**The missing piece for me is the following:**
**How likely is a failing fetch?** Can it just occur sometimes when some internal problems occur (A) ? Or will it happen all the time for one field that is somehow corrupted (B)?
While looking into possible tests for the fallback I tried to get to the core of the IOException. It seemed to be quite deep down (I remember some Byte-Buffers). So I would assume, that A would apply instead of B.
In that case, this argument from me
> if one field is broken, we basically have no benefit from the PFVF anymore
would not apply anymore, because one field would not constantly be broken.
These are my thoughts about this topic. I see arguments for both approaches and no showstoppers for either one, that's why I couldn't decide what to do.
Do you maybe have some insights about the likelihood of a fetch?
Or just general thoughts about it? Maybe I made some assumptions as base for my thoughts which aren't correct?
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.PrefetchingFieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+ // can be used for debugging to only fetch the field this features uses
+ public static final String DISABLE_PREFETCHING_FIELD_VALUE_FEATURE = "disablePrefetchingFieldValueFeature";
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SolrDocumentFetcher docFetcher;
+ private final Boolean disablePrefetching;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+
+ disablePrefetching = request.getParams().getBool(DISABLE_PREFETCHING_FIELD_VALUE_FEATURE, false);
+ // get the searcher directly from the request to be sure that we have a SolrIndexSearcher
+ this.docFetcher = request.getSearcher().getDocFetcher();
+ }
+
+ /**
+ * Return a FeatureScorer that works with stored fields and makes use of the cache if the configured field is stored
+ * and has no docValues.
+ * Otherwise, delegate the work to the FieldValueFeature.
+ *
+ * @param context the segment this FeatureScorer is working with
+ * @return FeatureScorer for the current segment and field
+ * @throws IOException as defined by abstract class Feature
+ */
+ @Override
+ public FeatureScorer scorer(LeafReaderContext context) throws IOException {
+ if (schemaField != null && !schemaField.stored() && schemaField.hasDocValues()) {
+ return super.scorer(context);
+ }
+ return new PrefetchingFieldValueFeatureScorer(this, context,
+ DocIdSetIterator.all(DocIdSetIterator.NO_MORE_DOCS));
+ }
+
+ /**
+ * A FeatureScorer that reads the stored value for a field
+ * docFetcher does not request a single field but all the prefetchFields to improve performance through caching
+ */
+ public class PrefetchingFieldValueFeatureScorer extends FieldValueFeatureScorer {
+
+ public PrefetchingFieldValueFeatureScorer(FeatureWeight weight,
+ LeafReaderContext context, DocIdSetIterator itr) {
+ super(weight, context, itr);
+ }
+
+ @Override
+ public float score() throws IOException {
+ try {
+ final Document document;
+ if(disablePrefetching) {
+ document = docFetcher.doc(context.docBase + itr.docID(), getFieldAsSet());
+ } else {
+ document = docFetcher.doc(context.docBase + itr.docID(), prefetchFields);
+ }
Review comment:
That is a very good suggestion, thank you! ( 8c5071b )
Enabling other people to customize their Solr (and hopefully contribute their ideas) is really something to keep an eye out for. :)
I am impressed by the number of ideas you already have for possible extensions of the class. First thoughts about that: While the disabling on a field name level is possible in subclasses of the PFVF, the disabling based on feature name seems to be a bigger task because we have no access to the names of other features yet.
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.PrefetchingFieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+ // can be used for debugging to only fetch the field this features uses
+ public static final String DISABLE_PREFETCHING_FIELD_VALUE_FEATURE = "disablePrefetchingFieldValueFeature";
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SolrDocumentFetcher docFetcher;
+ private final Boolean disablePrefetching;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+
+ disablePrefetching = request.getParams().getBool(DISABLE_PREFETCHING_FIELD_VALUE_FEATURE, false);
Review comment:
I just answered your comment below about extracting `fetchDocument()` and I think that I did not understand your comment regarding the `disablePrefetchign` correctly until now. Referencing this one:
> If we had a fetchDocument method here for this block of code and did not provide a disablePrefetching option then the fetchDocument method would provide a hook
Your intention is something like this
```java
public static final String DISABLE_PREFETCHING_FIELDS = "disablePrefetchingFieldValueFeatureFields";
private final List<String> disablePrefetching;
disablePrefetching = (List<String>) request.getParams().get(DISABLE_PREFETCHING_FIELDS, Collections.emptyList());
```
And that's why you want to remove the parameter from the PFVF because it does not have to be a boolean is all cases, right?
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.PrefetchingFieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+ // can be used for debugging to only fetch the field this features uses
+ public static final String DISABLE_PREFETCHING_FIELD_VALUE_FEATURE = "disablePrefetchingFieldValueFeature";
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SolrDocumentFetcher docFetcher;
+ private final Boolean disablePrefetching;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+
+ disablePrefetching = request.getParams().getBool(DISABLE_PREFETCHING_FIELD_VALUE_FEATURE, false);
Review comment:
I just answered your comment below about extracting `fetchDocument()` and I think that I did not understand your comment regarding the `disablePrefetchign` correctly until now. Referencing this one:
> If we had a fetchDocument method here for this block of code and did not provide a disablePrefetching option then the fetchDocument method would provide a hook
Your intention is something like this (in a subclass of PFVF)
```java
public static final String DISABLE_PREFETCHING_FIELDS = "disablePrefetchingFieldValueFeatureFields";
private final List<String> disablePrefetching;
disablePrefetching = (List<String>) request.getParams().get(DISABLE_PREFETCHING_FIELDS, Collections.emptyList());
```
And that's why you want to remove the parameter from the PFVF because it does not have to be a boolean is all cases, right?
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.PrefetchingFieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+ // can be used for debugging to only fetch the field this features uses
+ public static final String DISABLE_PREFETCHING_FIELD_VALUE_FEATURE = "disablePrefetchingFieldValueFeature";
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SolrDocumentFetcher docFetcher;
+ private final Boolean disablePrefetching;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+
+ disablePrefetching = request.getParams().getBool(DISABLE_PREFETCHING_FIELD_VALUE_FEATURE, false);
Review comment:
I just answered your comment below about extracting `fetchDocument()` and I think that I did not understand your comment regarding the `disablePrefetchign` correctly until now. Referencing this one:
> If we had a fetchDocument method here for this block of code and did not provide a disablePrefetching option then the fetchDocument method would provide a hook
Your intention is something like this (in a subclass of PFVF)
```java
public static final String DISABLE_PREFETCHING_FIELDS = "disablePrefetchingFieldValueFeatureFields";
private final List<String> disablePrefetching;
disablePrefetching = (List<String>) request.getParams().get(DISABLE_PREFETCHING_FIELDS, Collections.emptyList());
```
And that's why you want to remove the parameter from the PFVF because it does not have to be a boolean in all cases, right?
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.PrefetchingFieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+ // can be used for debugging to only fetch the field this features uses
+ public static final String DISABLE_PREFETCHING_FIELD_VALUE_FEATURE = "disablePrefetchingFieldValueFeature";
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SolrDocumentFetcher docFetcher;
+ private final Boolean disablePrefetching;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+
+ disablePrefetching = request.getParams().getBool(DISABLE_PREFETCHING_FIELD_VALUE_FEATURE, false);
Review comment:
Hi @cpoerschke ,
please do not worry about the length of your responses! I am extremely happy and thankful, that you take the time to share your thoughts and ideas in so much detail and with examples. :)
Besides being useful to understand you, it also sparks ideas in my brain and points out aspects that are not on my radar. Because of that collaborating with you is an amazing learning experience!
I stumbled upon this question of yours:
> how would the document fetcher behave if one of the fields does not exist in the schema or it exists but is not stored and not docValues? or if the field exists but is optional and the document being scored has no value for the field?
and added a test for it ( 4d84293 ).
No matter if a field is not existing, only indexed or empty, we always return the default.
The reason for this is that Lucene iterates over present fields and check which ones are needed, instead of trying to read all wanted fields (https://github.com/apache/lucene/blob/be94a667f2091b2c0fad570f9e726197d466767b/lucene/core/src/java/org/apache/lucene/codecs/lucene90/compressing/Lucene90CompressingStoredFieldsReader.java#L653 and https://github.com/apache/lucene/blob/be94a667f2091b2c0fad570f9e726197d466767b/lucene/core/src/java/org/apache/lucene/document/DocumentStoredFieldVisitor.java#L101).
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.PrefetchingFieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+ // can be used for debugging to only fetch the field this features uses
+ public static final String DISABLE_PREFETCHING_FIELD_VALUE_FEATURE = "disablePrefetchingFieldValueFeature";
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SolrDocumentFetcher docFetcher;
+ private final Boolean disablePrefetching;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+
+ disablePrefetching = request.getParams().getBool(DISABLE_PREFETCHING_FIELD_VALUE_FEATURE, false);
Review comment:
Reading through your commit, I am wondering why you split up the `computedPrefetchFields` and the `effectivePrefetchFields`.
I do understand the difference between them, but I am not sure what to use them for apart from the output of `paramsToMap()`.
Speaking of the output of paramsToMap, I remember this input of yours
> if our two hypethical nodes both complained about some features but one complained about fZ, fA, fX, fB and the other about fA, fZ, fB, fX then it might be tricky to interpret that they are both complaining about the same thing
I think in terms of debugging, your suggestion to use a sorted set, combined with keeping the field of the feature inside the params.prefetchFields makes the most sense.
This would result in duplicated information in the output, but I imagine the user doing stuff like "highlight" => "Strg + F" or searching the logs like "prefetchFields=fA, fB, fX, fZ" which would be impossible if we always remove the `field` from the output.
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.PrefetchingFieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+ // can be used for debugging to only fetch the field this features uses
+ public static final String DISABLE_PREFETCHING_FIELD_VALUE_FEATURE = "disablePrefetchingFieldValueFeature";
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SolrDocumentFetcher docFetcher;
+ private final Boolean disablePrefetching;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+
+ disablePrefetching = request.getParams().getBool(DISABLE_PREFETCHING_FIELD_VALUE_FEATURE, false);
Review comment:
Looking at `configuredPrefetchFields` maybe this could be an optional parameter in the features.json?
Behaving like `ltr.PrefetchingFieldValueFeature`.~~exclusions~~`.inclusions` on a config level instead of as a query parameter.
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.PrefetchingFieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+ // can be used for debugging to only fetch the field this features uses
+ public static final String DISABLE_PREFETCHING_FIELD_VALUE_FEATURE = "disablePrefetchingFieldValueFeature";
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SolrDocumentFetcher docFetcher;
+ private final Boolean disablePrefetching;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+
+ disablePrefetching = request.getParams().getBool(DISABLE_PREFETCHING_FIELD_VALUE_FEATURE, false);
Review comment:
Regarding `ltr.PrefetchingFieldValueFeature.exclusions`:
> I don't know if practically additional prefetch fields could work out as costly enough to justify the operational efforts of the outlined ltr.PrefetchingFieldValueFeature.exclusions parameter use
I was surprised by your point that multiple models may be using the same store. I just assumed that people would use one feature-store per model.
This could really lead to a lot of unnecessary fetching, especially is there is not much overlap between the features of the models.
**However**, after taking a step back and writing the test ( 4d84293 ), this `costly enough to justify` thought of you stayed in my mind.
The exclusion is not necessary to handle problems with only indexed or not existing fields.
Also, to put it very bluntly, I am wondering how much of user's misconfiguration we should try to handle in our code.
If there are two models that barely use the same features, which really would result in a lot of unnecessary fetching to the point where performance worsens, I would expect the user to use a separate feature-store per model.
Maybe his could even be a hint / recommendation in the documentation.
This statement from you:
> index corruption is (hopefully) a rare source of errors, and in such a scenario rethrowing the exception with the details would seem most useful.
made me want to remove the `disablePrefetchingFieldValueFeature` completely. While implementing it I felt like there would be more value to it, but now it seems like this param would only be useful in a situation where something is really wrong with the index (assuming that 'useful' means that there are quite some `Unable to extract feature...`-exceptions).
In that case the failing fetch of the PFVF seems to be the smallest problem besides like being unable to properly search anymore.
So I am thinking about:
```java
protected Document fetchDocument() throws FeatureException {
try {
return docFetcher.doc(context.docBase + itr.docID(), prefetchFields);
} catch (final IOException e) {
final String prefetchedFields = disablePrefetching ? getField() : StrUtils.join(prefetchFields, ',');
throw new FeatureException(...);
}
}
```
What do you think about the above change?
We could also provide some fail-safety with your suggested try-catch. (Good structure in the method / comments could make up for the complexity. And maybe log an error instead of a warning?)
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.PrefetchingFieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+ // can be used for debugging to only fetch the field this features uses
+ public static final String DISABLE_PREFETCHING_FIELD_VALUE_FEATURE = "disablePrefetchingFieldValueFeature";
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SolrDocumentFetcher docFetcher;
+ private final Boolean disablePrefetching;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+
+ disablePrefetching = request.getParams().getBool(DISABLE_PREFETCHING_FIELD_VALUE_FEATURE, false);
Review comment:
About:
> one thread reading and another updating seems potentially problematic
Thank you for having thread-safety / concurrency in mind! This is a topic about which I still have a lot to learn and build an awareness for.
To separate the different construction sites we are on, I would like to move the conversation about that to your comment at the bottom. (https://github.com/apache/solr/pull/166#discussion_r650491855)
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/LTRScoringQuery.java
##########
@@ -223,6 +223,7 @@ public ModelWeight createWeight(IndexSearcher searcher, ScoreMode scoreMode, flo
else{
createWeightsParallel(searcher, scoreMode.needsScores(), featureWeights, features);
}
+ // TODO: potential prefetchFields computation site?
Review comment:
Hi @cpoerschke ,
I am glad that this PR sparks your creativity! :)
This is why I find it so valuable to have sometimes long conversations. Hearing different opinions or concerns or having to explain some thoughts makes us reevaluate the current point of view which can be really inspiring!
I am also having a lot of fun discovering more and more of the internal workings of Solr & Lucene.
Thanks for leaving the `TODO` in the code!
> less or different visibility into the overall prefetch fields set
I think we could still provide the visibility by hooking into `doGet()` of the `ManagedFeatureStore` and collecting all the fields. So that shouldn't even be a problem.
I can clearly follow your pros and cons. My only concern is the re-computation per query. If we do the collection in the ManagedFeatureStore, we have to iterate and set the fields approx. once per shard*. If we do it after creating the weights, we have to do it once per query.
(*That's what a very quick "debugging" of the procedures while executing `TestCacheInteractionOfPrefetchingFieldValueFeature.testSimpleQuery()` with different shard numbers told me.)
I would like to think about the cost of the re-computation vs. the cost of handling the concurrency.
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.PrefetchingFieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+ // can be used for debugging to only fetch the field this features uses
+ public static final String DISABLE_PREFETCHING_FIELD_VALUE_FEATURE = "disablePrefetchingFieldValueFeature";
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SolrDocumentFetcher docFetcher;
+ private final Boolean disablePrefetching;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+
+ disablePrefetching = request.getParams().getBool(DISABLE_PREFETCHING_FIELD_VALUE_FEATURE, false);
Review comment:
Looking at `configuredPrefetchFields` maybe this could be an optional parameter in the features.json?
Behaving like `ltr.PrefetchingFieldValueFeature`.~~exclusions~~`.inclusions` on a config level instead of as a query parameter.
Apart from such an usage I am not sure about your thoughts behind that. This code here
```java
public void setPrefetchFields(Collection<String> fields) {
configuredPrefetchFields = fields;
// no effectivePrefetchFields update here, only computed fields are effective
}
```
confuses me.
You say that only the computed fields are the important ones. However the configured fields get updated but are not used anywhere.
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+
+ public void setField(String field) {
+ this.field = field;
+ }
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @Override
+ public LinkedHashMap<String,Object> paramsToMap() {
+ final LinkedHashMap<String,Object> params = defaultParamsToMap();
+ params.put("field", field);
+ return params;
+ }
+
+ @Override
+ protected void validate() throws FeatureException {
+ if (field == null || field.isEmpty()) {
+ throw new FeatureException(getClass().getSimpleName()+
+ ": field must be provided");
+ }
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SchemaField schemaField;
+ private final SolrDocumentFetcher docFetcher;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+ if (searcher instanceof SolrIndexSearcher) {
+ schemaField = ((SolrIndexSearcher) searcher).getSchema().getFieldOrNull(field);
+ } else {
+ schemaField = null;
+ }
+ this.docFetcher = request.getSearcher().getDocFetcher();
+ }
+
+ /**
+ * Return a FeatureScorer that work with stored fields and makes use of the cache if the configured field is stored
+ * and has no docValues.
+ * Otherwise, delegate the work to the FieldValueFeature.
+ *
+ * @param context the segment this FeatureScorer is working with
+ * @return FeatureScorer for the current segment and field
+ * @throws IOException as defined by abstract class Feature
+ */
+ @Override
+ public FeatureScorer scorer(LeafReaderContext context) throws IOException {
+ if (schemaField != null && !schemaField.stored() && schemaField.hasDocValues()) {
+ return super.scorer(context);
+ }
+ return new PrefetchingFieldValueFeatureScorer(this, context,
+ DocIdSetIterator.all(DocIdSetIterator.NO_MORE_DOCS));
+ }
+
+ /**
+ * A FeatureScorer that reads the stored value for a field
+ * docFetcher does not request a single field but all the prefetchFields to improve performance through caching
+ */
+ public class PrefetchingFieldValueFeatureScorer extends FieldValueFeatureScorer {
+
+ public PrefetchingFieldValueFeatureScorer(FeatureWeight weight,
+ LeafReaderContext context, DocIdSetIterator itr) {
+ super(weight, context, itr);
+ }
+
+ @Override
+ public float score() throws IOException {
+ try {
+ final Document document = docFetcher.doc(context.docBase + itr.docID(), prefetchFields);
+
+ return super.parseStoredFieldValue(document.getField(field));
+ } catch (final IOException e) {
+ throw new FeatureException(
+ e.toString() + ": " +
+ "Unable to extract feature for "
+ + name, e);
Review comment:
I talked about your commit in a comment below ( https://github.com/apache/solr/pull/166#discussion_r650527430 and following ), so I'm just leaving the pointer here. :)
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #166:
URL: https://github.com/apache/solr/pull/166#issuecomment-864225607
> ... summary of possible next steps and questions ...
Splendid idea, thanks for adding 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.
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
[GitHub] [solr] cpoerschke commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r656440736
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/LTRScoringQuery.java
##########
@@ -223,6 +223,7 @@ public ModelWeight createWeight(IndexSearcher searcher, ScoreMode scoreMode, flo
else{
createWeightsParallel(searcher, scoreMode.needsScores(), featureWeights, features);
}
+ // TODO: potential prefetchFields computation site?
Review comment:
> ... I never heard about `Clonable`, but that sounds like we could use it here. :)
I've seen it but never used it myself as yet, so learnt something new here too :)
> Yes, it makes the code more comprehensive. But my problem is, that now we just throw a generic `UnsupportedOperationException` instead of providing more information about the cause like we did with the custom message.
Yes, it's a bit of a trade-off i.e. more concise code in the general common code path case at the expense of only a generic exception in the rare/impossible code path when an exception is thrown. From my current understanding of the code the
```
throw new UnsupportedOperationException("Feature " + name + " is in use by a model. Its prefetchingFields may not be changed!");
```
can't ever happen, right? And so if it did happen then some very strange bug or problem presumably would have arisen and whilst the name of the feature might be useful to figure out the details perhaps the "is in use by a model" claim made by the exception would not be accurate in that scenario i.e. when the impossible happens very few assumptions about the state of the systems can then be made.
Removal of the `keepFeaturesFinalAfterModelCreation` boolean also means that we don't need to worry about boolean vs. AtomicBoolean or other thread-safety for it and it being or not being included when the object is cloned.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk commented on pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on pull request #166:
URL: https://github.com/apache/solr/pull/166#issuecomment-860246850
Hi @cpoerschke ,
rereading our conversation over the last two days made clear that we have quite some open constructions sites.
To not lose the overview about them (especially with a likely break from my side due to the upcoming Buzzwords) I want to put a summary of possible next steps and questions here:
* do we want to collect the prefetchFields after creating the weights instead of on the feature-level?
* concurrency vs. collecting more often
* remove the "disablePrefetchingFieldValueFeature" parameter
* add try-catch with single field to `score()`?
* if yes, then log error instead of warning?
* use sorted set for prefetchFields
* come to a decision regarding options to exclude / include fields
* do we want that?
* if yes, how (parameter in json / something per 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.
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
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r656487847
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/LTRScoringQuery.java
##########
@@ -223,6 +223,7 @@ public ModelWeight createWeight(IndexSearcher searcher, ScoreMode scoreMode, flo
else{
createWeightsParallel(searcher, scoreMode.needsScores(), featureWeights, features);
}
+ // TODO: potential prefetchFields computation site?
Review comment:
> can't ever happen, right? And so if it did happen then some very strange bug or problem presumably would have arisen
I think so too. That also made me struggle yesterday as I thought about testing.
Your arguments make sense to me. The additional complexity that e.g. the AtomicBoolean would provide also speaks against the use of the `keepFeaturesFinalAfterModelCreation`.
Maybe a comment could provide a bit more clarity for people who see the `prefetchFields = Collections.unmodifiableSortedSet(prefetchFields);` and a few lines below `prefetchFields.addAll(fields);`
Something like this b82b880
Regarding `Cloneable`:
I just experimented a bit with the `Cloneable` interface. It seems to only do a shallow copy which could be problematic with the prefetchFields. I used this code:
```java
public PrefetchingFieldValueFeature clone() throws CloneNotSupportedException {
return (PrefetchingFieldValueFeature) super.clone();
}
```
and made the PrefetchingFieldValueFeature implement Cloneable
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r646157071
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/FieldValueFeature.java
##########
@@ -65,7 +65,7 @@
*/
public class FieldValueFeature extends Feature {
- private String field;
+ String field;
Review comment:
Thank you for the comments below to help me making it private again. :) ( ab81d8b )
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r646023649
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
Review comment:
```suggestion
"class": "org.apache.solr.ltr.feature.PrefetchingFieldValueFeature",
```
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+
+ public void setField(String field) {
+ this.field = field;
+ }
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @Override
+ public LinkedHashMap<String,Object> paramsToMap() {
+ final LinkedHashMap<String,Object> params = defaultParamsToMap();
+ params.put("field", field);
+ return params;
+ }
+
+ @Override
+ protected void validate() throws FeatureException {
+ if (field == null || field.isEmpty()) {
+ throw new FeatureException(getClass().getSimpleName()+
+ ": field must be provided");
+ }
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
Review comment:
minor/subjective: place getter right after setter in the code, easier to see then that both getter and setter are present.
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/FieldValueFeature.java
##########
@@ -65,7 +65,7 @@
*/
public class FieldValueFeature extends Feature {
- private String field;
+ String field;
Review comment:
```suggestion
protected String field;
```
or if not needed for access in derived class (please see comment below) then keep as private
```suggestion
private String field;
```
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/FieldValueFeature.java
##########
@@ -166,33 +166,36 @@ public float score() throws IOException {
try {
final Document document = context.reader().document(itr.docID(),
fieldAsSet);
- final IndexableField indexableField = document.getField(field);
- if (indexableField == null) {
- return getDefaultValue();
- }
- final Number number = indexableField.numericValue();
- if (number != null) {
- return number.floatValue();
- } else {
- final String string = indexableField.stringValue();
- if (string.length() == 1) {
- // boolean values in the index are encoded with the
- // a single char contained in TRUE_TOKEN or FALSE_TOKEN
- // (see BoolField)
- if (string.charAt(0) == BoolField.TRUE_TOKEN[0]) {
- return 1;
- }
- if (string.charAt(0) == BoolField.FALSE_TOKEN[0]) {
- return 0;
- }
- }
- }
+ return parseStoredFieldValue(document.getField(field));
} catch (final IOException e) {
throw new FeatureException(
e.toString() + ": " +
"Unable to extract feature for "
+ name, e);
}
+ }
+
+ float parseStoredFieldValue(IndexableField indexableField) throws IOException {
Review comment:
```suggestion
protected float parseStoredFieldValue(IndexableField indexableField) throws IOException {
```
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+
+ public void setField(String field) {
+ this.field = field;
+ }
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @Override
+ public LinkedHashMap<String,Object> paramsToMap() {
+ final LinkedHashMap<String,Object> params = defaultParamsToMap();
+ params.put("field", field);
+ return params;
+ }
+
+ @Override
+ protected void validate() throws FeatureException {
+ if (field == null || field.isEmpty()) {
+ throw new FeatureException(getClass().getSimpleName()+
+ ": field must be provided");
+ }
+ }
Review comment:
```suggestion
```
Use of super class' methods should work or if not let's add comment w.r.t. why we are overriding.
##########
File path: solr/contrib/ltr/src/test-files/solr/collection1/conf/solrconfig-ltr.xml
##########
@@ -34,6 +34,8 @@
initialSize="2048" autowarmCount="0" />
<cache name="QUERY_DOC_FV" class="solr.search.CaffeineCache" size="4096"
initialSize="2048" autowarmCount="4096" regenerator="solr.search.NoOpRegenerator" />
+ <!-- false is the default, just here to make it explizit in contrast to solrconfig-ltr-lazy-->
+ <enableLazyFieldLoading>false</enableLazyFieldLoading>
Review comment:
Thanks for adding the comment here!
Question: is the `enableLazyFieldLoading` value the only difference between the `solrconfig-ltr.xml` and `solrconfig-ltr-lazy.xml` files?
Context for the question: if it is the only difference (and no other future differences are anticipated or intended) then we could consider use of something like `${enableLazyFieldLoading:false}` to achieve differentiated test behaviour from a single file. https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.8.2/solr/core/src/test-files/solr/collection1/conf/solrconfig-managed-schema.xml#L56 does something like that elsewhere but for this boolean or https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.8.2/solr/contrib/ltr/src/test-files/solr/collection1/conf/solrconfig-ltr.xml#L45 does something similar within the LTR tests.
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+
+ public void setField(String field) {
+ this.field = field;
+ }
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @Override
+ public LinkedHashMap<String,Object> paramsToMap() {
+ final LinkedHashMap<String,Object> params = defaultParamsToMap();
+ params.put("field", field);
+ return params;
+ }
+
+ @Override
+ protected void validate() throws FeatureException {
+ if (field == null || field.isEmpty()) {
+ throw new FeatureException(getClass().getSimpleName()+
+ ": field must be provided");
+ }
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SchemaField schemaField;
Review comment:
```suggestion
```
If we change the super class's `schemaField` visibility from `private` to `protected` then `PrefetchingFieldValueFeatureWeight` would have access for use without needing to initialise it in the `PrefetchingFieldValueFeatureWeight` constructor.
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+
+ public void setField(String field) {
+ this.field = field;
+ }
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @Override
+ public LinkedHashMap<String,Object> paramsToMap() {
+ final LinkedHashMap<String,Object> params = defaultParamsToMap();
+ params.put("field", field);
+ return params;
+ }
+
+ @Override
+ protected void validate() throws FeatureException {
+ if (field == null || field.isEmpty()) {
+ throw new FeatureException(getClass().getSimpleName()+
+ ": field must be provided");
+ }
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SchemaField schemaField;
+ private final SolrDocumentFetcher docFetcher;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+ if (searcher instanceof SolrIndexSearcher) {
+ schemaField = ((SolrIndexSearcher) searcher).getSchema().getFieldOrNull(field);
+ } else {
+ schemaField = null;
+ }
+ this.docFetcher = request.getSearcher().getDocFetcher();
Review comment:
A comment here w.r.t. why `request.getSearcher()` rather than `(SolrIndexSearcher) searcher` could be useful for future readers of the code.
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+
+ public void setField(String field) {
+ this.field = field;
+ }
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @Override
+ public LinkedHashMap<String,Object> paramsToMap() {
+ final LinkedHashMap<String,Object> params = defaultParamsToMap();
+ params.put("field", field);
+ return params;
+ }
+
+ @Override
+ protected void validate() throws FeatureException {
+ if (field == null || field.isEmpty()) {
+ throw new FeatureException(getClass().getSimpleName()+
+ ": field must be provided");
+ }
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SchemaField schemaField;
+ private final SolrDocumentFetcher docFetcher;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+ if (searcher instanceof SolrIndexSearcher) {
+ schemaField = ((SolrIndexSearcher) searcher).getSchema().getFieldOrNull(field);
+ } else {
+ schemaField = null;
+ }
+ this.docFetcher = request.getSearcher().getDocFetcher();
+ }
+
+ /**
+ * Return a FeatureScorer that work with stored fields and makes use of the cache if the configured field is stored
+ * and has no docValues.
+ * Otherwise, delegate the work to the FieldValueFeature.
+ *
+ * @param context the segment this FeatureScorer is working with
+ * @return FeatureScorer for the current segment and field
+ * @throws IOException as defined by abstract class Feature
+ */
+ @Override
+ public FeatureScorer scorer(LeafReaderContext context) throws IOException {
+ if (schemaField != null && !schemaField.stored() && schemaField.hasDocValues()) {
+ return super.scorer(context);
+ }
+ return new PrefetchingFieldValueFeatureScorer(this, context,
+ DocIdSetIterator.all(DocIdSetIterator.NO_MORE_DOCS));
+ }
+
+ /**
+ * A FeatureScorer that reads the stored value for a field
+ * docFetcher does not request a single field but all the prefetchFields to improve performance through caching
+ */
+ public class PrefetchingFieldValueFeatureScorer extends FieldValueFeatureScorer {
+
+ public PrefetchingFieldValueFeatureScorer(FeatureWeight weight,
+ LeafReaderContext context, DocIdSetIterator itr) {
+ super(weight, context, itr);
+ }
+
+ @Override
+ public float score() throws IOException {
+ try {
+ final Document document = docFetcher.doc(context.docBase + itr.docID(), prefetchFields);
+
+ return super.parseStoredFieldValue(document.getField(field));
+ } catch (final IOException e) {
+ throw new FeatureException(
+ e.toString() + ": " +
+ "Unable to extract feature for "
+ + name, e);
Review comment:
For `FieldValueFeature[Scorer]` it is possible to map a feature name to the field whilst troubleshooting if this exception were to be encountered. For `PrefetchingFieldValueFeature[Scorer]` the scenario is more complex though i.e. the root cause of the exception might not be the field of the named feature but another field or fields in the `prefetchFields` set.
Would you have any thoughts on how we might account for that in the exception wording? My initial thought was to include all the `prefetchFields` fields in the exception but on second thought that could be overwhelming if there are quite a few fields in some use cases.
Also with troubleshooting in mind, I wonder if `prefetchFields` being not just a set but an ordered set might be useful (_assuming_ that ordering would _not_ have a runtime performance cost).
Oh and one more thing vaguely related also, overriding `toString(String)` or `toString()` might be something to consider, the super class methods will correctly include the field itself but it will not include the `prefetchFields` since that is specific to this class and not part of the configuration parameters.
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/store/rest/ManagedFeatureStore.java
##########
@@ -138,15 +147,36 @@ public Object applyUpdatesToManagedData(Object updates) {
Map<String,Object> updatesMap = (Map<String,Object>) updates;
final String featureStore = (String) updatesMap.get(FEATURE_STORE_NAME_KEY);
addFeature(updatesMap, featureStore);
+ updatedFeatureStores.add(featureStore);
}
+ preparePrefetchingFieldValueFeatures(updatedFeatureStores);
+
final List<Object> features = new ArrayList<>();
for (final FeatureStore fs : stores.values()) {
features.addAll(featuresAsManagedResources(fs));
}
return features;
}
+ private void preparePrefetchingFieldValueFeatures(Set<String> updatedFeatureStores) {
+ for(String featureStoreName: updatedFeatureStores) {
+ final Set<PrefetchingFieldValueFeature> prefetchingFieldValueFeatures = getFeatureStore(featureStoreName)
+ .getFeatures().stream()
+ .filter(feature -> feature instanceof PrefetchingFieldValueFeature)
+ .map(feature -> ((PrefetchingFieldValueFeature) feature))
+ .collect(Collectors.toSet());
Review comment:
Very elegant! :-)
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+
+ public void setField(String field) {
+ this.field = field;
+ }
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @Override
+ public LinkedHashMap<String,Object> paramsToMap() {
+ final LinkedHashMap<String,Object> params = defaultParamsToMap();
+ params.put("field", field);
+ return params;
+ }
+
+ @Override
+ protected void validate() throws FeatureException {
+ if (field == null || field.isEmpty()) {
+ throw new FeatureException(getClass().getSimpleName()+
+ ": field must be provided");
+ }
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SchemaField schemaField;
+ private final SolrDocumentFetcher docFetcher;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+ if (searcher instanceof SolrIndexSearcher) {
+ schemaField = ((SolrIndexSearcher) searcher).getSchema().getFieldOrNull(field);
+ } else {
+ schemaField = null;
+ }
Review comment:
```suggestion
```
in favour of super class' `schemaField` becoming `protected` instead of `private`
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+
+ public void setField(String field) {
+ this.field = field;
+ }
Review comment:
```suggestion
```
Use of the super class' `setField` should work (I think, or if not let's add comment w.r.t. why we override) and whilst at first glance it seems that the super class also filling the singleton`fieldAsSet` could be marginally wasteful I suspect it might actually be necessary if this class here were to end up delegating to the super class implementation where the super class implementation might use `fieldAsSet`. Also less code here if we don't have a setField override.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk commented on pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on pull request #166:
URL: https://github.com/apache/solr/pull/166#issuecomment-855438132
Hi @cpoerschke ,
thank you for the quick response. :)
I addressed your suggestions that were quick to include and will look into the exception-handling / toString()-topics next.
> Not yet looked at the test changes
I am actually quite glad that you didn't, because I just changed the tests quite a lot.
Now there is quite a lot of duplicated code if you look at the two prefetching tests in `org.apache.solr.ltr` and the `TestLTROnSolrCloud`.
I already had a `TestLTROnSolrCloudBase`-class ready to extract the duplicates but was not sure whether to include such a change in this PR because it it would entail a few changes in `TestLTROnSolrCloud` which have nothing to do with the prefetching functionality.
I can make a commit tomorrow(?) only including this change and if you think that it would make the PR too complex we can just revert 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.
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
[GitHub] [solr] cpoerschke commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r648514785
##########
File path: solr/contrib/ltr/src/test/org/apache/solr/ltr/TestCacheInteractionOfPrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,530 @@
+/* * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr;
+
+import org.apache.lucene.document.Document;
+import org.apache.lucene.document.Field;
+import org.apache.lucene.document.StoredField;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.misc.document.LazyDocument;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.client.solrj.SolrQuery;
+import org.apache.solr.client.solrj.response.QueryResponse;
+import org.apache.solr.common.SolrDocument;
+import org.apache.solr.common.SolrInputDocument;
+import org.apache.solr.ltr.feature.FeatureException;
+import org.apache.solr.ltr.feature.PrefetchingFieldValueFeature;
+import org.apache.solr.ltr.model.LinearModel;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.solr.ltr.feature.PrefetchingFieldValueFeature.DISABLE_PREFETCHING_FIELD_VALUE_FEATURE;
+
+public class TestCacheInteractionOfPrefetchingFieldValueFeature extends TestLTROnSolrCloudBase {
+ private final String LAZY_FIELD_LOADING_CONFIG_KEY = "solr.query.enableLazyFieldLoading";
+
+ private static final String FEATURE_STORE_NAME = "test";
+ private static final int NUM_FEATURES = 6;
+ private static final String[] FIELD_NAMES = new String[]{"storedIntField", "storedLongField",
+ "storedFloatField", "storedDoubleField", "storedStrNumField", "storedStrBoolField"};
+ private static final String[] FEATURE_NAMES = new String[]{"storedIntFieldFeature", "storedLongFieldFeature",
+ "storedFloatFieldFeature", "storedDoubleFieldFeature", "storedStrNumFieldFeature", "storedStrBoolFieldFeature"};
+ private static final String MODEL_WEIGHTS = "{\"weights\":{\"storedIntFieldFeature\":0.1,\"storedLongFieldFeature\":0.1," +
+ "\"storedFloatFieldFeature\":0.1,\"storedDoubleFieldFeature\":0.1," +
+ "\"storedStrNumFieldFeature\":0.1,\"storedStrBoolFieldFeature\":0.1}}";
+
+ @Override
+ void setupSolrCluster(int numShards, int numReplicas) throws Exception {
+ // we do not want to test the scoring / ranking but the interaction with the cache
+ // because the scoring itself behaves just like the FieldValueFeature
+ // so just one shard, replica and node serve the purpose
+ setupSolrCluster(1, 1, 1);
+ }
+
+ @Test
+ public void testSimpleQuery() throws Exception {
+ ObservingPrefetchingFieldValueFeature.setBreakPrefetching(false);
+ ObservingPrefetchingFieldValueFeature.loadedFields = new HashMap<>();
+ System.setProperty(LAZY_FIELD_LOADING_CONFIG_KEY, "false");
+ // needed to clear cache because we make assertions on its content
+ reloadCollection(COLLECTION);
+
+ SolrQuery query = new SolrQuery("{!func}sub(8,field(popularity))");
+ query.setRequestHandler("/query");
+ query.setParam("rows", "8");
+ query.setFields("id,features:[fv]");
+ query.add("rq", "{!ltr model=powpularityS-model reRankDocs=8}");
+
+ QueryResponse queryResponse = solrCluster.getSolrClient().query(COLLECTION,query);
+
+ Map<String, List<List<String>>> loadedFields = ObservingPrefetchingFieldValueFeature.loadedFields;
+
+ assertEquals(loadedFields.size(), queryResponse.getResults().size());
+ for (SolrDocument doc : queryResponse.getResults()) {
+ String docId = (String) doc.getFirstValue("id");
+ if (docId.equals("1")) {
+ assertEquals(NUM_FEATURES, loadedFields.get(docId).stream()
+ // doc with id 1 has no values set for 3 of the 6 feature fields
+ .filter(fieldLoadedList -> fieldLoadedList.size() == NUM_FEATURES - 3)
+ .count());
+ } else {
+ // all the fields were loaded at once
+ assertEquals(NUM_FEATURES, loadedFields.get(docId).stream()
+ .filter(fieldLoadedList -> fieldLoadedList.size() == NUM_FEATURES)
+ .count());
+ }
+ }
+ assertTheResponse(queryResponse);
+ }
+
+ @Test
+ public void testSimpleQueryLazy() throws Exception {
+ ObservingPrefetchingFieldValueFeature.setBreakPrefetching(false);
+ ObservingPrefetchingFieldValueFeature.loadedFields = new HashMap<>();
+ System.setProperty(LAZY_FIELD_LOADING_CONFIG_KEY, "true");
+ // needed to clear cache because we make assertions on its content
+ reloadCollection(COLLECTION);
+
+ SolrQuery query = new SolrQuery("{!func}sub(8,field(popularity))");
+ query.setRequestHandler("/query");
+ query.setParam("rows", "8");
+ query.setFields("id,features:[fv]");
+ query.add("rq", "{!ltr model=powpularityS-model reRankDocs=8}");
+
+ QueryResponse queryResponse = solrCluster.getSolrClient().query(COLLECTION,query);
+
+ Map<String, List<List<String>>> loadedFields = ObservingPrefetchingFieldValueFeature.loadedFields;
+
+ for (SolrDocument doc : queryResponse.getResults()) {
+ String docId = (String) doc.getFirstValue("id");
+ if (docId.equals("1")) {
+ assertEquals(NUM_FEATURES, loadedFields.get(docId).stream()
+ // doc with id 1 has no values set for 3 of the 6 feature fields
+ .filter(fieldLoadedList -> fieldLoadedList.size() == NUM_FEATURES - 3)
+ .count());
+ } else {
+ // all the fields were loaded at once
+ assertEquals(NUM_FEATURES, loadedFields.get(docId).stream()
+ .filter(fieldLoadedList -> fieldLoadedList.size() == NUM_FEATURES)
+ .count());
+ }
+ }
+ assertTheResponse(queryResponse);
+ }
+
+ @Test
+ public void testSimpleQueryBreakPrefetching() throws Exception {
+ ObservingPrefetchingFieldValueFeature.setBreakPrefetching(true);
+ ObservingPrefetchingFieldValueFeature.loadedFields = new HashMap<>();
+ System.setProperty(LAZY_FIELD_LOADING_CONFIG_KEY, "false");
+ // needed to clear cache because we make assertions on its content
+ reloadCollection(COLLECTION);
+
+ SolrQuery query = new SolrQuery("{!func}sub(8,field(popularity))");
+ query.setRequestHandler("/query");
+ query.setParam("rows", "8");
+ query.setFields("id,features:[fv]");
+ query.add("rq", "{!ltr model=powpularityS-model reRankDocs=8}");
+
+ QueryResponse queryResponse =
+ solrCluster.getSolrClient().query(COLLECTION,query);
+
+ Map<String, List<List<String>>> loadedFields = ObservingPrefetchingFieldValueFeature.loadedFields;
+
+ assertEquals(loadedFields.size(), queryResponse.getResults().size());
+ for (SolrDocument doc : queryResponse.getResults()) {
+ String docId = (String) doc.getFirstValue("id");
+ if (docId.equals("1")) {
+ // doc with id 1 has no values set for 3 of the 6 feature fields
+ assertEquals(NUM_FEATURES - 3, loadedFields.get(docId).stream()
+ .filter(fieldLoadedList -> fieldLoadedList.size() == 1)
+ .count());
+ } else {
+ // each single field used for a feature gets loaded separately
+ assertEquals(NUM_FEATURES, loadedFields.get(docId).stream()
+ .filter(fieldLoadedList -> fieldLoadedList.size() == 1)
+ .count());
+ }
+ }
+ assertTheResponse(queryResponse);
+ }
+
+ @Test
+ public void testSimpleQueryLazyBreakPrefetching() throws Exception {
Review comment:
At a glance it's tricky to see how much overlap there is between the tests in this class here but conceptually if two tests `testOdd` and `testEven` are almost identical it could be helpful to factor out a common implementation method to reduce code duplication e.g.
```
public void testOdd() {
implTestOddOrEven(true);
}
public void testEven() {
implTestOddOrEven(false);
}
private void implTestOddOrEven(boolean odd) {
...
}
```
##########
File path: solr/contrib/ltr/src/test/org/apache/solr/ltr/TestCacheInteractionOfPrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,530 @@
+/* * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr;
+
+import org.apache.lucene.document.Document;
+import org.apache.lucene.document.Field;
+import org.apache.lucene.document.StoredField;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.misc.document.LazyDocument;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.client.solrj.SolrQuery;
+import org.apache.solr.client.solrj.response.QueryResponse;
+import org.apache.solr.common.SolrDocument;
+import org.apache.solr.common.SolrInputDocument;
+import org.apache.solr.ltr.feature.FeatureException;
+import org.apache.solr.ltr.feature.PrefetchingFieldValueFeature;
+import org.apache.solr.ltr.model.LinearModel;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.solr.ltr.feature.PrefetchingFieldValueFeature.DISABLE_PREFETCHING_FIELD_VALUE_FEATURE;
+
+public class TestCacheInteractionOfPrefetchingFieldValueFeature extends TestLTROnSolrCloudBase {
+ private final String LAZY_FIELD_LOADING_CONFIG_KEY = "solr.query.enableLazyFieldLoading";
+
+ private static final String FEATURE_STORE_NAME = "test";
+ private static final int NUM_FEATURES = 6;
+ private static final String[] FIELD_NAMES = new String[]{"storedIntField", "storedLongField",
+ "storedFloatField", "storedDoubleField", "storedStrNumField", "storedStrBoolField"};
+ private static final String[] FEATURE_NAMES = new String[]{"storedIntFieldFeature", "storedLongFieldFeature",
+ "storedFloatFieldFeature", "storedDoubleFieldFeature", "storedStrNumFieldFeature", "storedStrBoolFieldFeature"};
+ private static final String MODEL_WEIGHTS = "{\"weights\":{\"storedIntFieldFeature\":0.1,\"storedLongFieldFeature\":0.1," +
+ "\"storedFloatFieldFeature\":0.1,\"storedDoubleFieldFeature\":0.1," +
+ "\"storedStrNumFieldFeature\":0.1,\"storedStrBoolFieldFeature\":0.1}}";
+
+ @Override
+ void setupSolrCluster(int numShards, int numReplicas) throws Exception {
+ // we do not want to test the scoring / ranking but the interaction with the cache
+ // because the scoring itself behaves just like the FieldValueFeature
+ // so just one shard, replica and node serve the purpose
+ setupSolrCluster(1, 1, 1);
+ }
+
+ @Test
+ public void testSimpleQuery() throws Exception {
+ ObservingPrefetchingFieldValueFeature.setBreakPrefetching(false);
+ ObservingPrefetchingFieldValueFeature.loadedFields = new HashMap<>();
+ System.setProperty(LAZY_FIELD_LOADING_CONFIG_KEY, "false");
+ // needed to clear cache because we make assertions on its content
+ reloadCollection(COLLECTION);
+
+ SolrQuery query = new SolrQuery("{!func}sub(8,field(popularity))");
+ query.setRequestHandler("/query");
+ query.setParam("rows", "8");
+ query.setFields("id,features:[fv]");
+ query.add("rq", "{!ltr model=powpularityS-model reRankDocs=8}");
Review comment:
minor: Might factoring out a `composeSolrQuery` method similar to how `assertTheResponse` logic is already shared between tests help reduce code duplication?
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+
+ public void setField(String field) {
+ this.field = field;
+ }
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @Override
+ public LinkedHashMap<String,Object> paramsToMap() {
+ final LinkedHashMap<String,Object> params = defaultParamsToMap();
+ params.put("field", field);
+ return params;
+ }
+
+ @Override
+ protected void validate() throws FeatureException {
+ if (field == null || field.isEmpty()) {
+ throw new FeatureException(getClass().getSimpleName()+
+ ": field must be provided");
+ }
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SchemaField schemaField;
+ private final SolrDocumentFetcher docFetcher;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+ if (searcher instanceof SolrIndexSearcher) {
+ schemaField = ((SolrIndexSearcher) searcher).getSchema().getFieldOrNull(field);
+ } else {
+ schemaField = null;
+ }
+ this.docFetcher = request.getSearcher().getDocFetcher();
Review comment:
> `// get the searcher directly from the request to be sure that we have a SolrIndexSearcher`
Yes, that's exactly the kind of comment that will avoid confusion for future code readers, thanks for adding it!
##########
File path: solr/contrib/ltr/src/test/org/apache/solr/ltr/TestCacheInteractionOfPrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,530 @@
+/* * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr;
+
+import org.apache.lucene.document.Document;
+import org.apache.lucene.document.Field;
+import org.apache.lucene.document.StoredField;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.misc.document.LazyDocument;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.client.solrj.SolrQuery;
+import org.apache.solr.client.solrj.response.QueryResponse;
+import org.apache.solr.common.SolrDocument;
+import org.apache.solr.common.SolrInputDocument;
+import org.apache.solr.ltr.feature.FeatureException;
+import org.apache.solr.ltr.feature.PrefetchingFieldValueFeature;
+import org.apache.solr.ltr.model.LinearModel;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.solr.ltr.feature.PrefetchingFieldValueFeature.DISABLE_PREFETCHING_FIELD_VALUE_FEATURE;
+
+public class TestCacheInteractionOfPrefetchingFieldValueFeature extends TestLTROnSolrCloudBase {
+ private final String LAZY_FIELD_LOADING_CONFIG_KEY = "solr.query.enableLazyFieldLoading";
+
+ private static final String FEATURE_STORE_NAME = "test";
+ private static final int NUM_FEATURES = 6;
+ private static final String[] FIELD_NAMES = new String[]{"storedIntField", "storedLongField",
+ "storedFloatField", "storedDoubleField", "storedStrNumField", "storedStrBoolField"};
+ private static final String[] FEATURE_NAMES = new String[]{"storedIntFieldFeature", "storedLongFieldFeature",
+ "storedFloatFieldFeature", "storedDoubleFieldFeature", "storedStrNumFieldFeature", "storedStrBoolFieldFeature"};
+ private static final String MODEL_WEIGHTS = "{\"weights\":{\"storedIntFieldFeature\":0.1,\"storedLongFieldFeature\":0.1," +
+ "\"storedFloatFieldFeature\":0.1,\"storedDoubleFieldFeature\":0.1," +
+ "\"storedStrNumFieldFeature\":0.1,\"storedStrBoolFieldFeature\":0.1}}";
+
+ @Override
+ void setupSolrCluster(int numShards, int numReplicas) throws Exception {
+ // we do not want to test the scoring / ranking but the interaction with the cache
+ // because the scoring itself behaves just like the FieldValueFeature
+ // so just one shard, replica and node serve the purpose
+ setupSolrCluster(1, 1, 1);
+ }
+
+ @Test
+ public void testSimpleQuery() throws Exception {
+ ObservingPrefetchingFieldValueFeature.setBreakPrefetching(false);
+ ObservingPrefetchingFieldValueFeature.loadedFields = new HashMap<>();
+ System.setProperty(LAZY_FIELD_LOADING_CONFIG_KEY, "false");
+ // needed to clear cache because we make assertions on its content
+ reloadCollection(COLLECTION);
+
+ SolrQuery query = new SolrQuery("{!func}sub(8,field(popularity))");
+ query.setRequestHandler("/query");
+ query.setParam("rows", "8");
+ query.setFields("id,features:[fv]");
+ query.add("rq", "{!ltr model=powpularityS-model reRankDocs=8}");
+
+ QueryResponse queryResponse = solrCluster.getSolrClient().query(COLLECTION,query);
+
+ Map<String, List<List<String>>> loadedFields = ObservingPrefetchingFieldValueFeature.loadedFields;
+
+ assertEquals(loadedFields.size(), queryResponse.getResults().size());
+ for (SolrDocument doc : queryResponse.getResults()) {
+ String docId = (String) doc.getFirstValue("id");
+ if (docId.equals("1")) {
+ assertEquals(NUM_FEATURES, loadedFields.get(docId).stream()
+ // doc with id 1 has no values set for 3 of the 6 feature fields
+ .filter(fieldLoadedList -> fieldLoadedList.size() == NUM_FEATURES - 3)
+ .count());
+ } else {
+ // all the fields were loaded at once
+ assertEquals(NUM_FEATURES, loadedFields.get(docId).stream()
+ .filter(fieldLoadedList -> fieldLoadedList.size() == NUM_FEATURES)
+ .count());
+ }
+ }
+ assertTheResponse(queryResponse);
+ }
+
+ @Test
+ public void testSimpleQueryLazy() throws Exception {
+ ObservingPrefetchingFieldValueFeature.setBreakPrefetching(false);
+ ObservingPrefetchingFieldValueFeature.loadedFields = new HashMap<>();
+ System.setProperty(LAZY_FIELD_LOADING_CONFIG_KEY, "true");
+ // needed to clear cache because we make assertions on its content
+ reloadCollection(COLLECTION);
Review comment:
I haven't yet looked in detail w.r.t. how the lazy-loading flag influences the test logic but if the lazy=false and lazy=true tests are identical (or pretty similar) then it might be worth considering to use randomisation e.g. like in https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.8.2/solr/core/src/test/org/apache/solr/search/CursorMarkTest.java#L54 i.e. before the tests `false` or `true` is chosen and that is then used for the tests. It means that a given run only covers "half" the possibilities but it's also half the code _and_ there would be no need for collection reloading (which could also slow down the test) but yes if cache clearing needs to be achieved we still would have to do that -- might adding/deleting/updating of a document have that effect indirectly or maybe there's some other way to directly clear the document cache in a test?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r648658565
##########
File path: solr/contrib/ltr/src/test/org/apache/solr/ltr/TestCacheInteractionOfPrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,530 @@
+/* * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr;
+
+import org.apache.lucene.document.Document;
+import org.apache.lucene.document.Field;
+import org.apache.lucene.document.StoredField;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.misc.document.LazyDocument;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.client.solrj.SolrQuery;
+import org.apache.solr.client.solrj.response.QueryResponse;
+import org.apache.solr.common.SolrDocument;
+import org.apache.solr.common.SolrInputDocument;
+import org.apache.solr.ltr.feature.FeatureException;
+import org.apache.solr.ltr.feature.PrefetchingFieldValueFeature;
+import org.apache.solr.ltr.model.LinearModel;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.solr.ltr.feature.PrefetchingFieldValueFeature.DISABLE_PREFETCHING_FIELD_VALUE_FEATURE;
+
+public class TestCacheInteractionOfPrefetchingFieldValueFeature extends TestLTROnSolrCloudBase {
+ private final String LAZY_FIELD_LOADING_CONFIG_KEY = "solr.query.enableLazyFieldLoading";
+
+ private static final String FEATURE_STORE_NAME = "test";
+ private static final int NUM_FEATURES = 6;
+ private static final String[] FIELD_NAMES = new String[]{"storedIntField", "storedLongField",
+ "storedFloatField", "storedDoubleField", "storedStrNumField", "storedStrBoolField"};
+ private static final String[] FEATURE_NAMES = new String[]{"storedIntFieldFeature", "storedLongFieldFeature",
+ "storedFloatFieldFeature", "storedDoubleFieldFeature", "storedStrNumFieldFeature", "storedStrBoolFieldFeature"};
+ private static final String MODEL_WEIGHTS = "{\"weights\":{\"storedIntFieldFeature\":0.1,\"storedLongFieldFeature\":0.1," +
+ "\"storedFloatFieldFeature\":0.1,\"storedDoubleFieldFeature\":0.1," +
+ "\"storedStrNumFieldFeature\":0.1,\"storedStrBoolFieldFeature\":0.1}}";
+
+ @Override
+ void setupSolrCluster(int numShards, int numReplicas) throws Exception {
+ // we do not want to test the scoring / ranking but the interaction with the cache
+ // because the scoring itself behaves just like the FieldValueFeature
+ // so just one shard, replica and node serve the purpose
+ setupSolrCluster(1, 1, 1);
+ }
+
+ @Test
+ public void testSimpleQuery() throws Exception {
+ ObservingPrefetchingFieldValueFeature.setBreakPrefetching(false);
+ ObservingPrefetchingFieldValueFeature.loadedFields = new HashMap<>();
+ System.setProperty(LAZY_FIELD_LOADING_CONFIG_KEY, "false");
+ // needed to clear cache because we make assertions on its content
+ reloadCollection(COLLECTION);
+
+ SolrQuery query = new SolrQuery("{!func}sub(8,field(popularity))");
+ query.setRequestHandler("/query");
+ query.setParam("rows", "8");
+ query.setFields("id,features:[fv]");
+ query.add("rq", "{!ltr model=powpularityS-model reRankDocs=8}");
+
+ QueryResponse queryResponse = solrCluster.getSolrClient().query(COLLECTION,query);
+
+ Map<String, List<List<String>>> loadedFields = ObservingPrefetchingFieldValueFeature.loadedFields;
+
+ assertEquals(loadedFields.size(), queryResponse.getResults().size());
+ for (SolrDocument doc : queryResponse.getResults()) {
+ String docId = (String) doc.getFirstValue("id");
+ if (docId.equals("1")) {
+ assertEquals(NUM_FEATURES, loadedFields.get(docId).stream()
+ // doc with id 1 has no values set for 3 of the 6 feature fields
+ .filter(fieldLoadedList -> fieldLoadedList.size() == NUM_FEATURES - 3)
+ .count());
+ } else {
+ // all the fields were loaded at once
+ assertEquals(NUM_FEATURES, loadedFields.get(docId).stream()
+ .filter(fieldLoadedList -> fieldLoadedList.size() == NUM_FEATURES)
+ .count());
+ }
+ }
+ assertTheResponse(queryResponse);
+ }
+
+ @Test
+ public void testSimpleQueryLazy() throws Exception {
+ ObservingPrefetchingFieldValueFeature.setBreakPrefetching(false);
+ ObservingPrefetchingFieldValueFeature.loadedFields = new HashMap<>();
+ System.setProperty(LAZY_FIELD_LOADING_CONFIG_KEY, "true");
+ // needed to clear cache because we make assertions on its content
+ reloadCollection(COLLECTION);
Review comment:
Oh, the randomization is a very good idea!
With this we were able to remove half of the tests. ( 2d355c7 ) However, the reload is still needed.
I can have a look tomorrow if I find a nicer solution. At least it happens half as often now.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r648658709
##########
File path: solr/contrib/ltr/src/test/org/apache/solr/ltr/TestCacheInteractionOfPrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,530 @@
+/* * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr;
+
+import org.apache.lucene.document.Document;
+import org.apache.lucene.document.Field;
+import org.apache.lucene.document.StoredField;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.misc.document.LazyDocument;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.client.solrj.SolrQuery;
+import org.apache.solr.client.solrj.response.QueryResponse;
+import org.apache.solr.common.SolrDocument;
+import org.apache.solr.common.SolrInputDocument;
+import org.apache.solr.ltr.feature.FeatureException;
+import org.apache.solr.ltr.feature.PrefetchingFieldValueFeature;
+import org.apache.solr.ltr.model.LinearModel;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.solr.ltr.feature.PrefetchingFieldValueFeature.DISABLE_PREFETCHING_FIELD_VALUE_FEATURE;
+
+public class TestCacheInteractionOfPrefetchingFieldValueFeature extends TestLTROnSolrCloudBase {
+ private final String LAZY_FIELD_LOADING_CONFIG_KEY = "solr.query.enableLazyFieldLoading";
+
+ private static final String FEATURE_STORE_NAME = "test";
+ private static final int NUM_FEATURES = 6;
+ private static final String[] FIELD_NAMES = new String[]{"storedIntField", "storedLongField",
+ "storedFloatField", "storedDoubleField", "storedStrNumField", "storedStrBoolField"};
+ private static final String[] FEATURE_NAMES = new String[]{"storedIntFieldFeature", "storedLongFieldFeature",
+ "storedFloatFieldFeature", "storedDoubleFieldFeature", "storedStrNumFieldFeature", "storedStrBoolFieldFeature"};
+ private static final String MODEL_WEIGHTS = "{\"weights\":{\"storedIntFieldFeature\":0.1,\"storedLongFieldFeature\":0.1," +
+ "\"storedFloatFieldFeature\":0.1,\"storedDoubleFieldFeature\":0.1," +
+ "\"storedStrNumFieldFeature\":0.1,\"storedStrBoolFieldFeature\":0.1}}";
+
+ @Override
+ void setupSolrCluster(int numShards, int numReplicas) throws Exception {
+ // we do not want to test the scoring / ranking but the interaction with the cache
+ // because the scoring itself behaves just like the FieldValueFeature
+ // so just one shard, replica and node serve the purpose
+ setupSolrCluster(1, 1, 1);
+ }
+
+ @Test
+ public void testSimpleQuery() throws Exception {
+ ObservingPrefetchingFieldValueFeature.setBreakPrefetching(false);
+ ObservingPrefetchingFieldValueFeature.loadedFields = new HashMap<>();
+ System.setProperty(LAZY_FIELD_LOADING_CONFIG_KEY, "false");
+ // needed to clear cache because we make assertions on its content
+ reloadCollection(COLLECTION);
+
+ SolrQuery query = new SolrQuery("{!func}sub(8,field(popularity))");
+ query.setRequestHandler("/query");
+ query.setParam("rows", "8");
+ query.setFields("id,features:[fv]");
+ query.add("rq", "{!ltr model=powpularityS-model reRankDocs=8}");
Review comment:
Yes, your idea helped to reduce the duplication. :) ( 2d355c7 )
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r648536798
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.PrefetchingFieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+ // can be used for debugging to only fetch the field this features uses
+ public static final String DISABLE_PREFETCHING_FIELD_VALUE_FEATURE = "disablePrefetchingFieldValueFeature";
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SolrDocumentFetcher docFetcher;
+ private final Boolean disablePrefetching;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+
+ disablePrefetching = request.getParams().getBool(DISABLE_PREFETCHING_FIELD_VALUE_FEATURE, false);
+ // get the searcher directly from the request to be sure that we have a SolrIndexSearcher
+ this.docFetcher = request.getSearcher().getDocFetcher();
+ }
+
+ /**
+ * Return a FeatureScorer that works with stored fields and makes use of the cache if the configured field is stored
+ * and has no docValues.
+ * Otherwise, delegate the work to the FieldValueFeature.
+ *
+ * @param context the segment this FeatureScorer is working with
+ * @return FeatureScorer for the current segment and field
+ * @throws IOException as defined by abstract class Feature
+ */
+ @Override
+ public FeatureScorer scorer(LeafReaderContext context) throws IOException {
+ if (schemaField != null && !schemaField.stored() && schemaField.hasDocValues()) {
+ return super.scorer(context);
+ }
+ return new PrefetchingFieldValueFeatureScorer(this, context,
+ DocIdSetIterator.all(DocIdSetIterator.NO_MORE_DOCS));
+ }
+
+ /**
+ * A FeatureScorer that reads the stored value for a field
+ * docFetcher does not request a single field but all the prefetchFields to improve performance through caching
+ */
+ public class PrefetchingFieldValueFeatureScorer extends FieldValueFeatureScorer {
+
+ public PrefetchingFieldValueFeatureScorer(FeatureWeight weight,
+ LeafReaderContext context, DocIdSetIterator itr) {
+ super(weight, context, itr);
+ }
+
+ @Override
+ public float score() throws IOException {
+ try {
+ final Document document;
+ if(disablePrefetching) {
+ document = docFetcher.doc(context.docBase + itr.docID(), getFieldAsSet());
+ } else {
+ document = docFetcher.doc(context.docBase + itr.docID(), prefetchFields);
+ }
Review comment:
If we had a `fetchDocument` method here for this block of code and did not provide a `disablePrefetching` option then the `fetchDocument` method would provide a hook via which expert users (and test classes) can build their own `MyPrefetchingFieldValueFeature extends PrefetchingFieldValueFeature` class with prefetching disabling support _and_ they might choose a non-boolean implementation e.g. `noPrefetchFeatures=foo,bar` could disable prefetching only for the features named `foo` and `bar` or `noPrefetchFeaturePrefix=bla` for partial matching against the feature name to cover `bla1` and `blaBla` and `blaBlaBla` etc. feature names. Or perhaps prefetching disabling should not be by feature name but by field name?
```
protected Document fetchDocument() {
...
}
```
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.PrefetchingFieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+ // can be used for debugging to only fetch the field this features uses
+ public static final String DISABLE_PREFETCHING_FIELD_VALUE_FEATURE = "disablePrefetchingFieldValueFeature";
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SolrDocumentFetcher docFetcher;
+ private final Boolean disablePrefetching;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+
+ disablePrefetching = request.getParams().getBool(DISABLE_PREFETCHING_FIELD_VALUE_FEATURE, false);
Review comment:
Hmm, interesting idea, I'd like to go away and ponder it a little. My initial thoughts are that yes debugging can be facilitated this way but at a cost of (marginal) additional code complexity and from a general user perspective it's another flag to understand and use (or not use), and with that in mind my intuitive preference would be to not have it, but I'd like to go and think about it some more properly.
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+
+ public void setField(String field) {
+ this.field = field;
+ }
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @Override
+ public LinkedHashMap<String,Object> paramsToMap() {
+ final LinkedHashMap<String,Object> params = defaultParamsToMap();
+ params.put("field", field);
+ return params;
+ }
+
+ @Override
+ protected void validate() throws FeatureException {
+ if (field == null || field.isEmpty()) {
+ throw new FeatureException(getClass().getSimpleName()+
+ ": field must be provided");
+ }
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SchemaField schemaField;
+ private final SolrDocumentFetcher docFetcher;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+ if (searcher instanceof SolrIndexSearcher) {
+ schemaField = ((SolrIndexSearcher) searcher).getSchema().getFieldOrNull(field);
+ } else {
+ schemaField = null;
+ }
+ this.docFetcher = request.getSearcher().getDocFetcher();
+ }
+
+ /**
+ * Return a FeatureScorer that work with stored fields and makes use of the cache if the configured field is stored
+ * and has no docValues.
+ * Otherwise, delegate the work to the FieldValueFeature.
+ *
+ * @param context the segment this FeatureScorer is working with
+ * @return FeatureScorer for the current segment and field
+ * @throws IOException as defined by abstract class Feature
+ */
+ @Override
+ public FeatureScorer scorer(LeafReaderContext context) throws IOException {
+ if (schemaField != null && !schemaField.stored() && schemaField.hasDocValues()) {
+ return super.scorer(context);
+ }
+ return new PrefetchingFieldValueFeatureScorer(this, context,
+ DocIdSetIterator.all(DocIdSetIterator.NO_MORE_DOCS));
+ }
+
+ /**
+ * A FeatureScorer that reads the stored value for a field
+ * docFetcher does not request a single field but all the prefetchFields to improve performance through caching
+ */
+ public class PrefetchingFieldValueFeatureScorer extends FieldValueFeatureScorer {
+
+ public PrefetchingFieldValueFeatureScorer(FeatureWeight weight,
+ LeafReaderContext context, DocIdSetIterator itr) {
+ super(weight, context, itr);
+ }
+
+ @Override
+ public float score() throws IOException {
+ try {
+ final Document document = docFetcher.doc(context.docBase + itr.docID(), prefetchFields);
+
+ return super.parseStoredFieldValue(document.getField(field));
+ } catch (final IOException e) {
+ throw new FeatureException(
+ e.toString() + ": " +
+ "Unable to extract feature for "
+ + name, e);
Review comment:
> Also with troubleshooting in mind, I wonder if `prefetchFields` being not just a set but an ordered set might be useful (_assuming_ that ordering would _not_ have a runtime performance cost).
> I assume that your intention behind the ordered set is, that if the user wanted to prefetch fields `A, B, C, D` and an exception regarding field `C` would happen, they would know that `A` and `B` worked. Did I understand you correctly?
I think my thoughts were a bit vague on that actually, thank you for encouraging me to organise them a bit more :)
* Depending on how folks name their features and how many features there are, if one had to interpret error/exception logging with the details then `fA, fC, ... fX, fZ` i.e. alphabetical is (subjectively) more human friendly than some random ordering that might arise via the HashSet implementation.
* Given the same inputs in the same order, will two Solr nodes come up with the same HashSet ordering for them or not? What if the nodes run different Java versions or one node has been running a long time and another is recently started? I don't know the answer to those questions but if our two hypethical nodes both complained about some features but one complained about `fZ, fA, fX, fB` and the other about `fA, fZ, fB, fX` then it might be tricky to interpret that they are both complaining about the same thing. Or if there is a difference between them then sorted ordering should make it easier to spot what the difference is.
* In terms of knowing that a field C exception means that fields A and B still worked, no, that's actually not something that had occurred to me. But now that you mention it in that way i.e. the potential for one feature or one field to affect some but not all of the other features ... I wonder if the exception handling could log (say) a WARNing and then do a noPrefetching retry, something like this:
```
try {
document = docFetcher.doc(context.docBase + itr.docID(), prefetchFields);
...
} catch {
... log warning ...
try {
document = docFetcher.doc(context.docBase + itr.docID(), fieldAsSet);
... log info to say it worked ...
...
} catch {
throw exception
}
}
```
(And I note that such a fallback-to-no-prefetching mechanism would reduce the need for a `disablePrefetching` flag on the request level i.e. the system would "self debug" i.e. the unproblematic fields/features would log a warning and then an info to indicate that the fallback worked and problematic fields/features would log a warning and then presumably would fail again and throw an exception after the single-field retry.)
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r656418979
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/LTRScoringQuery.java
##########
@@ -223,6 +223,7 @@ public ModelWeight createWeight(IndexSearcher searcher, ScoreMode scoreMode, flo
else{
createWeightsParallel(searcher, scoreMode.needsScores(), featureWeights, features);
}
+ // TODO: potential prefetchFields computation site?
Review comment:
> if the setField and setIndex calls in the clone() method were a key part to fix the tests
They were essential to make the tests work. I never heard about `Clonable`, but that sounds like we could use it here. :)
> 69c6611 tries that out, what do you think?
I can see where you are coming from, but to be honest I am not really a fan of this change.
Yes, it makes the code more comprehensive. But my problem is, that now we just throw a generic `UnsupportedOperationException` instead of providing more information about the cause like we did with the custom message.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r650532471
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.PrefetchingFieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+ // can be used for debugging to only fetch the field this features uses
+ public static final String DISABLE_PREFETCHING_FIELD_VALUE_FEATURE = "disablePrefetchingFieldValueFeature";
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SolrDocumentFetcher docFetcher;
+ private final Boolean disablePrefetching;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+
+ disablePrefetching = request.getParams().getBool(DISABLE_PREFETCHING_FIELD_VALUE_FEATURE, false);
Review comment:
Regarding `ltr.PrefetchingFieldValueFeature.exclusions`:
> I don't know if practically additional prefetch fields could work out as costly enough to justify the operational efforts of the outlined ltr.PrefetchingFieldValueFeature.exclusions parameter use
I was surprised by your point that multiple models may be using the same store. I just assumed that people would use one feature-store per model.
This could really lead to a lot of unnecessary fetching, especially is there is not much overlap between the features of the models.
**However**, after taking a step back and writing the test ( 4d84293 ), this `costly enough to justify` thought of you stayed in my mind.
The exclusion is not necessary to handle problems with only indexed or not existing fields.
Also, to put it very bluntly, I am wondering how much of user's misconfiguration we should try to handle in our code.
If there are two models that barely use the same features, which really would result in a lot of unnecessary fetching to the point where performance worsens, I would expect the user to use a separate feature-store per model.
Maybe his could even be a hint / recommendation in the documentation.
This statement from you:
> index corruption is (hopefully) a rare source of errors, and in such a scenario rethrowing the exception with the details would seem most useful.
made me want to remove the `disablePrefetchingFieldValueFeature` completely. While implementing it I felt like there would be more value to it, but now it seems like this param would only be useful in a situation where something is really wrong with the index (assuming that 'useful' means that there are quite some `Unable to extract feature...`-exceptions).
In that case the failing fetch of the PFVF seems to be the smallest problem besides like being unable to properly search anymore.
So I am thinking about:
```java
protected Document fetchDocument() throws FeatureException {
try {
return docFetcher.doc(context.docBase + itr.docID(), prefetchFields);
} catch (final IOException e) {
final String prefetchedFields = StrUtils.join(prefetchFields, ',');
throw new FeatureException(...);
}
}
```
What do you think about the above change?
We could also provide some fail-safety with your suggested try-catch. (Good structure in the method / comments could make up for the complexity. And maybe log an error instead of a warning?)
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r647722195
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+
+ public void setField(String field) {
+ this.field = field;
+ }
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @Override
+ public LinkedHashMap<String,Object> paramsToMap() {
+ final LinkedHashMap<String,Object> params = defaultParamsToMap();
+ params.put("field", field);
+ return params;
+ }
+
+ @Override
+ protected void validate() throws FeatureException {
+ if (field == null || field.isEmpty()) {
+ throw new FeatureException(getClass().getSimpleName()+
+ ": field must be provided");
+ }
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SchemaField schemaField;
+ private final SolrDocumentFetcher docFetcher;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+ if (searcher instanceof SolrIndexSearcher) {
+ schemaField = ((SolrIndexSearcher) searcher).getSchema().getFieldOrNull(field);
+ } else {
+ schemaField = null;
+ }
+ this.docFetcher = request.getSearcher().getDocFetcher();
+ }
+
+ /**
+ * Return a FeatureScorer that work with stored fields and makes use of the cache if the configured field is stored
+ * and has no docValues.
+ * Otherwise, delegate the work to the FieldValueFeature.
+ *
+ * @param context the segment this FeatureScorer is working with
+ * @return FeatureScorer for the current segment and field
+ * @throws IOException as defined by abstract class Feature
+ */
+ @Override
+ public FeatureScorer scorer(LeafReaderContext context) throws IOException {
+ if (schemaField != null && !schemaField.stored() && schemaField.hasDocValues()) {
+ return super.scorer(context);
+ }
+ return new PrefetchingFieldValueFeatureScorer(this, context,
+ DocIdSetIterator.all(DocIdSetIterator.NO_MORE_DOCS));
+ }
+
+ /**
+ * A FeatureScorer that reads the stored value for a field
+ * docFetcher does not request a single field but all the prefetchFields to improve performance through caching
+ */
+ public class PrefetchingFieldValueFeatureScorer extends FieldValueFeatureScorer {
+
+ public PrefetchingFieldValueFeatureScorer(FeatureWeight weight,
+ LeafReaderContext context, DocIdSetIterator itr) {
+ super(weight, context, itr);
+ }
+
+ @Override
+ public float score() throws IOException {
+ try {
+ final Document document = docFetcher.doc(context.docBase + itr.docID(), prefetchFields);
+
+ return super.parseStoredFieldValue(document.getField(field));
+ } catch (final IOException e) {
+ throw new FeatureException(
+ e.toString() + ": " +
+ "Unable to extract feature for "
+ + name, e);
Review comment:
Small update on that topic:
I was looking into possible exceptions that could be thrown and tried to debug whether the order of the fields is equal to the order that they are read in from the index.
Based on this:
```java
public void visitDocument(int docID, StoredFieldVisitor visitor) throws IOException {
final Lucene90CompressingStoredFieldsReader.SerializedDocument doc = document(docID);
for (int fieldIDX = 0; fieldIDX < doc.numStoredFields; fieldIDX++) {
final long infoAndBits = doc.in.readVLong();
final int fieldNumber = (int) (infoAndBits >>> TYPE_BITS);
final FieldInfo fieldInfo = fieldInfos.fieldInfo(fieldNumber);
...
}
```
it seems to me like it only depends on the fieldNumber.
I assume that your intention behind the ordered set is, that if the user wanted to prefetch fields `A, B, C, D` and an exception regarding field `C` would happen, they would know that `A` and `B` worked. Did I understand you correctly?
The IOException seems to be thrown quite deep in the code, so I am not sure whether we can get useful information from there.
I had the idea of adding a toggle that the user could use to disable the prefetching (which would result in the same behavior the `FieldValueFeature` has). This way the user can narrow down the problematic field. ( 33efe62 )
What do you think about that approach?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r646156912
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
Review comment:
ups. ^^ ( ab81d8b )
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk edited a comment on pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk edited a comment on pull request #166:
URL: https://github.com/apache/solr/pull/166#issuecomment-860246850
Hi @cpoerschke ,
rereading our conversation over the last two days made clear that we have quite some open constructions sites.
To not lose the overview about them (especially with a likely break from my side due to the upcoming Buzzwords) I want to put a summary of possible next steps and questions here:
- [ ] do we want to collect the prefetchFields after creating the weights instead of on the feature-level?
* concurrency vs. collecting more often
- [X] remove the "disablePrefetchingFieldValueFeature" parameter ( 81a5c67 )
* add try-catch with single field to `score()`?
* if yes, then log error instead of warning?
* **added try-catch with error logging**
- [X] use sorted set for prefetchFields ( d0520a9 )
- [ ] come to a decision regarding options to exclude / include fields
* do we want that?
* if yes, how (parameter in json / something per 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.
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
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r646156639
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+
+ public void setField(String field) {
+ this.field = field;
+ }
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @Override
+ public LinkedHashMap<String,Object> paramsToMap() {
+ final LinkedHashMap<String,Object> params = defaultParamsToMap();
+ params.put("field", field);
+ return params;
+ }
+
+ @Override
+ protected void validate() throws FeatureException {
+ if (field == null || field.isEmpty()) {
+ throw new FeatureException(getClass().getSimpleName()+
+ ": field must be provided");
+ }
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SchemaField schemaField;
Review comment:
Changed the visibility and removed the logic from the constructor. ( ab81d8b )
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r655924437
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/LTRScoringQuery.java
##########
@@ -223,6 +223,7 @@ public ModelWeight createWeight(IndexSearcher searcher, ScoreMode scoreMode, flo
else{
createWeightsParallel(searcher, scoreMode.needsScores(), featureWeights, features);
}
+ // TODO: potential prefetchFields computation site?
Review comment:
Hi @cpoerschke ,
from the approaches that we discussed until now, I like this one best.
I committed your implementation in 7ac6e35.
(Also, I did a few small changes to fix the tests and some renaming because the terms `frozen` and `freeze` made it a bit hard to understand.)
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/LTRScoringQuery.java
##########
@@ -223,6 +223,7 @@ public ModelWeight createWeight(IndexSearcher searcher, ScoreMode scoreMode, flo
else{
createWeightsParallel(searcher, scoreMode.needsScores(), featureWeights, features);
}
+ // TODO: potential prefetchFields computation site?
Review comment:
Hi @cpoerschke ,
from the approaches that we discussed until now, I like this one best.
I committed your implementation in 7ac6e35.
(Also, I did a few small changes to fix the tests and some renaming because the terms `frozen` and `freeze` made it a bit hard to understand.)
Tests for that are missing. Yesterday evening felt a bit too late to start with that.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r648001252
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+
+ public void setField(String field) {
+ this.field = field;
+ }
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @Override
+ public LinkedHashMap<String,Object> paramsToMap() {
+ final LinkedHashMap<String,Object> params = defaultParamsToMap();
+ params.put("field", field);
+ return params;
+ }
+
+ @Override
+ protected void validate() throws FeatureException {
+ if (field == null || field.isEmpty()) {
+ throw new FeatureException(getClass().getSimpleName()+
+ ": field must be provided");
+ }
+ }
Review comment:
I removed the methods. ( ab81d8b )
However, I am still thinking about `paramsToMap()`, like mentioned here https://github.com/apache/solr/pull/166#discussion_r647723902
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk edited a comment on pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk edited a comment on pull request #166:
URL: https://github.com/apache/solr/pull/166#issuecomment-860246850
Hi @cpoerschke ,
rereading our conversation over the last two days made clear that we have quite some open constructions sites.
To not lose the overview about them (especially with a likely break from my side due to the upcoming Buzzwords) I want to put a summary of possible next steps and questions here:
- [ ] do we want to collect the prefetchFields after creating the weights instead of on the feature-level?
* concurrency vs. collecting more often
- [X] remove the "disablePrefetchingFieldValueFeature" parameter ( 81a5c67 )
* add try-catch with single field to `score()`?
* if yes, then log error instead of warning?
* **added try-catch with error logging**
- [X] use sorted set for prefetchFields ( d0520a9 )
- [ ] come to a decision regarding options to exclude / include fields
* do we want that?
* if yes, how (parameter in json / something per query)
* **proposed solution / answer**: No. This could be an additional feature that users can implement (like your outline here https://github.com/apache/solr/pull/166#discussion_r648536798 ) but I would exclude it from this feature because we do not have clear use cases 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.
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
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r646156520
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+
+ public void setField(String field) {
+ this.field = field;
+ }
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @Override
+ public LinkedHashMap<String,Object> paramsToMap() {
+ final LinkedHashMap<String,Object> params = defaultParamsToMap();
+ params.put("field", field);
+ return params;
+ }
+
+ @Override
+ protected void validate() throws FeatureException {
+ if (field == null || field.isEmpty()) {
+ throw new FeatureException(getClass().getSimpleName()+
+ ": field must be provided");
+ }
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SchemaField schemaField;
+ private final SolrDocumentFetcher docFetcher;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+ if (searcher instanceof SolrIndexSearcher) {
+ schemaField = ((SolrIndexSearcher) searcher).getSchema().getFieldOrNull(field);
+ } else {
+ schemaField = null;
+ }
Review comment:
In combination with the comment above this makes sense to me. ( ab81d8b )
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r648660579
##########
File path: solr/contrib/ltr/src/test/org/apache/solr/ltr/TestCacheInteractionOfPrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,530 @@
+/* * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr;
+
+import org.apache.lucene.document.Document;
+import org.apache.lucene.document.Field;
+import org.apache.lucene.document.StoredField;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.misc.document.LazyDocument;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.client.solrj.SolrQuery;
+import org.apache.solr.client.solrj.response.QueryResponse;
+import org.apache.solr.common.SolrDocument;
+import org.apache.solr.common.SolrInputDocument;
+import org.apache.solr.ltr.feature.FeatureException;
+import org.apache.solr.ltr.feature.PrefetchingFieldValueFeature;
+import org.apache.solr.ltr.model.LinearModel;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.solr.ltr.feature.PrefetchingFieldValueFeature.DISABLE_PREFETCHING_FIELD_VALUE_FEATURE;
+
+public class TestCacheInteractionOfPrefetchingFieldValueFeature extends TestLTROnSolrCloudBase {
+ private final String LAZY_FIELD_LOADING_CONFIG_KEY = "solr.query.enableLazyFieldLoading";
+
+ private static final String FEATURE_STORE_NAME = "test";
+ private static final int NUM_FEATURES = 6;
+ private static final String[] FIELD_NAMES = new String[]{"storedIntField", "storedLongField",
+ "storedFloatField", "storedDoubleField", "storedStrNumField", "storedStrBoolField"};
+ private static final String[] FEATURE_NAMES = new String[]{"storedIntFieldFeature", "storedLongFieldFeature",
+ "storedFloatFieldFeature", "storedDoubleFieldFeature", "storedStrNumFieldFeature", "storedStrBoolFieldFeature"};
+ private static final String MODEL_WEIGHTS = "{\"weights\":{\"storedIntFieldFeature\":0.1,\"storedLongFieldFeature\":0.1," +
+ "\"storedFloatFieldFeature\":0.1,\"storedDoubleFieldFeature\":0.1," +
+ "\"storedStrNumFieldFeature\":0.1,\"storedStrBoolFieldFeature\":0.1}}";
+
+ @Override
+ void setupSolrCluster(int numShards, int numReplicas) throws Exception {
+ // we do not want to test the scoring / ranking but the interaction with the cache
+ // because the scoring itself behaves just like the FieldValueFeature
+ // so just one shard, replica and node serve the purpose
+ setupSolrCluster(1, 1, 1);
+ }
+
+ @Test
+ public void testSimpleQuery() throws Exception {
+ ObservingPrefetchingFieldValueFeature.setBreakPrefetching(false);
+ ObservingPrefetchingFieldValueFeature.loadedFields = new HashMap<>();
+ System.setProperty(LAZY_FIELD_LOADING_CONFIG_KEY, "false");
+ // needed to clear cache because we make assertions on its content
+ reloadCollection(COLLECTION);
+
+ SolrQuery query = new SolrQuery("{!func}sub(8,field(popularity))");
+ query.setRequestHandler("/query");
+ query.setParam("rows", "8");
+ query.setFields("id,features:[fv]");
+ query.add("rq", "{!ltr model=powpularityS-model reRankDocs=8}");
+
+ QueryResponse queryResponse = solrCluster.getSolrClient().query(COLLECTION,query);
+
+ Map<String, List<List<String>>> loadedFields = ObservingPrefetchingFieldValueFeature.loadedFields;
+
+ assertEquals(loadedFields.size(), queryResponse.getResults().size());
+ for (SolrDocument doc : queryResponse.getResults()) {
+ String docId = (String) doc.getFirstValue("id");
+ if (docId.equals("1")) {
+ assertEquals(NUM_FEATURES, loadedFields.get(docId).stream()
+ // doc with id 1 has no values set for 3 of the 6 feature fields
+ .filter(fieldLoadedList -> fieldLoadedList.size() == NUM_FEATURES - 3)
+ .count());
+ } else {
+ // all the fields were loaded at once
+ assertEquals(NUM_FEATURES, loadedFields.get(docId).stream()
+ .filter(fieldLoadedList -> fieldLoadedList.size() == NUM_FEATURES)
+ .count());
+ }
+ }
+ assertTheResponse(queryResponse);
+ }
+
+ @Test
+ public void testSimpleQueryLazy() throws Exception {
+ ObservingPrefetchingFieldValueFeature.setBreakPrefetching(false);
+ ObservingPrefetchingFieldValueFeature.loadedFields = new HashMap<>();
+ System.setProperty(LAZY_FIELD_LOADING_CONFIG_KEY, "true");
+ // needed to clear cache because we make assertions on its content
+ reloadCollection(COLLECTION);
+
+ SolrQuery query = new SolrQuery("{!func}sub(8,field(popularity))");
+ query.setRequestHandler("/query");
+ query.setParam("rows", "8");
+ query.setFields("id,features:[fv]");
+ query.add("rq", "{!ltr model=powpularityS-model reRankDocs=8}");
+
+ QueryResponse queryResponse = solrCluster.getSolrClient().query(COLLECTION,query);
+
+ Map<String, List<List<String>>> loadedFields = ObservingPrefetchingFieldValueFeature.loadedFields;
+
+ for (SolrDocument doc : queryResponse.getResults()) {
+ String docId = (String) doc.getFirstValue("id");
+ if (docId.equals("1")) {
+ assertEquals(NUM_FEATURES, loadedFields.get(docId).stream()
+ // doc with id 1 has no values set for 3 of the 6 feature fields
+ .filter(fieldLoadedList -> fieldLoadedList.size() == NUM_FEATURES - 3)
+ .count());
+ } else {
+ // all the fields were loaded at once
+ assertEquals(NUM_FEATURES, loadedFields.get(docId).stream()
+ .filter(fieldLoadedList -> fieldLoadedList.size() == NUM_FEATURES)
+ .count());
+ }
+ }
+ assertTheResponse(queryResponse);
+ }
+
+ @Test
+ public void testSimpleQueryBreakPrefetching() throws Exception {
+ ObservingPrefetchingFieldValueFeature.setBreakPrefetching(true);
+ ObservingPrefetchingFieldValueFeature.loadedFields = new HashMap<>();
+ System.setProperty(LAZY_FIELD_LOADING_CONFIG_KEY, "false");
+ // needed to clear cache because we make assertions on its content
+ reloadCollection(COLLECTION);
+
+ SolrQuery query = new SolrQuery("{!func}sub(8,field(popularity))");
+ query.setRequestHandler("/query");
+ query.setParam("rows", "8");
+ query.setFields("id,features:[fv]");
+ query.add("rq", "{!ltr model=powpularityS-model reRankDocs=8}");
+
+ QueryResponse queryResponse =
+ solrCluster.getSolrClient().query(COLLECTION,query);
+
+ Map<String, List<List<String>>> loadedFields = ObservingPrefetchingFieldValueFeature.loadedFields;
+
+ assertEquals(loadedFields.size(), queryResponse.getResults().size());
+ for (SolrDocument doc : queryResponse.getResults()) {
+ String docId = (String) doc.getFirstValue("id");
+ if (docId.equals("1")) {
+ // doc with id 1 has no values set for 3 of the 6 feature fields
+ assertEquals(NUM_FEATURES - 3, loadedFields.get(docId).stream()
+ .filter(fieldLoadedList -> fieldLoadedList.size() == 1)
+ .count());
+ } else {
+ // each single field used for a feature gets loaded separately
+ assertEquals(NUM_FEATURES, loadedFields.get(docId).stream()
+ .filter(fieldLoadedList -> fieldLoadedList.size() == 1)
+ .count());
+ }
+ }
+ assertTheResponse(queryResponse);
+ }
+
+ @Test
+ public void testSimpleQueryLazyBreakPrefetching() throws Exception {
Review comment:
I hope that the commit to make the tests more comprehensible ( 2d355c7 ) helps with identifying overlap.
The `BreakPrefetching` and `DisablePrefetching` tests are still very similar. They are not intended to test different behavior but rather that different configurations lead to the same behavior.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r656402593
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/LTRScoringQuery.java
##########
@@ -223,6 +223,7 @@ public ModelWeight createWeight(IndexSearcher searcher, ScoreMode scoreMode, flo
else{
createWeightsParallel(searcher, scoreMode.needsScores(), featureWeights, features);
}
+ // TODO: potential prefetchFields computation site?
Review comment:
> ... Also, I did a few small changes to fix the tests ...
Thanks! I'm not sure if the `setField` and `setIndex` calls in the `clone()` method were a key part to fix the tests but seeing them made me wonder if we need to give a clone method to the super class or declare the class(es) `Cloneable` since both field and index are not specific to the PFVF class.
> ... some renaming because the terms `frozen` and `freeze` made it a bit hard to understand.)
Yes, those terms were a bit generic. Upon further thought it seems that Java Collection's _unmodifiable_ concept is a good match here -- https://github.com/apache/solr/pull/166/commits/69c6611c467b8129bd31ea7527f091f734d1e3b2 tries that out, 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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r655924437
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/LTRScoringQuery.java
##########
@@ -223,6 +223,7 @@ public ModelWeight createWeight(IndexSearcher searcher, ScoreMode scoreMode, flo
else{
createWeightsParallel(searcher, scoreMode.needsScores(), featureWeights, features);
}
+ // TODO: potential prefetchFields computation site?
Review comment:
Hi @cpoerschke ,
from the approaches that we discussed until now, I like this one best.
I committed your implementation in 7ac6e35.
(Also, I did a few small changes to fix the tests and some renaming because the terms `frozen` and `freeze` made it a bit hard to understand.)
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk edited a comment on pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk edited a comment on pull request #166:
URL: https://github.com/apache/solr/pull/166#issuecomment-855438132
Hi @cpoerschke ,
thank you for the quick response. :)
I addressed your suggestions that were quick to include and will look into the exception-handling / toString()-topics next.
> Not yet looked at the test changes
I am actually quite glad that you didn't, because I just changed the tests quite a lot.
Now there is quite a lot of duplicated code if you look at the two prefetching tests in `org.apache.solr.ltr` and the `TestLTROnSolrCloud`.
I already had a `TestLTROnSolrCloudBase`-class ready to extract the duplicates but was not sure whether to include such a change in this PR because it it would entail a few changes in `TestLTROnSolrCloud` which have nothing to do with the prefetching functionality.
I can make a commit tomorrow(?) only including this change and if you think that it would make the PR too complex we can just revert it.
**EDIT:** Link to the "restructure-commit" : 88d1897
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r646156817
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+
+ public void setField(String field) {
+ this.field = field;
+ }
Review comment:
> might actually be necessary if this class here were to end up delegating to the super class implementation where the super class implementation might use fieldAsSet
Although it does not break anything because the `PrefetchingFieldValueFeature` only delegates non-stored fields to the `FieldValueFeature` and the `fieldAsSet` is only used for stored fields, I removed it in the context of making the `field` private again. ( ab81d8b )
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk edited a comment on pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk edited a comment on pull request #166:
URL: https://github.com/apache/solr/pull/166#issuecomment-860246850
Hi @cpoerschke ,
rereading our conversation over the last two days made clear that we have quite some open constructions sites.
To not lose the overview about them (especially with a likely break from my side due to the upcoming Buzzwords) I want to put a summary of possible next steps and questions here:
- [ ] do we want to collect the prefetchFields after creating the weights instead of on the feature-level?
* concurrency vs. collecting more often
- [ ] remove the "disablePrefetchingFieldValueFeature" parameter
* add try-catch with single field to `score()`?
* if yes, then log error instead of warning?
- [X] use sorted set for prefetchFields ( d0520a9 )
- [ ] come to a decision regarding options to exclude / include fields
* do we want that?
* if yes, how (parameter in json / something per 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.
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
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r646156717
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+
+ public void setField(String field) {
+ this.field = field;
+ }
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @Override
+ public LinkedHashMap<String,Object> paramsToMap() {
+ final LinkedHashMap<String,Object> params = defaultParamsToMap();
+ params.put("field", field);
+ return params;
+ }
+
+ @Override
+ protected void validate() throws FeatureException {
+ if (field == null || field.isEmpty()) {
+ throw new FeatureException(getClass().getSimpleName()+
+ ": field must be provided");
+ }
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
Review comment:
I agree, that improves the readability. :) ( ab81d8b )
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r646152833
##########
File path: solr/contrib/ltr/src/test-files/solr/collection1/conf/solrconfig-ltr.xml
##########
@@ -34,6 +34,8 @@
initialSize="2048" autowarmCount="0" />
<cache name="QUERY_DOC_FV" class="solr.search.CaffeineCache" size="4096"
initialSize="2048" autowarmCount="4096" regenerator="solr.search.NoOpRegenerator" />
+ <!-- false is the default, just here to make it explizit in contrast to solrconfig-ltr-lazy-->
+ <enableLazyFieldLoading>false</enableLazyFieldLoading>
Review comment:
> is the enableLazyFieldLoading value the only difference between the solrconfig-ltr.xml and solrconfig-ltr-lazy.xml files?
Yes, and I do not see any more differences coming in the future.
I parameterized it like you suggested, this saves us a lot of unnecessary duplication. ( 0e55556 )
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r647621691
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+
+ public void setField(String field) {
+ this.field = field;
+ }
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @Override
+ public LinkedHashMap<String,Object> paramsToMap() {
+ final LinkedHashMap<String,Object> params = defaultParamsToMap();
+ params.put("field", field);
+ return params;
+ }
+
+ @Override
+ protected void validate() throws FeatureException {
+ if (field == null || field.isEmpty()) {
+ throw new FeatureException(getClass().getSimpleName()+
+ ": field must be provided");
+ }
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SchemaField schemaField;
+ private final SolrDocumentFetcher docFetcher;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+ if (searcher instanceof SolrIndexSearcher) {
+ schemaField = ((SolrIndexSearcher) searcher).getSchema().getFieldOrNull(field);
+ } else {
+ schemaField = null;
+ }
+ this.docFetcher = request.getSearcher().getDocFetcher();
Review comment:
If you think about a replacement like this:
```java
if (searcher instanceof SolrIndexSearcher) {
this.docFetcher = ((SolrIndexSearcher) searcher).getDocFetcher();
} else {
this.docFetcher = request.getSearcher().getDocFetcher();
}
```
the `request.getSearcher()` actually saves an if-else-block.
Even if `searcher` always was a `SolrIndexSearcher` (and equal to `request.getSearcher()`) while I was running tests, I do not know if there could be cases where this is not true, so I would not want to just cast it.
Thank you for calling attention to the possible irritation. I added a comment ( 67adb99 ).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r647723902
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/PrefetchingFieldValueFeature.java
##########
@@ -0,0 +1,149 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.ltr.feature;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.schema.SchemaField;
+import org.apache.solr.search.SolrDocumentFetcher;
+import org.apache.solr.search.SolrIndexSearcher;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This feature returns the value of a field in the current document.
+ * The field must have stored="true" or docValues="true" properties.
+ * Example configuration:
+ * <pre>{
+ "name": "rawHits",
+ "class": "org.apache.solr.ltr.feature.FieldValueFeature",
+ "params": {
+ "field": "hits"
+ }
+}</pre>
+ */
+public class PrefetchingFieldValueFeature extends FieldValueFeature {
+ // used to store all fields from all PrefetchingFieldValueFeatures
+ private Set<String> prefetchFields;
+
+ public void setField(String field) {
+ this.field = field;
+ }
+
+ public void setPrefetchFields(Set<String> fields) {
+ prefetchFields = fields;
+ }
+
+ @Override
+ public LinkedHashMap<String,Object> paramsToMap() {
+ final LinkedHashMap<String,Object> params = defaultParamsToMap();
+ params.put("field", field);
+ return params;
+ }
+
+ @Override
+ protected void validate() throws FeatureException {
+ if (field == null || field.isEmpty()) {
+ throw new FeatureException(getClass().getSimpleName()+
+ ": field must be provided");
+ }
+ }
+
+ public PrefetchingFieldValueFeature(String name, Map<String,Object> params) {
+ super(name, params);
+ }
+
+ @Override
+ public FeatureWeight createWeight(IndexSearcher searcher, boolean needsScores,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi)
+ throws IOException {
+ return new PrefetchingFieldValueFeatureWeight(searcher, request, originalQuery, efi);
+ }
+
+ @VisibleForTesting
+ public Set<String> getPrefetchFields(){
+ return prefetchFields;
+ }
+
+ public class PrefetchingFieldValueFeatureWeight extends FieldValueFeatureWeight {
+ private final SchemaField schemaField;
+ private final SolrDocumentFetcher docFetcher;
+
+ public PrefetchingFieldValueFeatureWeight(IndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) {
+ super(searcher, request, originalQuery, efi);
+ if (searcher instanceof SolrIndexSearcher) {
+ schemaField = ((SolrIndexSearcher) searcher).getSchema().getFieldOrNull(field);
+ } else {
+ schemaField = null;
+ }
+ this.docFetcher = request.getSearcher().getDocFetcher();
+ }
+
+ /**
+ * Return a FeatureScorer that work with stored fields and makes use of the cache if the configured field is stored
+ * and has no docValues.
+ * Otherwise, delegate the work to the FieldValueFeature.
+ *
+ * @param context the segment this FeatureScorer is working with
+ * @return FeatureScorer for the current segment and field
+ * @throws IOException as defined by abstract class Feature
+ */
+ @Override
+ public FeatureScorer scorer(LeafReaderContext context) throws IOException {
+ if (schemaField != null && !schemaField.stored() && schemaField.hasDocValues()) {
+ return super.scorer(context);
+ }
+ return new PrefetchingFieldValueFeatureScorer(this, context,
+ DocIdSetIterator.all(DocIdSetIterator.NO_MORE_DOCS));
+ }
+
+ /**
+ * A FeatureScorer that reads the stored value for a field
+ * docFetcher does not request a single field but all the prefetchFields to improve performance through caching
+ */
+ public class PrefetchingFieldValueFeatureScorer extends FieldValueFeatureScorer {
+
+ public PrefetchingFieldValueFeatureScorer(FeatureWeight weight,
+ LeafReaderContext context, DocIdSetIterator itr) {
+ super(weight, context, itr);
+ }
+
+ @Override
+ public float score() throws IOException {
+ try {
+ final Document document = docFetcher.doc(context.docBase + itr.docID(), prefetchFields);
+
+ return super.parseStoredFieldValue(document.getField(field));
+ } catch (final IOException e) {
+ throw new FeatureException(
+ e.toString() + ": " +
+ "Unable to extract feature for "
+ + name, e);
Review comment:
> overriding toString(String) or toString() might be something to consider
I am wondering whether it could also be an option to add the prefetchFields to `paramsToMap()`. I did not look into the side effects / benefits this might have 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.
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
[GitHub] [solr] tomglk commented on a change in pull request #166: SOLR-15450 add PrefetchingFieldValueFeature
Posted by GitBox <gi...@apache.org>.
tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r656487847
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/LTRScoringQuery.java
##########
@@ -223,6 +223,7 @@ public ModelWeight createWeight(IndexSearcher searcher, ScoreMode scoreMode, flo
else{
createWeightsParallel(searcher, scoreMode.needsScores(), featureWeights, features);
}
+ // TODO: potential prefetchFields computation site?
Review comment:
> can't ever happen, right? And so if it did happen then some very strange bug or problem presumably would have arisen
I think so too. That also made me struggle yesterday as I thought about testing.
Your arguments make sense to me. The additional complexity that e.g. the AtomicBoolean would provide also speaks against the use of the `keepFeaturesFinalAfterModelCreation`.
Maybe a comment could provide a bit more clarity for people who see the `prefetchFields = Collections.unmodifiableSortedSet(prefetchFields);` and a few lines below `prefetchFields.addAll(fields);`?
Regarding `Cloneable`:
I just experimented a bit with the `Cloneable` interface. It seems to only do a shallow copy which could be problematic with the prefetchFields. I used this code:
```java
public PrefetchingFieldValueFeature clone() throws CloneNotSupportedException {
return (PrefetchingFieldValueFeature) super.clone();
}
```
and made the PrefetchingFieldValueFeature implement Cloneable
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org