magibney commented on a change in pull request #513:
URL: https://github.com/apache/solr/pull/513#discussion_r789162650
##########
File path:
solr/core/src/java/org/apache/solr/response/transform/GeoTransformerFactory.java
##########
@@ -144,88 +148,111 @@ public void transform(SolrDocument doc, int docid)
throws IOException {
}
+ // if source has been renamed, update reference
Review comment:
(I just pushed commits addressing all other feedback but this
subclass/interface question -- oh, and did also add tests to stress the
renaming issue, as I suggested immediately above)
wrt the subclass/interface question: on further reflection, unfortunately I
think it might just make things more convoluted (and less flexible) to try to
factor out the "renaming" logic, as opposed to (as currently implemented)
delegating that responsibility to each class that implements `FieldRenamer` (at
a minimum each DocTransformer needs to parse its own SolrParams to determine
"source" field, and the GeoTransformer in some cases _doesn't_ rename a field
(instead retrieving source values directly from docValues) -- and once you add
hooks for both those cases, it's unclear that anything is really being
simplified).
Unless you feel strongly about it, I'm also inclined to leave this as an
interface, because I don't think there's any direct benefit (in terms of
factoring out code) to making it a subclass, and although the subclass approach
would clarify somewhat the main use of this type, it would also needlessly
prevent users from extending existing `TransformerFactory` classes to add
support for field renaming. This might be kind of an edge case, but absent a
strong preference for strict subclass, I'll probably stick with the flexibility
of an interface-based 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.
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]
