sonatype-lift[bot] commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r789235002
##########
File path: solr/core/src/java/org/apache/solr/search/SolrReturnFields.java
##########
@@ -168,32 +174,26 @@ private void parseFieldList(String[] fl, SolrQueryRequest
req) {
return;
}
- NamedList<String> rename = new NamedList<>();
+ Deque<DeferredRenameEntry> deferredRenameAugmenters = new LinkedList<>();
Review comment:
*JdkObsolete:* It is very rare for LinkedList to out-perform ArrayList
or ArrayDeque. Avoid it unless you're willing to invest a lot of time into
benchmarking. Caveat: LinkedList supports null elements, but ArrayDeque does
not. [(details)](https://errorprone.info/bugpattern/JdkObsolete)
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with
`help` or `ignore`)
##########
File path:
solr/core/src/java/org/apache/solr/response/transform/TransformerFactory.java
##########
@@ -40,6 +41,62 @@ public void init(NamedList<?> args) {
public abstract DocTransformer create(String field, SolrParams params,
SolrQueryRequest req);
+ /**
+ * The {@link FieldRenamer} interface should be implemented by any {@link
TransformerFactory} capable of generating
+ * transformers that might rename fields, and should implement {@link
#create(String, SolrParams, SolrQueryRequest, Map, Set)}
+ * in place of {@link #create(String, SolrParams, SolrQueryRequest)} (with
the latter method
+ * overridden to throw {@link UnsupportedOperationException}).
+ *
+ * {@link DocTransformer}s returned via {@link #create(String, SolrParams,
SolrQueryRequest, Map, Set)}
+ * will be added in a second pass, allowing simplified logic in {@link
TransformerFactory#create(String, SolrParams, SolrQueryRequest)}
+ * for non-renaming factories.
+ *
+ * {@link #create(String, SolrParams, SolrQueryRequest, Map, Set)} must
implement extra logic to be aware of
+ * preceding field renames, and to make subsequent {@link FieldRenamer}
transformers aware of its own field renames.
+ *
+ * It is harmless for a {@link DocTransformer} that does _not_ in practice
rename fields to be returned from a
+ * factory that implements this interface (e.g., for conditional renames?);
but doing so opens the possibility of
+ * {@link #create(String, SolrParams, SolrQueryRequest, Map, Set)} being
called _after_ fields have been renamed,
+ * so such implementations must still check whether the field with which
they are concerned has been renamed ...
+ * and if it _has_, must copy the field back to its original name. This
situation also demonstrates the
+ * motivation for separating the creation of {@link DocTransformer}s into
two phases: an initial phase involving
+ * no field renames, and a subsequent phase that implement extra logic to
properly handle field renames.
+ */
+ public interface FieldRenamer {
+ // TODO: Behavior is undefined in the event of a "destination field"
collision (e.g., a user maps two fields to
+ // the same "destination field", or maps a field to a top-level requested
field). In the future, the easiest way
+ // to detect such a case would be by "failing fast" upon renaming to a
field that already has an associated value,
+ // or support for this feature could be expressly added via a hypothetical
+ // `combined_field:[consolidate fl=field_1,field_2]` transformer.
+ /**
+ * Analogous to {@link TransformerFactory#create(String, SolrParams,
SolrQueryRequest)}, but to be implemented
+ * by {@link TransformerFactory}s that produce {@link DocTransformer}s
that may rename fields.
+ *
+ * @param field The destination field
+ * @param params Local params associated with this transformer (e.g.,
source field)
+ * @param req The current request
+ * @param renamedFields Maps source=>dest renamed fields.
Implementations should check this first, updating
+ * their own "source" field(s) as necessary, and if
renaming (not copying) fields, should
+ * also update this map with the implementations
"own" introduced source=>dest field
+ * mapping
+ * @param reqFieldNames Set of explicitly requested field names;
implementations should consult this set to
+ * determine whether it's appropriate to rename (vs.
copy) a field (e.g.: <code>boolean
+ * copy = reqFieldNames != null &&
reqFieldNames.contains(sourceField)</code>)
+ * @return A transformer to be used in processing field values in returned
documents.
+ */
+ DocTransformer create(String field, SolrParams params, SolrQueryRequest
req, Map<String, String> renamedFields, Set<String> reqFieldNames);
+
+ /**
+ * @return <code>true</code> if implementations of this class may (even
subtly) modify field values.
Review comment:
*MissingSummary:* A summary fragment is required; consider using the
value of the @return block as a summary fragment instead.
[(details)](https://google.github.io/styleguide/javaguide.html#s7.2-summary-fragment)
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with
`help` or `ignore`)
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
