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 "source" 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 "source" 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: [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]
