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: 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