cpoerschke commented on a change in pull request #243:
URL: https://github.com/apache/solr/pull/243#discussion_r686186391
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/Tuple.java
##########
@@ -275,15 +295,32 @@ public void setMetrics(Map<String, Map<?,?>> metrics) {
}
public Tuple clone() {
- Tuple clone = new Tuple();
- clone.fields.putAll(fields);
- // TODO This doesn't copy EOF/Exception
https://issues.apache.org/jira/browse/SOLR-15480
+ Tuple clone = new Tuple(this.fields);
+ if (this.fieldNames != null) {
+ clone.fieldNames = new ArrayList<>(this.fieldNames);
+ }
+ if (this.fieldLabels != null) {
+ clone.fieldLabels = new HashMap<>(this.fieldLabels);
+ }
Review comment:
1 of 3: I wonder if the copy constructor could be used here?
```suggestion
Tuple clone = new Tuple(this);
```
##########
File path: solr/CHANGES.txt
##########
@@ -317,6 +317,11 @@ Other Changes
* SOLR-15385: Address many rawtypes warnings, resulting in several modified
signatures in the public API. (Mike Drob, David Smiley, Christine Poerschke)
+* SOLR-15480: Added a new putAll methods for Tuple.fields to take care of
marking EOF and EXCEPTION when those tokens are found.
+ The copy constructor (taking only fields), the merge, and the clone methods
were updated to use the new putAll method. Additionally,
+ a new copy constructor taking another Tuple object was added and it, along
with clone and merge, were refactored to handle setting
+ fieldNames and fieldLabels if necessary. (John Durham)
+
Review comment:
3 of 3: Assuming we're targetting the `8.10.0` release (I think we are,
@madrob do you agree?) let's relocate this into its section (ca. line 350
onwards below) and categorise it under "Bug Fixes" rather "Other Changes"
section.
The description also seems rather long, how about something shorter e.g.
_"Make Tuple copy constructor, clone and merge consistent w.r.t. markers (EOF,
EXCEPTION), field names and labels. (John Durham via Mike Drob, Christine
Poerschke)"_ i.e. focus on the outcome and omission of the implementation
details?
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/Tuple.java
##########
@@ -275,15 +295,32 @@ public void setMetrics(Map<String, Map<?,?>> metrics) {
}
public Tuple clone() {
- Tuple clone = new Tuple();
- clone.fields.putAll(fields);
- // TODO This doesn't copy EOF/Exception
https://issues.apache.org/jira/browse/SOLR-15480
+ Tuple clone = new Tuple(this.fields);
+ if (this.fieldNames != null) {
+ clone.fieldNames = new ArrayList<>(this.fieldNames);
+ }
+ if (this.fieldLabels != null) {
+ clone.fieldLabels = new HashMap<>(this.fieldLabels);
+ }
return clone;
}
public void merge(Tuple other) {
- // TODO This doesn't copy EOF/Exception
https://issues.apache.org/jira/browse/SOLR-15480
- fields.putAll(other.getFields());
+ this.putAll(other.getFields());
+ if (other.fieldNames != null) {
+ if (this.fieldNames != null) {
+ this.fieldNames.addAll(other.fieldNames);
+ } else {
+ this.fieldNames = new ArrayList<>(other.fieldNames);
+ }
+ }
+ if (other.fieldLabels != null) {
+ if (this.fieldLabels != null) {
+ this.fieldLabels.putAll(other.fieldLabels);
+ } else {
+ this.fieldLabels = new HashMap<>(other.fieldLabels);
Review comment:
2b of 3: similar to 2a above, for a copy-constructor there are (perhaps)
expectations w.r.t. preservation of map choice, for a `merge` operator such
expectations may not exist or they may be different though it might be nicely
consistent to preserve map choice where possible. What do you think?
##########
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:
2a of 3: So based on existing usage within `solr/main` code base
`HashMap` is the map of choice, though if some other map was chosen e.g. in
custom code or future `solr/main` code then copying here would potentially
change a `SortedMap` to one that is not. Though I'm a little unclear on whether
or not that might be "okay" for a copy-constructor. I wonder if
`fieldLabels.clone` might be an alternative given that it's a String-to-String
map?
--
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]
