Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
dsmiley merged PR #3048: URL: https://github.com/apache/solr/pull/3048 -- 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]
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
renatoh commented on PR #3048: URL: https://github.com/apache/solr/pull/3048#issuecomment-2627271485 > As you are a very new contributor, I can tell by your changes and your last question that you have not been running `./gradlew precommit`, which will detect issues that you can correct before pushing. > BTW all source files need a header. IntelliJ does it automatically for me, as I've configured it to. Ah yes, I've forgot about that one, was for some reason under the impression running the 'tidy' is sufficient, now I know:) -- 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]
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
dsmiley commented on PR #3048: URL: https://github.com/apache/solr/pull/3048#issuecomment-2625909437 As you are a very new contributor, I can tell by your changes and your last question that you have not been running `./gradlew precommit`, which will detect issues that you can correct before pushing. BTW all source files need a header. IntelliJ does it automatically for me, as I've configured it to. -- 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]
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
renatoh commented on PR #3048: URL: https://github.com/apache/solr/pull/3048#issuecomment-2625544960 > I removed the asShallowMap returning SimpleOrderedMap so such a decision can be in Solr 10. I intend on merging this when the tests pass and back porting to 9x. Today/tomorrow. Thanks again for your contribution! You are welcome, thanks for your support! Now it is failing with: Detected license header issues (skip with -Pvalidation.rat.failOnError=false): Unknown license: /home/runner/work/solr/solr/solr/solrj/src/test/org/apache/solr/common/util/SimpleOrderedMapTest.java No idea what this means -- 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]
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
dsmiley commented on PR #3048: URL: https://github.com/apache/solr/pull/3048#issuecomment-2625396992 I removed the asShallowMap returning SimpleOrderedMap so such a decision can be in Solr 10. I intend on merging this when the tests pass and back porting to 9x. Today/tomorrow. Thanks again for your contribution! -- 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]
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
renatoh commented on PR #3048: URL: https://github.com/apache/solr/pull/3048#issuecomment-2622942999 > have a slight concern if there's existing code relying on quirky sad behaviors of NamedList.asShallowMap having weird/sad implementations of `keySet`, `values`, `entrySet` because it calls `asMap(1)` on them. Had it been `asMap(0)`, probably lower/no concern. _Maybe_ this change should be in Solr 10 only. On the other hand... maybe it's an unlikely impact? Hard to say... right away I see SolrXmlConfig calling `.asShallowMap().entrySet()`. Isn't SolrXmlConfig always calling .asShallowMap on a NamedList, which is create org.apache.solr.common.ConfigNode#forEachChild? So the changes do not make a difference here, or do they? -- 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]
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
renatoh commented on PR #3048: URL: https://github.com/apache/solr/pull/3048#issuecomment-2622728797 > Just needs a CHANGES.txt entry. I've added that under Solr 9.9.0, is that correct? -- 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]
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
dsmiley commented on code in PR #3048:
URL: https://github.com/apache/solr/pull/3048#discussion_r1934300672
##
solr/solrj/src/java/org/apache/solr/common/util/NamedList.java:
##
@@ -238,6 +238,14 @@ public int indexOf(String name, int start) {
return -1;
}
+ /***
+ * Scans the list sequentially beginning at index 0
Review Comment:
nitpick: javadoc should include clarity that it's the index of the name.
It's not great to require looking at a parameter's name to disambiguate the
meaning of a method.
##
solr/solrj/src/java/org/apache/solr/common/util/NamedList.java:
##
@@ -403,8 +411,8 @@ public Map asShallowMap() {
return asShallowMap(false);
}
- public Map asShallowMap(boolean allowDps) {
-return new Map() {
+ private Map asShallowMap(boolean allowDps) {
Review Comment:
now's not the time to make private but feel free to deprecate.
##
solr/solrj/src/java/org/apache/solr/common/util/NamedList.java:
##
@@ -845,7 +853,7 @@ public void forEachKey(Consumer fun) {
}
}
- public void forEach(BiConsumer action) {
+ public void forEach(BiConsumer action) {
Review Comment:
did doing this fix something? it's weird.
##
solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java:
##
@@ -68,24 +75,123 @@ public SimpleOrderedMap(Map.Entry[]
nameValuePairs) {
public SimpleOrderedMap clone() {
ArrayList newList = new ArrayList<>(nvPairs.size());
newList.addAll(nvPairs);
-return new SimpleOrderedMap<>(newList);
+return new SimpleOrderedMap(newList);
+ }
+
+ @Override
+ public boolean isEmpty() {
+return nvPairs.isEmpty();
+ }
+
+ @Override
+ public boolean containsKey(final Object key) {
+return this.indexOf((String) key) >= 0;
+ }
+
+ @Override
+ public boolean containsValue(final Object value) {
+return values().contains(value);
}
/**
- * Returns a shared, empty, and immutable instance of SimpleOrderedMap.
+ * {@inheritDoc}
+ *
+ * Has linear lookup time O(N)
*
- * @return Empty SimpleOrderedMap (immutable)
+ * @see NamedList#get(String)
*/
- public static SimpleOrderedMap of() {
-return EMPTY;
+ @Override
+ public T get(final Object key) {
+return super.get((String) key);
+ }
+
+ @Override
+ public T put(String key, T value) {
+int idx = indexOf(key);
+if (idx == -1) {
+ add(key, value);
+ return null;
+} else {
+ T t = get(key);
+ setVal(idx, value);
+ return t;
+}
}
/**
- * Returns an immutable instance of SimpleOrderedMap with a single key-value
pair.
+ * @see NamedList#remove(String)
+ */
+ @Override
+ public T remove(final Object key) {
+return super.remove((String) key);
+ }
+
+ @Override
+ public void putAll(final Map m) {
+if (isEmpty()) {
+ m.forEach(this::add);
+} else {
+ m.forEach(this::put);
+}
+ }
+
+ @Override
+ public Map asShallowMap() {
Review Comment:
nitpick but place this up above implementing Map interfaces rather than
arbitrarily somewhere in the middle.
--
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]
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
renatoh commented on PR #3048: URL: https://github.com/apache/solr/pull/3048#issuecomment-2622011097 > **Separately** in another issue/pr, NamedList needs some more attention around this and toMap. Definitely deprecating that one. I interpreted you thumbs up on my comment early as OK for both of the issue mentioned, hence I committed and pushed it, was too quick:). Shall I revert it? -- 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]
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
dsmiley commented on PR #3048: URL: https://github.com/apache/solr/pull/3048#issuecomment-2621748542 > Wouldn't it be a better approach just to override asShallowMap in SimpleOrderedMap and return itself? :face-palm: of course; wow I have no excuse for myself. Not enough coffee maybe. > Additionally,I have noticed that org.apache.solr.common.util.NamedList#asShallowMap(boolean) is not called outside NamedList, do we want to set it to private? **Separately** in another issue/pr, NamedList needs some more attention around this and toMap. Definitely deprecating that one. -- 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]
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
renatoh commented on PR #3048: URL: https://github.com/apache/solr/pull/3048#issuecomment-2620972046 > As part of this PR, let's have NamedList.asShallowMap do an instanceof check for Map and return it if so. That will lead to real usage / code coverage of the changes you've done. Wouldn't it be a better approach just to override asShallowMap in SimpleOrderedMap and return itself? Additionally,I have noticed that org.apache.solr.common.util.NamedList#asShallowMap(boolean) is not called outside NamedList, do we want to set it to private? -- 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]
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
dsmiley commented on code in PR #3048:
URL: https://github.com/apache/solr/pull/3048#discussion_r1933308834
##
solr/solrj/build.gradle:
##
@@ -48,7 +48,6 @@ dependencies {
})
implementation libs.apache.httpcomponents.httpclient
implementation libs.apache.httpcomponents.httpcore
- implementation libs.apache.commons.lang3
Review Comment:
weird; what's that doing in this PR?
##
solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java:
##
@@ -146,43 +126,21 @@ public T remove(final Object key) {
return super.remove((String) key);
}
- /**
- * Copies all of the mappings from the specified map to this map. These
mappings will replace any
- * mappings that this map had for any of the keys currently in the specified
map.
- *
- * @param m mappings to be stored in this map
- * @throws NullPointerException if the specified map is null
- */
@Override
public void putAll(final Map m) {
m.forEach(this::put);
Review Comment:
if you look at NamedList's shallow Map putAll impl, there's a faster
implementation if the map is empty at the start. We could incorporate that
here with a simple check that calls `this::add`
--
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]
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
cpoerschke commented on code in PR #3048:
URL: https://github.com/apache/solr/pull/3048#discussion_r1932629861
##
solr/solrj/build.gradle:
##
@@ -81,7 +81,7 @@ dependencies {
testRuntimeOnly(libs.eclipse.jetty.alpnjavaserver, {
exclude group: "org.eclipse.jetty.alpn", module: "alpn-api"
})
- testImplementation(libs.mockito.core, {
+ testImplementation(libs.mockito.core, {hb
Review Comment:
```suggestion
testImplementation(libs.mockito.core, {
```
--
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]
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
dsmiley commented on code in PR #3048:
URL: https://github.com/apache/solr/pull/3048#discussion_r1929111648
##
solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java:
##
@@ -68,24 +74,153 @@ public SimpleOrderedMap(Map.Entry[]
nameValuePairs) {
public SimpleOrderedMap clone() {
ArrayList newList = new ArrayList<>(nvPairs.size());
newList.addAll(nvPairs);
-return new SimpleOrderedMap<>(newList);
+return new SimpleOrderedMap(newList);
+ }
+
+ @Override
+ public boolean isEmpty() {
+return nvPairs.isEmpty();
+ }
+
+ @Override
+ public boolean containsKey(final Object key) {
+return this.indexOf((String) key) >= 0;
}
/**
- * Returns a shared, empty, and immutable instance of SimpleOrderedMap.
+ * Returns {@code true} if this map maps one or more keys to the specified
value.
Review Comment:
If we don't have anything extra to say beyond the default javadoc, then add
nothing at all (no javadoc). If you want to add a tidbit of extra info to say
something O(N) then you can use `{@inheritDoc}` and add the sentence. But I
think you could just augment the class level javadocs to warn users about usage
of this as a general Map for large maps.
##
solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java:
##
@@ -68,24 +74,153 @@ public SimpleOrderedMap(Map.Entry[]
nameValuePairs) {
public SimpleOrderedMap clone() {
ArrayList newList = new ArrayList<>(nvPairs.size());
newList.addAll(nvPairs);
-return new SimpleOrderedMap<>(newList);
+return new SimpleOrderedMap(newList);
+ }
+
+ @Override
+ public boolean isEmpty() {
+return nvPairs.isEmpty();
+ }
+
+ @Override
+ public boolean containsKey(final Object key) {
+return this.indexOf((String) key) >= 0;
}
/**
- * Returns a shared, empty, and immutable instance of SimpleOrderedMap.
+ * Returns {@code true} if this map maps one or more keys to the specified
value.
*
- * @return Empty SimpleOrderedMap (immutable)
+ * @param value value whose presence in this map is to be tested
+ * @return {@code true} if this map maps one or more keys to the specified
value
*/
- public static SimpleOrderedMap of() {
-return EMPTY;
+ @Override
+ public boolean containsValue(final Object value) {
+int sz = size();
Review Comment:
instead of all these lines, could be a one-liner values().contains(value).
It'll be rare if ever for any Solr code to call this.
##
solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java:
##
@@ -68,24 +74,153 @@ public SimpleOrderedMap(Map.Entry[]
nameValuePairs) {
public SimpleOrderedMap clone() {
ArrayList newList = new ArrayList<>(nvPairs.size());
newList.addAll(nvPairs);
-return new SimpleOrderedMap<>(newList);
+return new SimpleOrderedMap(newList);
+ }
+
+ @Override
+ public boolean isEmpty() {
+return nvPairs.isEmpty();
+ }
+
+ @Override
+ public boolean containsKey(final Object key) {
+return this.indexOf((String) key) >= 0;
}
/**
- * Returns a shared, empty, and immutable instance of SimpleOrderedMap.
+ * Returns {@code true} if this map maps one or more keys to the specified
value.
*
- * @return Empty SimpleOrderedMap (immutable)
+ * @param value value whose presence in this map is to be tested
+ * @return {@code true} if this map maps one or more keys to the specified
value
*/
- public static SimpleOrderedMap of() {
-return EMPTY;
+ @Override
+ public boolean containsValue(final Object value) {
+int sz = size();
+for (int i = 0; i < sz; i++) {
+ T val = getVal(i);
+ if (Objects.equals(value, val)) {
+return true;
+ }
+}
+return false;
+ }
+
+ /**
+ * Has linear lookup time O(N)
+ *
+ * @see NamedList#get(String)
+ */
+ @Override
+ public T get(final Object key) {
+return super.get((String) key);
}
/**
- * Returns an immutable instance of SimpleOrderedMap with a single key-value
pair.
+ * Associates the specified value with the specified key in this map If the
map previously
+ * contained a mapping for the key, the old value is replaced by the
specified value.
*
- * @return SimpleOrderedMap containing one key-value pair
+ * @param key key with which the specified value is to be associated value –
value to be
+ * associated with the specified key
+ * @param value to be associated with the specified key
+ * @return the previous value associated with key, or null if there was no
mapping for key. (A
+ * null return can also indicate that the map previously associated null
with key)
+ */
+ @Override
+ public T put(String key, T value) {
+int idx = indexOf(key);
+if (idx == -1) {
+ add(key, value);
+ return null;
+} else {
+ T t = get(key);
+ setVal(idx, value);
+ return t;
+}
+ }
+
+ /**
+ * @see NamedList#remove(Str
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
renatoh commented on code in PR #3048:
URL: https://github.com/apache/solr/pull/3048#discussion_r1928520200
##
solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java:
##
@@ -68,24 +74,153 @@ public SimpleOrderedMap(Map.Entry[]
nameValuePairs) {
public SimpleOrderedMap clone() {
ArrayList newList = new ArrayList<>(nvPairs.size());
newList.addAll(nvPairs);
-return new SimpleOrderedMap<>(newList);
+return new SimpleOrderedMap(newList);
+ }
+
+ @Override
+ public boolean isEmpty() {
+return nvPairs.isEmpty();
+ }
+
+ @Override
+ public boolean containsKey(final Object key) {
+return this.indexOf((String) key) >= 0;
}
/**
- * Returns a shared, empty, and immutable instance of SimpleOrderedMap.
+ * Returns {@code true} if this map maps one or more keys to the specified
value.
*
- * @return Empty SimpleOrderedMap (immutable)
+ * @param value value whose presence in this map is to be tested
+ * @return {@code true} if this map maps one or more keys to the specified
value
*/
- public static SimpleOrderedMap of() {
-return EMPTY;
+ @Override
+ public boolean containsValue(final Object value) {
+int sz = size();
+for (int i = 0; i < sz; i++) {
+ T val = getVal(i);
+ if (Objects.equals(value, val)) {
+return true;
+ }
+}
+return false;
+ }
+
+ /**
+ * Has linear lookup time O(N)
+ *
+ * @see NamedList#get(String)
+ */
+ @Override
+ public T get(final Object key) {
+return super.get((String) key);
}
/**
- * Returns an immutable instance of SimpleOrderedMap with a single key-value
pair.
+ * Associates the specified value with the specified key in this map If the
map previously
+ * contained a mapping for the key, the old value is replaced by the
specified value.
*
- * @return SimpleOrderedMap containing one key-value pair
+ * @param key key with which the specified value is to be associated value –
value to be
+ * associated with the specified key
+ * @param value to be associated with the specified key
+ * @return the previous value associated with key, or null if there was no
mapping for key. (A
+ * null return can also indicate that the map previously associated null
with key)
+ */
+ @Override
+ public T put(String key, T value) {
+int idx = indexOf(key);
+if (idx == -1) {
+ add(key, value);
+ return null;
+} else {
+ T t = get(key);
+ setVal(idx, value);
+ return t;
+}
+ }
+
+ /**
+ * @see NamedList#remove(String)
+ */
+ @Override
+ public T remove(final Object key) {
+return super.remove((String) key);
+ }
+
+ /**
+ * Copies all of the mappings from the specified map to this map. These
mappings will replace any
+ * mappings that this map had for any of the keys currently in the specified
map.
+ *
+ * @param m mappings to be stored in this map
+ * @throws NullPointerException if the specified map is null
+ */
+ @Override
+ public void putAll(final Map m) {
+m.forEach(this::put);
+ }
+
+ /**
+ * return a {@link Set} of all keys in the map.
+ *
+ * @return {@link Set} of all keys in the map
+ */
+ @Override
+ public Set keySet() {
+return new InnerMap().keySet();
Review Comment:
I've tried using an AbstractList / AbstractSet for values() / keySet(), but
on the AbstractSet I need to implement an anonymous Iterator within
the anonymous implementation of AbstractSet. Hence the approach with the
InnerMap is much more concise.
--
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]
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
renatoh commented on code in PR #3048:
URL: https://github.com/apache/solr/pull/3048#discussion_r1928511029
##
solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java:
##
@@ -68,24 +74,153 @@ public SimpleOrderedMap(Map.Entry[]
nameValuePairs) {
public SimpleOrderedMap clone() {
ArrayList newList = new ArrayList<>(nvPairs.size());
newList.addAll(nvPairs);
-return new SimpleOrderedMap<>(newList);
+return new SimpleOrderedMap(newList);
+ }
+
+ @Override
+ public boolean isEmpty() {
+return nvPairs.isEmpty();
+ }
+
+ @Override
+ public boolean containsKey(final Object key) {
+return this.indexOf((String) key) >= 0;
}
/**
- * Returns a shared, empty, and immutable instance of SimpleOrderedMap.
+ * Returns {@code true} if this map maps one or more keys to the specified
value.
Review Comment:
the java-doc is automatically inherited? isn't that only the case if I use
the {@inheritDoc} tag in the java doc itself. And even then, it does not
inherit the param, return and throw part of the java-doc, unless I tag them one
by one. Nevertheless, I will move to {@inheritDoc} where possible, in order
not to repeat java doc.
With respect to "one or more", I did not come up with that phrasing, that is
on Map#contains, and although it sounds strange, it is technically correct.
--
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]
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
dsmiley commented on code in PR #3048:
URL: https://github.com/apache/solr/pull/3048#discussion_r1927992917
##
solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java:
##
@@ -68,24 +74,153 @@ public SimpleOrderedMap(Map.Entry[]
nameValuePairs) {
public SimpleOrderedMap clone() {
ArrayList newList = new ArrayList<>(nvPairs.size());
newList.addAll(nvPairs);
-return new SimpleOrderedMap<>(newList);
+return new SimpleOrderedMap(newList);
+ }
+
+ @Override
+ public boolean isEmpty() {
+return nvPairs.isEmpty();
+ }
+
+ @Override
+ public boolean containsKey(final Object key) {
+return this.indexOf((String) key) >= 0;
}
/**
- * Returns a shared, empty, and immutable instance of SimpleOrderedMap.
+ * Returns {@code true} if this map maps one or more keys to the specified
value.
Review Comment:
"one or more" -- why more? Any way, don't feel obliged to re-specify
javadocs for interface implementations since it's automatically inherited.
##
solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java:
##
@@ -68,24 +74,153 @@ public SimpleOrderedMap(Map.Entry[]
nameValuePairs) {
public SimpleOrderedMap clone() {
ArrayList newList = new ArrayList<>(nvPairs.size());
newList.addAll(nvPairs);
-return new SimpleOrderedMap<>(newList);
+return new SimpleOrderedMap(newList);
+ }
+
+ @Override
+ public boolean isEmpty() {
+return nvPairs.isEmpty();
+ }
+
+ @Override
+ public boolean containsKey(final Object key) {
+return this.indexOf((String) key) >= 0;
}
/**
- * Returns a shared, empty, and immutable instance of SimpleOrderedMap.
+ * Returns {@code true} if this map maps one or more keys to the specified
value.
*
- * @return Empty SimpleOrderedMap (immutable)
+ * @param value value whose presence in this map is to be tested
+ * @return {@code true} if this map maps one or more keys to the specified
value
*/
- public static SimpleOrderedMap of() {
-return EMPTY;
+ @Override
+ public boolean containsValue(final Object value) {
+int sz = size();
+for (int i = 0; i < sz; i++) {
+ T val = getVal(i);
+ if (Objects.equals(value, val)) {
+return true;
+ }
+}
+return false;
+ }
+
+ /**
+ * Has linear lookup time O(N)
+ *
+ * @see NamedList#get(String)
+ */
+ @Override
+ public T get(final Object key) {
+return super.get((String) key);
}
/**
- * Returns an immutable instance of SimpleOrderedMap with a single key-value
pair.
+ * Associates the specified value with the specified key in this map If the
map previously
+ * contained a mapping for the key, the old value is replaced by the
specified value.
*
- * @return SimpleOrderedMap containing one key-value pair
+ * @param key key with which the specified value is to be associated value –
value to be
+ * associated with the specified key
+ * @param value to be associated with the specified key
+ * @return the previous value associated with key, or null if there was no
mapping for key. (A
+ * null return can also indicate that the map previously associated null
with key)
+ */
+ @Override
+ public T put(String key, T value) {
+int idx = indexOf(key);
+if (idx == -1) {
+ add(key, value);
+ return null;
+} else {
+ T t = get(key);
+ setVal(idx, value);
+ return t;
+}
+ }
+
+ /**
+ * @see NamedList#remove(String)
+ */
+ @Override
+ public T remove(final Object key) {
+return super.remove((String) key);
+ }
+
+ /**
+ * Copies all of the mappings from the specified map to this map. These
mappings will replace any
+ * mappings that this map had for any of the keys currently in the specified
map.
+ *
+ * @param m mappings to be stored in this map
+ * @throws NullPointerException if the specified map is null
+ */
+ @Override
+ public void putAll(final Map m) {
+m.forEach(this::put);
+ }
+
+ /**
+ * return a {@link Set} of all keys in the map.
+ *
+ * @return {@link Set} of all keys in the map
+ */
+ @Override
+ public Set keySet() {
+return new InnerMap().keySet();
Review Comment:
I think it'd be clearer to return a new AbstractList (and can declare the
return type to be List as well)
##
solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java:
##
@@ -68,24 +74,153 @@ public SimpleOrderedMap(Map.Entry[]
nameValuePairs) {
public SimpleOrderedMap clone() {
ArrayList newList = new ArrayList<>(nvPairs.size());
newList.addAll(nvPairs);
-return new SimpleOrderedMap<>(newList);
+return new SimpleOrderedMap(newList);
+ }
+
+ @Override
+ public boolean isEmpty() {
+return nvPairs.isEmpty();
+ }
+
+ @Override
+ public boolean containsKey(final Object key) {
+return this.indexOf((String) key) >= 0;
}
/**
- * Returns a shared, empty, and immutable ins
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
renatoh commented on PR #3048: URL: https://github.com/apache/solr/pull/3048#issuecomment-2610858357 @dsmiley Please have another look, thanks! -- 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]
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
dsmiley commented on PR #3048: URL: https://github.com/apache/solr/pull/3048#issuecomment-2605960508 I would implement the methods here, copying bits of code from #3050 if needed so that you don't call NamedList.asShallowMap. Take from that whatever you want. I'm not sure NamedList.asShallowMap was a wise idea since it can't implement the Map contract if the keys repeat. It might have pre-dated SimpleOrderedMap. Assuming you finish the PR here, I'm inclined to maybe make NamedList.asShallowMap do an instanceof check for SimpleOrderedMap and if so return "this" but otherwise throw an exception. Or remove it. Ideally we only use NamedList that isn't SimpleOrderedMap for the rare cases where it's appropriate. -- 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]
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
renatoh commented on PR #3048:
URL: https://github.com/apache/solr/pull/3048#issuecomment-2605722808
@dsmiley Considering your improvements on NamedList..asShallowMap, could I
just delegate the method keySet, values and entrySet to the shallowMap e.g. :
public Collection values() {
return super.asShallowMap(true).values();
}
--
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]
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
dsmiley commented on PR #3048: URL: https://github.com/apache/solr/pull/3048#issuecomment-2605415831 > I am curious if there is a way to implement values() and entrySet(0 without an unchecked cast, e.g. (T) nvPairs.get(i) Almost nobody could answer a cast question like that without literally working on it in their IDE. I'll take a look at a future iteration of yours but meanwhile, don't worry about it -- it's not important. -- 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]
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
renatoh commented on code in PR #3048:
URL: https://github.com/apache/solr/pull/3048#discussion_r1922882654
##
solr/solrj/src/test/org/apache/solr/common/util/SimpleOrderedMapTest.java:
##
@@ -0,0 +1,125 @@
+package org.apache.solr.common.util;
+
+import java.util.Collection;
+import java.util.Map;
+import java.util.Set;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class SimpleOrderedMapTest extends Assert {
+
+ private final SimpleOrderedMap map = new SimpleOrderedMap<>();
Review Comment:
JUnit calls every single test-method with a fresh instance of the test
class, there is no need for a lifecycle with a clear.
--
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]
Re: [PR] SOLR-17623: Simple ordered map should implement map [solr]
renatoh commented on code in PR #3048:
URL: https://github.com/apache/solr/pull/3048#discussion_r1922881412
##
solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java:
##
@@ -86,6 +91,79 @@ public static SimpleOrderedMap of() {
* @return SimpleOrderedMap containing one key-value pair
*/
public static SimpleOrderedMap of(String name, T val) {
-return new SimpleOrderedMap<>(List.of(name, val));
+return new SimpleOrderedMap(List.of(name, val));
+ }
+
+ @Override
+ public boolean isEmpty() {
+return nvPairs.isEmpty();
+ }
+
+ @Override
+ public boolean containsKey(final Object key) {
+return super.get((String) key) != null;
+ }
+
+ @Override
+ public boolean containsValue(final Object value) {
+int sz = size();
+for (int i = 0; i < sz; i++) {
+ T val = getVal(i);
+ if (Objects.equals(value, val)) {
+return true;
+ }
+}
+return false;
+ }
+
+ @Override
+ public T get(final Object key) {
+return super.get((String) key);
+ }
+
+ @Override
+ public T put(final String key, final T value) {
+T oldValue = get(key);
+add(key, value);
+return oldValue;
+ }
+
+ @Override
+ public T remove(final Object key) {
+return super.remove((String) key);
+ }
+
+ @Override
+ public void putAll(final Map m) {
+m.forEach(this::put);
+ }
+
+ @Override
+ public Set keySet() {
+var keys = new HashSet();
Review Comment:
How else should I collect every other entry from the nvPairs into Set?
The only other way I can think of is to use a .stream()
--
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]
Re: [PR] Solr 17623: Simple ordered map should implement map [solr]
dsmiley commented on code in PR #3048:
URL: https://github.com/apache/solr/pull/3048#discussion_r1922851418
##
solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java:
##
@@ -86,6 +91,79 @@ public static SimpleOrderedMap of() {
* @return SimpleOrderedMap containing one key-value pair
*/
public static SimpleOrderedMap of(String name, T val) {
-return new SimpleOrderedMap<>(List.of(name, val));
+return new SimpleOrderedMap(List.of(name, val));
+ }
+
+ @Override
+ public boolean isEmpty() {
+return nvPairs.isEmpty();
+ }
+
+ @Override
+ public boolean containsKey(final Object key) {
+return super.get((String) key) != null;
+ }
+
+ @Override
+ public boolean containsValue(final Object value) {
+int sz = size();
+for (int i = 0; i < sz; i++) {
+ T val = getVal(i);
+ if (Objects.equals(value, val)) {
+return true;
+ }
+}
+return false;
+ }
+
+ @Override
+ public T get(final Object key) {
+return super.get((String) key);
+ }
+
+ @Override
+ public T put(final String key, final T value) {
+T oldValue = get(key);
+add(key, value);
+return oldValue;
+ }
+
+ @Override
+ public T remove(final Object key) {
+return super.remove((String) key);
+ }
+
+ @Override
+ public void putAll(final Map m) {
+m.forEach(this::put);
+ }
+
+ @Override
+ public Set keySet() {
+var keys = new HashSet();
+for (int i = 0; i < nvPairs.size(); i += 2) {
+ keys.add((String) nvPairs.get(i));
+}
+return keys;
+ }
+
+ @Override
+ @SuppressWarnings({"unchecked"})
+ public Collection values() {
Review Comment:
Please don't create new datastructures when calling methods you add
##
solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java:
##
@@ -86,6 +91,79 @@ public static SimpleOrderedMap of() {
* @return SimpleOrderedMap containing one key-value pair
*/
public static SimpleOrderedMap of(String name, T val) {
-return new SimpleOrderedMap<>(List.of(name, val));
+return new SimpleOrderedMap(List.of(name, val));
+ }
+
+ @Override
+ public boolean isEmpty() {
+return nvPairs.isEmpty();
+ }
+
+ @Override
+ public boolean containsKey(final Object key) {
+return super.get((String) key) != null;
+ }
+
+ @Override
+ public boolean containsValue(final Object value) {
+int sz = size();
+for (int i = 0; i < sz; i++) {
+ T val = getVal(i);
+ if (Objects.equals(value, val)) {
+return true;
+ }
+}
+return false;
+ }
+
+ @Override
+ public T get(final Object key) {
+return super.get((String) key);
+ }
+
+ @Override
+ public T put(final String key, final T value) {
+T oldValue = get(key);
+add(key, value);
+return oldValue;
+ }
+
+ @Override
+ public T remove(final Object key) {
+return super.remove((String) key);
+ }
+
+ @Override
+ public void putAll(final Map m) {
+m.forEach(this::put);
+ }
+
+ @Override
+ public Set keySet() {
+var keys = new HashSet();
Review Comment:
Please don't create new datastructures when calling methods you add
##
solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java:
##
@@ -86,6 +91,79 @@ public static SimpleOrderedMap of() {
* @return SimpleOrderedMap containing one key-value pair
*/
public static SimpleOrderedMap of(String name, T val) {
-return new SimpleOrderedMap<>(List.of(name, val));
+return new SimpleOrderedMap(List.of(name, val));
+ }
+
+ @Override
+ public boolean isEmpty() {
+return nvPairs.isEmpty();
+ }
+
+ @Override
+ public boolean containsKey(final Object key) {
+return super.get((String) key) != null;
Review Comment:
the value can actually be null in a NamedList, so this is inaccurate. Use
indexOf
##
solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java:
##
@@ -86,6 +91,79 @@ public static SimpleOrderedMap of() {
* @return SimpleOrderedMap containing one key-value pair
*/
public static SimpleOrderedMap of(String name, T val) {
-return new SimpleOrderedMap<>(List.of(name, val));
+return new SimpleOrderedMap(List.of(name, val));
+ }
+
+ @Override
+ public boolean isEmpty() {
+return nvPairs.isEmpty();
+ }
+
+ @Override
+ public boolean containsKey(final Object key) {
+return super.get((String) key) != null;
+ }
+
+ @Override
+ public boolean containsValue(final Object value) {
+int sz = size();
+for (int i = 0; i < sz; i++) {
+ T val = getVal(i);
+ if (Objects.equals(value, val)) {
+return true;
+ }
+}
+return false;
+ }
+
+ @Override
+ public T get(final Object key) {
+return super.get((String) key);
+ }
+
+ @Override
+ public T put(final String key, final T value) {
+T oldValue = get(key);
+add(key, value);
Review Comment:
add will not replace the existing value. This is why that metho
Re: [PR] Solr 17623: Simple ordered map should implement map [solr]
dsmiley commented on PR #3048: URL: https://github.com/apache/solr/pull/3048#issuecomment-2603231369 As the PR template indicates, the PR title must use exactly the JIRA syntax. Thus all-caps and hyphen separating SOLR and 17623, probably then a colon. If you do it correctly, moments later a bot will update the JIRA to link to the PR. (albeit sometimes the bot is delayed inexplicably) -- 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]
Re: [PR] Solr 17623: Simple ordered map should implement map [solr]
renatoh commented on PR #3048: URL: https://github.com/apache/solr/pull/3048#issuecomment-2603171608 I am curious if there is a way to implement values() and entrySet(0 without an unchecked cast, e.g. (T) nvPairs.get(i) -- 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]
Re: [PR] Solr 17623 simple ordered map should implement map [solr]
renatoh commented on PR #3048: URL: https://github.com/apache/solr/pull/3048#issuecomment-2603163228 @dsmiley Could you please have a look at the implementation oaf the method from the Map interface? Also, I had to make a few adjustment to other parts in the code to make it work with the new methods. I need to add java-doc and tidy up the unit-tests but would be good to get some feedback before I take care of that. Thanks -- 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]
Re: [PR] Solr 17623 simple ordered map should implement map [solr]
epugh commented on PR #3048: URL: https://github.com/apache/solr/pull/3048#issuecomment-2603160290 Nitpicky, but can you title the PR `SOLR-17623:`, that is the style we use... -- 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]
