kotman12 commented on code in PR #5188:
URL: https://github.com/apache/solr/pull/5188#discussion_r4360971566
##########
solr/core/src/test/org/apache/solr/search/ReturnFieldsTest.java:
##########
@@ -465,7 +505,7 @@ public void testWhiteboxSolrDocumentConversion() {
new SolrReturnFields(req()),
new SolrReturnFields(req("fl", "*")),
new SolrReturnFields(req("fl", "*,score")),
- new SolrReturnFields(req("fl", "id,subword,uniq,foo_*,store_*")),
+ new SolrReturnFields(req("fl", "id,subword,uniq,foo_*,store*")),
Review Comment:
Curious how this passed before?
##########
solr/core/src/java/org/apache/solr/search/SolrDocumentFetcher.java:
##########
@@ -875,8 +875,10 @@ private Set<String>
calcDocValueFieldsForReturn(ReturnFields returnFields) {
}
}
} else if (returnFields.hasPatternMatching()) {
- for (String s : getNonStoredDVs(true)) {
- if (returnFields.wantsField(s)) {
+ for (String s : getNonStoredDVs(false)) {
Review Comment:
Out of scope but I don't understand why this function is parameterized.
AFAICT it only gets invoked with compile-time constant `true` or `false`. It
makes it really hard to follow. I'd prefer if it were separately
`nonStoredDVs()` and `useAsStoredDVs()`. The whole "non-stored-use-as-stored"
is a mind-bend on its own and this makes it even more opaque.
##########
solr/core/src/java/org/apache/solr/search/ReturnFields.java:
##########
@@ -104,6 +104,20 @@ public Set<String> getNonScoreDependentReturnFieldNames() {
*/
public abstract boolean wantsField(String name);
+ /**
+ * Returns <code>true</code> if the value of the specified index field is
needed, either to return
+ * or for use by transformers. This is <code>false</code> for a field that
only shares its name
+ * with a pseudo field (e.g. an alias or a transformer), unless
<code>*</code> was requested.
+ */
+ public abstract boolean wantsLuceneField(String field);
Review Comment:
Just reading `wantsLuceneField` it is hard to see how that encodes: "index
field to return or for use by transformer". `wantsField` OTOH is just about
what gets returned. So the difference between these two is just the word
`Lucene` which misses the important part which is these fields need to be
read/loaded for further use even if they aren't returned. Perhaps
`usesLuceneField`? My preference actually would be to drop `Lucene` altogether,
i.e.`uses(Index|Real|Source)Field` but I can see `Lucene` is already used in
`SolrReturnFields` so this is good for consistency.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
