You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "risdenk (via GitHub)" <gi...@apache.org> on 2023/03/22 19:59:07 UTC
[GitHub] [solr] risdenk opened a new pull request, #1482: SOLR-16715: Replace new HashMap(int) and new HashSet(int) with CollectionUtil.newHashMap / CollectionUtil.newHashSet
risdenk opened a new pull request, #1482:
URL: https://github.com/apache/solr/pull/1482
https://issues.apache.org/jira/browse/SOLR-16715
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1482: SOLR-16715: Replace new HashMap(int) and new HashSet(int) with CollectionUtil.newHashMap / CollectionUtil.newHashSet
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #1482:
URL: https://github.com/apache/solr/pull/1482#discussion_r1146210036
##########
solr/solrj/src/java/org/apache/solr/common/util/Utils.java:
##########
@@ -110,8 +110,8 @@ public static Map getDeepCopy(Map<?, ?> map, int maxDepth, boolean mutable, bool
} else {
copy =
map instanceof LinkedHashMap
- ? new LinkedHashMap<>(map.size())
- : new HashMap<>(map.size());
+ ? CollectionUtil.newLinkedHashMap(map.size())
Review Comment:
Hmm I see your point, but there are a variety of deepCopy methods that aren't all working on collections so I think it makes sense to leave them separate from CollectionUtil.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1482: SOLR-16715: Replace new HashMap(int) and new HashSet(int) with CollectionUtil.newHashMap / CollectionUtil.newHashSet
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #1482:
URL: https://github.com/apache/solr/pull/1482#discussion_r1146272227
##########
solr/core/src/java/org/apache/solr/request/SimpleFacets.java:
##########
@@ -595,7 +595,7 @@ private NamedList<Integer> getTermCounts(String field, Integer mincount, ParsedP
break;
case UIF:
// Emulate the JSON Faceting structure so we can use the same parsing classes
- Map<String, Object> jsonFacet = new HashMap<>(13);
+ Map<String, Object> jsonFacet = CollectionUtil.newHashMap(13);
Review Comment:
Hmmm well after 5cf9f86, Map.of causes a NPE since there are null keys :(
```
Caused by: java.lang.NullPointerException
2023-03-23T14:11:24.5115573Z 2> at java.util.Objects.requireNonNull(Objects.java:221) ~[?:?]
2023-03-23T14:11:24.5116312Z 2> at java.util.KeyValueHolder.<init>(KeyValueHolder.java:60) ~[?:?]
2023-03-23T14:11:24.5116875Z 2> at java.util.Map.entry(Map.java:1659) ~[?:?]
2023-03-23T14:11:24.5117685Z 2> at org.apache.solr.request.SimpleFacets.getTermCounts(SimpleFacets.java:617) ~[main/:?]
2023-03-23T14:11:24.5118298Z 2> at org.apache.solr.request.SimpleFacets.getTermCounts(SimpleFacets.java:425) ~[main/:?]
2023-03-23T14:11:24.5119110Z 2> at org.apache.solr.request.SimpleFacets.lambda$getFacetFieldCounts$0(SimpleFacets.java:911) ~[main/:?]
2023-03-23T14:11:24.5119635Z 2> ... 59 more
```
I'm going to revert that change just to simplify this.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1482: SOLR-16715: Replace new HashMap(int) and new HashSet(int) with CollectionUtil.newHashMap / CollectionUtil.newHashSet
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #1482:
URL: https://github.com/apache/solr/pull/1482#discussion_r1146250980
##########
solr/solrj/src/java/org/apache/solr/common/util/CollectionUtil.java:
##########
@@ -39,6 +41,17 @@ public static <K, V> HashMap<K, V> newHashMap(int size) {
return new HashMap<>((int) (size / 0.75f) + 1);
}
+ /**
+ * Returns a new {@link LinkedHashMap} sized to contain {@code size} items without resizing the
+ * internal array.
+ */
+ public static <K, V> LinkedHashMap<K, V> newLinkedHashMap(int size) {
+ // With Lucene 9.5 - we should replace this with
Review Comment:
Fixed in 33f0c54372f5933c59023d71b33ac903dadd438e
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1482: SOLR-16715: Replace new HashMap(int) and new HashSet(int) with CollectionUtil.newHashMap / CollectionUtil.newHashSet
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #1482:
URL: https://github.com/apache/solr/pull/1482#discussion_r1146272849
##########
solr/core/src/java/org/apache/solr/request/SimpleFacets.java:
##########
@@ -595,7 +595,7 @@ private NamedList<Integer> getTermCounts(String field, Integer mincount, ParsedP
break;
case UIF:
// Emulate the JSON Faceting structure so we can use the same parsing classes
- Map<String, Object> jsonFacet = new HashMap<>(13);
+ Map<String, Object> jsonFacet = CollectionUtil.newHashMap(13);
Review Comment:
reverted in 380eb1364f66a9a86a126452e4100062e30614b5
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1482: SOLR-16715: Replace new HashMap(int) and new HashSet(int) with CollectionUtil.newHashMap / CollectionUtil.newHashSet
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #1482:
URL: https://github.com/apache/solr/pull/1482#discussion_r1145376884
##########
solr/solrj/src/java/org/apache/solr/common/util/CommandOperation.java:
##########
@@ -223,7 +223,7 @@ public static List<CommandOperation> parse(InputStream in, Set<String> singleton
List<CommandOperation> operations = new ArrayList<>();
final HashMap<Object, Object> map =
- new HashMap<>(0) {
+ new HashMap<>() {
Review Comment:
If we want to enforce the size 0 - this needs to move to an real class that we can add suppressforbidden to. We can't add suppressforbidden to an anonymous class.
##########
solr/core/src/java/org/apache/solr/update/UpdateLog.java:
##########
@@ -2389,4 +2386,19 @@ public void seedBucketsWithHighestVersion(SolrIndexSearcher newSearcher) {
versionInfo.unblockUpdates();
}
}
+
+ @SuppressForbidden(reason = "extends linkedhashmap")
Review Comment:
This was moved here since SuppressForbidden doesn't work on the anonymous class
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] bruno-roustant commented on a diff in pull request #1482: SOLR-16715: Replace new HashMap(int) and new HashSet(int) with CollectionUtil.newHashMap / CollectionUtil.newHashSet
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #1482:
URL: https://github.com/apache/solr/pull/1482#discussion_r1145997060
##########
gradle/validation/forbidden-apis/java.solr.txt:
##########
@@ -18,3 +18,12 @@ java.lang.Thread#<init>()
java.lang.Thread#<init>(java.lang.Runnable)
java.lang.Thread#<init>(java.lang.ThreadGroup,java.lang.Runnable)
+@defaultMessage Use CollectionUtil.newHashMap(int)
Review Comment:
+1
##########
solr/core/src/java/org/apache/solr/search/join/ScoreModeParser.java:
##########
@@ -29,7 +29,7 @@ class ScoreModeParser {
private ScoreModeParser() {}
private static Map<String, ScoreMode> getLowerAndCapitalCaseMap() {
- Map<String, ScoreMode> map = new HashMap<>(ScoreMode.values().length * 2);
+ Map<String, ScoreMode> map = CollectionUtil.newHashMap(ScoreMode.values().length * 2);
Review Comment:
I suspect here it should be newHashSet(ScoreMode.values().length) (the author doubled, knowing he/she was setting a map capacity)
##########
solr/core/src/java/org/apache/solr/search/facet/UniqueAgg.java:
##########
@@ -85,7 +85,7 @@ public void merge(Object facetResult, Context mcontext) {
List<?> vals = (List<?>) map.get(VALS);
if (vals != null) {
if (values == null) {
- values = new HashSet<>(vals.size() * 4);
+ values = CollectionUtil.newHashSet(vals.size() * 4);
Review Comment:
When I see * 4 or * 2, I wonder if the author knew it was a capacity and decided to double the expected size.
I mean, maybe here it should be newHashSet(vals.size() * 2)?
##########
solr/solrj/src/java/org/apache/solr/common/util/Utils.java:
##########
@@ -110,8 +110,8 @@ public static Map getDeepCopy(Map<?, ?> map, int maxDepth, boolean mutable, bool
} else {
copy =
map instanceof LinkedHashMap
- ? new LinkedHashMap<>(map.size())
- : new HashMap<>(map.size());
+ ? CollectionUtil.newLinkedHashMap(map.size())
Review Comment:
These methods in Utils seem very related to the new CollectionUtil, and I prefer the name CollectionUtil.
Are they often used? deepCopy does not seem to be a common use case. If not, I suggest you could move them to CollectionUtil (without too much refactoring impact on other classes).
##########
solr/solrj/src/java/org/apache/solr/common/util/CommandOperation.java:
##########
@@ -223,7 +223,7 @@ public static List<CommandOperation> parse(InputStream in, Set<String> singleton
List<CommandOperation> operations = new ArrayList<>();
final HashMap<Object, Object> map =
- new HashMap<>(0) {
+ new HashMap<>() {
Review Comment:
+1 to not keep 0. Anyway it is replaced internally by a power of 2.
##########
solr/solrj/src/java/org/apache/solr/common/util/CollectionUtil.java:
##########
@@ -39,6 +41,17 @@ public static <K, V> HashMap<K, V> newHashMap(int size) {
return new HashMap<>((int) (size / 0.75f) + 1);
}
+ /**
+ * Returns a new {@link LinkedHashMap} sized to contain {@code size} items without resizing the
+ * internal array.
+ */
+ public static <K, V> LinkedHashMap<K, V> newLinkedHashMap(int size) {
+ // With Lucene 9.5 - we should replace this with
Review Comment:
Comment to finish?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk merged pull request #1482: SOLR-16715: Replace new HashMap(int) and new HashSet(int) with CollectionUtil.newHashMap / CollectionUtil.newHashSet
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk merged PR #1482:
URL: https://github.com/apache/solr/pull/1482
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1482: SOLR-16715: Replace new HashMap(int) and new HashSet(int) with CollectionUtil.newHashMap / CollectionUtil.newHashSet
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1482:
URL: https://github.com/apache/solr/pull/1482#discussion_r1145571433
##########
solr/core/src/java/org/apache/solr/request/SimpleFacets.java:
##########
@@ -595,7 +595,7 @@ private NamedList<Integer> getTermCounts(String field, Integer mincount, ParsedP
break;
case UIF:
// Emulate the JSON Faceting structure so we can use the same parsing classes
- Map<String, Object> jsonFacet = new HashMap<>(13);
+ Map<String, Object> jsonFacet = CollectionUtil.newHashMap(13);
Review Comment:
Could be converted to Map.of if you want (just compute sortVal before). Don't have to if you don't want to.
##########
solr/core/src/java/org/apache/solr/handler/RequestHandlerUtils.java:
##########
@@ -134,9 +134,7 @@ public static boolean handleRollback(
public static void setWt(SolrQueryRequest req, String wt) {
SolrParams params = req.getParams();
if (params.get(CommonParams.WT) != null) return; // wt is set by user
- Map<String, String> map = new HashMap<>(1);
- map.put(CommonParams.WT, wt);
- map.put("indent", "true");
+ Map<String, String> map = Map.of(CommonParams.WT, wt, "indent", "true");
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1482: SOLR-16715: Replace new HashMap(int) and new HashSet(int) with CollectionUtil.newHashMap / CollectionUtil.newHashSet
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #1482:
URL: https://github.com/apache/solr/pull/1482#discussion_r1145629125
##########
solr/core/src/java/org/apache/solr/request/SimpleFacets.java:
##########
@@ -595,7 +595,7 @@ private NamedList<Integer> getTermCounts(String field, Integer mincount, ParsedP
break;
case UIF:
// Emulate the JSON Faceting structure so we can use the same parsing classes
- Map<String, Object> jsonFacet = new HashMap<>(13);
+ Map<String, Object> jsonFacet = CollectionUtil.newHashMap(13);
Review Comment:
Fixed in 5cf9f86eb8dce4627068ccc1b537ff68ce45873f. I think there are a handful of other places that could be a smarter conversion just didn't tackle it here.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on pull request #1482: SOLR-16715: Replace new HashMap(int) and new HashSet(int) with CollectionUtil.newHashMap / CollectionUtil.newHashSet
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on PR #1482:
URL: https://github.com/apache/solr/pull/1482#issuecomment-1481549484
Force pushed just to fix solr/CHANGES.txt conflict and merge didn't work w/ crave (see dev mailing list)
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on pull request #1482: SOLR-16715: Replace new HashMap(int) and new HashSet(int) with CollectionUtil.newHashMap / CollectionUtil.newHashSet
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on PR #1482:
URL: https://github.com/apache/solr/pull/1482#issuecomment-1481533770
planning to merge this tomorrow (don't want any test failures to get mixed up with https://github.com/apache/solr/pull/1478)
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1482: SOLR-16715: Replace new HashMap(int) and new HashSet(int) with CollectionUtil.newHashMap / CollectionUtil.newHashSet
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1482:
URL: https://github.com/apache/solr/pull/1482#discussion_r1146196421
##########
solr/core/src/java/org/apache/solr/search/facet/UniqueAgg.java:
##########
@@ -85,7 +85,7 @@ public void merge(Object facetResult, Context mcontext) {
List<?> vals = (List<?>) map.get(VALS);
if (vals != null) {
if (values == null) {
- values = new HashSet<>(vals.size() * 4);
+ values = CollectionUtil.newHashSet(vals.size() * 4);
Review Comment:
Maybe... but this change is still okay.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1482: SOLR-16715: Replace new HashMap(int) and new HashSet(int) with CollectionUtil.newHashMap / CollectionUtil.newHashSet
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #1482:
URL: https://github.com/apache/solr/pull/1482#discussion_r1146206689
##########
solr/core/src/java/org/apache/solr/search/facet/UniqueAgg.java:
##########
@@ -85,7 +85,7 @@ public void merge(Object facetResult, Context mcontext) {
List<?> vals = (List<?>) map.get(VALS);
if (vals != null) {
if (values == null) {
- values = new HashSet<>(vals.size() * 4);
+ values = CollectionUtil.newHashSet(vals.size() * 4);
Review Comment:
I agree that some of the other * some multiple seem suspicious. I'll take a look. at least in UniqueAgg, I don't know what the size should be at all. Its not clear :/ Its been *4 since 2015 when it was added so going to leave it for now.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1482: SOLR-16715: Replace new HashMap(int) and new HashSet(int) with CollectionUtil.newHashMap / CollectionUtil.newHashSet
Posted by "risdenk (via GitHub)" <gi...@apache.org>.
risdenk commented on code in PR #1482:
URL: https://github.com/apache/solr/pull/1482#discussion_r1146207813
##########
solr/core/src/java/org/apache/solr/search/join/ScoreModeParser.java:
##########
@@ -29,7 +29,7 @@ class ScoreModeParser {
private ScoreModeParser() {}
private static Map<String, ScoreMode> getLowerAndCapitalCaseMap() {
- Map<String, ScoreMode> map = new HashMap<>(ScoreMode.values().length * 2);
+ Map<String, ScoreMode> map = CollectionUtil.newHashMap(ScoreMode.values().length * 2);
Review Comment:
`*2` I think is correct here since for each value two items are put in the map:
```
map.put(s.name().toLowerCase(Locale.ROOT), s);
map.put(s.name(), s);
```
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org