cpoerschke commented on a change in pull request #243: URL: https://github.com/apache/solr/pull/243#discussion_r687023177
########## File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/Tuple.java ########## @@ -87,10 +87,20 @@ public Tuple(String k1, Object v1, String k2, Object v2) { * @param fields map containing keys and values to be copied to this tuple */ public Tuple(Map<String, ?> fields) { - // TODO Use bulk putAll operation that will properly size the map - // https://issues.apache.org/jira/browse/SOLR-15480 - for (Map.Entry<String, ?> entry : fields.entrySet()) { - put(entry.getKey(), entry.getValue()); + putAll(fields); + } + + /** + * A copy constructor + * @param original Tuple that will be copied + */ + public Tuple(Tuple original) { + this.putAll(original.fields); + if (original.fieldNames != null) { + this.fieldNames = new ArrayList<>(original.fieldNames); + } + if (original.fieldLabels != null) { + this.fieldLabels = new HashMap<>(original.fieldLabels); Review comment: > ... The same could be said for `Tuple.fieldNames` as well since its written as `List<String>`, so a caller may not expect their implementation type to change either. ... Good point. > ... My initial thought is to do some kind of reflection to get the constructors of the given types and use those to instantiate the clones, but that doesn't seem quite right to me. ... Does that seem like the right approach, or would there be a better way to handle this? Thanks for looking into! Good questions. I share your "doesn't seem quite right" sentiment. Okay, maybe taking a step back and/or looking slightly sideways at things might move us forward here, thinking out aloud: * What does it mean to "merge" one tuple into another? * Is it meant to be the fields only (current implementation) or should field labels and names be included? * What if there is overlap between the two field labels lists? Should the resulting list have duplicates or should only not-yet-present elements be added? * What if there is overlap between the two field names maps? Does the caller intend for the "source" tuple (`other`) to override the content of the "destination" tuple (`this`) or should the "destination" prevail or should some sort of "renaming" happen? * If (and that is an if) field labels and names are _not_ to be included then a "fix" could be simply to document that is the intended behaviour and anyone who needs some other behaviour, well, they may call the setter methods for the field labels and field names. * If the "merge" method code did not change then the question of field labels and field names container type preservation does not arise for it (in the scenario where the "destination" tuple does not have a container as yet). * If the "merge" method is documented as having only `putAll` behaviour then one could also argue that callers might as well use `putAll` directly i.e. deprecation and eventual removal of "merge" as a way to "fix" it. * What does it mean to "clone" a tuple? * Cloning including field labels and field names, that seems uncontroversial. * Cloning preserving field label and field names container type, or not preserving it, that is the question. * The use of field labels and field names seems relatively rare. * To help with code base reading I've opened #256 and #257 to remove deprecated things. -- 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