dsmiley commented on code in PR #4465:
URL: https://github.com/apache/solr/pull/4465#discussion_r3404208569
##########
solr/solrj/src/java/org/apache/solr/common/cloud/ZkNodeProps.java:
##########
@@ -42,7 +43,7 @@ public ZkNodeProps(Map<String, Object> propMap) {
}
public ZkNodeProps(MapWriter mw) {
- propMap = mw.toMap(new HashMap<>());
+ propMap = new SimpleOrderedMap<>(mw);
Review Comment:
SOM should only be used on the request writing code path. ZkNodeProps isn't
that. Just use `Utils.convertToMap(new HashMap())` here.
##########
solr/modules/extraction/src/java/org/apache/solr/handler/extraction/TikaServerExtractionBackend.java:
##########
@@ -99,7 +100,7 @@ public TikaServerExtractionBackend(
this.maxCharsLimit = maxCharsLimit;
if (initArgs != null) {
- initArgs.toMap(this.initArgsMap);
+ initArgsMap.putAll(new SimpleOrderedMap<>(initArgs));
Review Comment:
Again, I'm seeing needless data structure copying. Can you do a ~1 liner
initArgs.forEach that calls put on initArgsMap
##########
solr/solrj/src/java/org/apache/solr/common/util/Utils.java:
##########
@@ -178,7 +178,7 @@ public MapWriter.EntryWriter put(CharSequence k, Object v) {
private static Object makeDeepCopy(Object v, int maxDepth, boolean mutable,
boolean sorted) {
if (v instanceof MapWriter && maxDepth > 1) {
- v = ((MapWriter) v).toMap(new LinkedHashMap<>());
+ v = new SimpleOrderedMap<>((MapWriter) v);
Review Comment:
don't use SimpleOrderedMap except in request serialization.
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/expr/StreamExplanation.java:
##########
@@ -58,17 +57,12 @@ public void addChild(Explanation child) {
}
@Override
- public Map<String, Object> toMap(Map<String, Object> map) {
- map = super.toMap(map);
-
- if (null != children && 0 != children.size()) {
- List<Map<String, Object>> childrenMaps = new ArrayList<>();
+ public void writeMap(EntryWriter ew) throws IOException {
Review Comment:
couldn't you just do the `.putIfNotNull(` on "children" like you did
similarly in the Explanation class?
##########
solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java:
##########
@@ -428,11 +427,6 @@ public boolean writeKnownType(Object val) throws
IOException {
writeMapEntry((Map.Entry) val);
return true;
}
- if (val instanceof MapSerializable) {
Review Comment:
it's strange that you are including this in this PR instead of the
MapSerializable removal PR
##########
solr/solrj/src/java/org/apache/solr/common/IteratorWriter.java:
##########
@@ -76,7 +76,7 @@ default List<Object> toList(List<Object> l) {
new ItemWriter() {
@Override
public ItemWriter add(Object o) throws IOException {
- if (o instanceof MapWriter) o = ((MapWriter) o).toMap(new
LinkedHashMap<>());
+ if (o instanceof MapWriter) o = new
SimpleOrderedMap<>((MapWriter) o);
Review Comment:
I feel like we need a static method on SimpleOrderedMap called maybe "wrap"
or "as" or "from" that either casts or creates a new SOM... since the MapWriter
may itself be a SimpleOrderedMap. Any way... feel free to defer/not-do
##########
solr/solrj/src/java/org/apache/solr/common/util/TextWriter.java:
##########
@@ -89,9 +87,6 @@ default void writeVal(String name, Object val, boolean raw)
throws IOException {
writeMap(name, (MapWriter) val);
} else if (val instanceof ReflectWritable) {
writeVal(name, Utils.getReflectWriter(val));
- } else if (val instanceof MapSerializable) {
Review Comment:
same; IMO doesn't belong in this PR
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/expr/Explanation.java:
##########
@@ -139,35 +138,14 @@ public void addHelper(Explanation helper) {
}
@Override
- public Map<String, Object> toMap(Map<String, Object> map) {
- if (null != expressionNodeId) {
- map.put("expressionNodeId", expressionNodeId);
- }
- if (null != expressionType) {
- map.put("expressionType", expressionType);
- }
- if (null != functionName) {
- map.put("functionName", functionName);
- }
- if (null != implementingClass) {
- map.put("implementingClass", implementingClass);
- }
- if (null != expression) {
- map.put("expression", expression);
- }
- if (null != note) {
- map.put("note", note);
- }
-
- if (null != helpers && 0 != helpers.size()) {
- List<Map<String, Object>> helperMaps = new ArrayList<>();
- for (Explanation helper : helpers) {
- helperMaps.add(helper.toMap(new LinkedHashMap<>()));
- }
- map.put("helpers", helperMaps);
- }
-
- return map;
+ public void writeMap(EntryWriter ew) throws IOException {
Review Comment:
nice
--
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]
