dsmiley commented on code in PR #4464:
URL: https://github.com/apache/solr/pull/4464#discussion_r3329240854
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -952,20 +947,35 @@ public int getFormUploadLimitKB() {
}
@Override
- public Map<String, Object> toMap(Map<String, Object> result) {
- if (znodeVersion > -1) result.put(ZNODEVER, znodeVersion);
- if (luceneMatchVersion != null)
- result.put(IndexSchema.LUCENE_MATCH_VERSION_PARAM,
luceneMatchVersion.toString());
- result.put("updateHandler", getUpdateHandlerInfo());
- Map<String, Object> m = new LinkedHashMap<>();
- result.put("query", m);
- m.put("useFilterForSortedQuery", useFilterForSortedQuery);
- m.put("queryResultWindowSize", queryResultWindowSize);
- m.put("queryResultMaxDocsCached", queryResultMaxDocsCached);
- m.put("enableLazyFieldLoading", enableLazyFieldLoading);
- m.put("maxBooleanClauses", booleanQueryMaxClauseCount);
- m.put(MIN_PREFIX_QUERY_TERM_LENGTH, prefixQueryMinPrefixLength);
+ public void writeMap(EntryWriter ew) throws IOException {
+ if (znodeVersion > -1) {
+ ew.put(ZNODEVER, znodeVersion);
+ }
+ if (luceneMatchVersion != null) {
+ ew.put(IndexSchema.LUCENE_MATCH_VERSION_PARAM,
luceneMatchVersion.toString());
+ }
+ ew.put("updateHandler", new SimpleOrderedMap<>(getUpdateHandlerInfo()));
Review Comment:
I suspect you could place the MapWriter directly into the value of
EntryWriter, without the SimpleOrderedMap copy. In the context of implementing
`writeMap` (as is done here), this should be universally true, I think.
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -982,45 +992,47 @@ public Map<String, Object> toMap(Map<String, Object>
result) {
overlay.getNamedPlugins(plugin.tag).entrySet()) {
items.put(e.getKey(), e.getValue());
}
- result.put(tag, items);
+ ew.put(
+ tag,
+ new SimpleOrderedMap<>(
+ m -> {
+ new NamedList<>(items).writeMap(m);
+ }));
} else {
if (plugin.options.contains(MULTI_OK)) {
- ArrayList<MapSerializable> l = new ArrayList<>();
- for (PluginInfo info : infos) l.add(info);
- result.put(tag, l);
+ ArrayList<MapWriter> writers = new ArrayList<>();
+ infos.forEach(info -> writers.add(new SimpleOrderedMap<>(info)));
Review Comment:
use of SimpleOrderedMap copy constructor / conversion seems needless
And same for other side of the if condition
##########
solr/core/src/java/org/apache/solr/search/CacheConfig.java:
##########
@@ -175,9 +177,8 @@ public SolrCache newInstance() {
}
@Override
- public Map<String, Object> toMap(Map<String, Object> argsMap) {
- // TODO: Should not create new HashMap?
- return new HashMap<>(args);
+ public void writeMap(EntryWriter ew) throws IOException {
+ new NamedList<>(args).writeMap(ew);
Review Comment:
surely we can do better (no NamedList creatoin). note the comment that was
deleted -- same concept.
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -982,45 +992,47 @@ public Map<String, Object> toMap(Map<String, Object>
result) {
overlay.getNamedPlugins(plugin.tag).entrySet()) {
items.put(e.getKey(), e.getValue());
}
- result.put(tag, items);
+ ew.put(
+ tag,
+ new SimpleOrderedMap<>(
+ m -> {
+ new NamedList<>(items).writeMap(m);
+ }));
} else {
if (plugin.options.contains(MULTI_OK)) {
- ArrayList<MapSerializable> l = new ArrayList<>();
- for (PluginInfo info : infos) l.add(info);
- result.put(tag, l);
+ ArrayList<MapWriter> writers = new ArrayList<>();
+ infos.forEach(info -> writers.add(new SimpleOrderedMap<>(info)));
+ ew.put(tag, writers);
} else {
- result.put(tag, infos.get(0));
+ ew.put(tag, new SimpleOrderedMap<>(m ->
infos.getFirst().writeMap(m)));
}
}
}
- addCacheConfig(
- m,
- filterCacheConfig,
- queryResultCacheConfig,
- documentCacheConfig,
- fieldValueCacheConfig,
- featureVectorCacheConfig);
- m = new LinkedHashMap<>();
- result.put("requestDispatcher", m);
- if (httpCachingConfig != null) m.put("httpCaching", httpCachingConfig);
- m.put(
- "requestParsers",
- Map.of(
- "multipartUploadLimitKB",
- multipartUploadLimitKB,
- "formUploadLimitKB",
- formUploadLimitKB));
- if (indexConfig != null) result.put("indexConfig", indexConfig);
-
- // TODO there is more to add
-
- return result;
+ ew.put(
+ "requestDispatcher",
+ new SimpleOrderedMap<>(
Review Comment:
The purpose of MapWriter is to write a map as efficiently as possible,
ideally without an intermediary forced map (or NamedList / SimpleOrderedMap or
other data structure conversions/copies). Prior to MapWriter, Solr used
MapSerializable which forced the creation of maps.
I'll skip repeating this point but it applies to many changes in this commit.
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -982,45 +992,47 @@ public Map<String, Object> toMap(Map<String, Object>
result) {
overlay.getNamedPlugins(plugin.tag).entrySet()) {
items.put(e.getKey(), e.getValue());
}
- result.put(tag, items);
+ ew.put(
+ tag,
+ new SimpleOrderedMap<>(
+ m -> {
+ new NamedList<>(items).writeMap(m);
+ }));
Review Comment:
can't this be simply `items` I think (as it was) -- why the double
conversion to NamedList and then to SimpleOrderedMap?
##########
solr/core/src/java/org/apache/solr/schema/IndexSchema.java:
##########
@@ -1699,7 +1698,7 @@ public Map<String, Object> toMap(Map<String, Object> map)
{
SchemaProps.Handler::getNameLower,
SchemaProps.Handler::getRealName));
public Map<String, Object> getNamedPropertyValues(String name, SolrParams
params) {
- return new SchemaProps(name, params, this).toMap(new LinkedHashMap<>());
+ return new SimpleOrderedMap<>(new SchemaProps(name, params, this));
Review Comment:
Use LinkedHashMap here as this method isn't apparently isolated for
serialization
##########
solr/core/src/java/org/apache/solr/core/PluginInfo.java:
##########
@@ -192,27 +194,35 @@ public PluginInfo getChild(String type) {
}
@Override
- @SuppressWarnings({"unchecked", "rawtypes"})
- public Map<String, Object> toMap(Map<String, Object> map) {
- map.putAll(attributes);
- Map m = map;
- if (initArgs != null) m.putAll(initArgs.asMap(3));
- if (children != null) {
- for (PluginInfo child : children) {
- Object old = m.get(child.name);
- if (old == null) {
- m.put(child.name, child.toMap(new LinkedHashMap<>()));
- } else if (old instanceof List list) {
- list.add(child.toMap(new LinkedHashMap<>()));
- } else {
- ArrayList l = new ArrayList();
- l.add(old);
- l.add(child.toMap(new LinkedHashMap<>()));
- m.put(child.name, l);
- }
+ @SuppressWarnings("unchecked")
+ public void writeMap(EntryWriter ew) throws IOException {
+ new NamedList<>(attributes).writeMap(ew);
+ if (initArgs != null) {
+ for (Map.Entry<String, Object> entry : initArgs.asMap(3).entrySet()) {
+ ew.put(entry.getKey(), entry.getValue());
+ }
+ }
+ if (children == null || children.isEmpty()) {
+ return;
+ }
+
+ Map<String, Object> childrenGrouped = new LinkedHashMap<>();
+ for (PluginInfo child : children) {
+ Object old = childrenGrouped.get(child.name);
+ if (old == null) {
+ childrenGrouped.put(child.name, child);
+ } else if (old instanceof List) {
+ ((List<Object>) old).add(child);
Review Comment:
as it was before, can use Java 17 instanceof auto cast to 'list" here
##########
solr/core/src/java/org/apache/solr/core/SolrConfig.java:
##########
@@ -982,45 +992,47 @@ public Map<String, Object> toMap(Map<String, Object>
result) {
overlay.getNamedPlugins(plugin.tag).entrySet()) {
items.put(e.getKey(), e.getValue());
}
- result.put(tag, items);
+ ew.put(
+ tag,
+ new SimpleOrderedMap<>(
+ m -> {
+ new NamedList<>(items).writeMap(m);
+ }));
} else {
if (plugin.options.contains(MULTI_OK)) {
- ArrayList<MapSerializable> l = new ArrayList<>();
- for (PluginInfo info : infos) l.add(info);
- result.put(tag, l);
+ ArrayList<MapWriter> writers = new ArrayList<>();
+ infos.forEach(info -> writers.add(new SimpleOrderedMap<>(info)));
+ ew.put(tag, writers);
} else {
- result.put(tag, infos.get(0));
+ ew.put(tag, new SimpleOrderedMap<>(m ->
infos.getFirst().writeMap(m)));
Review Comment:
can't be as before?
##########
solr/core/src/java/org/apache/solr/update/SolrIndexConfig.java:
##########
@@ -195,28 +195,19 @@ public SolrIndexConfig(ConfigNode cfg, SolrIndexConfig
def) {
}
@Override
- public Map<String, Object> toMap(Map<String, Object> map) {
- map.put("useCompoundFile", useCompoundFile);
- map.put("maxBufferedDocs", maxBufferedDocs);
- map.put("ramBufferSizeMB", ramBufferSizeMB);
- map.put("ramPerThreadHardLimitMB", ramPerThreadHardLimitMB);
- map.put("maxCommitMergeWaitTime", maxCommitMergeWaitMillis);
- map.put("writeLockTimeout", writeLockTimeout);
- map.put("lockType", lockType);
- map.put("infoStreamEnabled", infoStream != InfoStream.NO_OUTPUT);
- if (mergeSchedulerInfo != null) {
- map.put("mergeScheduler", mergeSchedulerInfo);
- }
- if (metricsInfo != null) {
- map.put("metrics", metricsInfo);
- }
- if (mergePolicyFactoryInfo != null) {
- map.put("mergePolicyFactory", mergePolicyFactoryInfo);
- }
- if (mergedSegmentWarmerInfo != null) {
- map.put("mergedSegmentWarmer", mergedSegmentWarmerInfo);
- }
- return map;
+ public void writeMap(EntryWriter ew) throws IOException {
+ ew.put("useCompoundFile", useCompoundFile)
+ .put("maxBufferedDocs", maxBufferedDocs)
+ .put("ramBufferSizeMB", ramBufferSizeMB)
+ .put("ramPerThreadHardLimitMB", ramPerThreadHardLimitMB)
+ .put("maxCommitMergeWaitTime", maxCommitMergeWaitMillis)
+ .put("writeLockTimeout", writeLockTimeout)
+ .put("lockType", lockType)
+ .put("infoStreamEnabled", infoStream != InfoStream.NO_OUTPUT)
+ .putIfNotNull("mergeScheduler", mergeSchedulerInfo)
+ .putIfNotNull("metrics", metricsInfo)
+ .putIfNotNull("mergePolicyFactory", mergePolicyFactoryInfo)
+ .putIfNotNull("mergedSegmentWarmer", mergedSegmentWarmerInfo);
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]
