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<String, T>[]
nameValuePairs) {
public SimpleOrderedMap<T> clone() {
ArrayList<Object> newList = new ArrayList<>(nvPairs.size());
newList.addAll(nvPairs);
- return new SimpleOrderedMap<>(newList);
+ return new SimpleOrderedMap<T>(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<String, T>[]
nameValuePairs) {
public SimpleOrderedMap<T> clone() {
ArrayList<Object> newList = new ArrayList<>(nvPairs.size());
newList.addAll(nvPairs);
- return new SimpleOrderedMap<>(newList);
+ return new SimpleOrderedMap<T>(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<Object> 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<String, T>[]
nameValuePairs) {
public SimpleOrderedMap<T> clone() {
ArrayList<Object> newList = new ArrayList<>(nvPairs.size());
newList.addAll(nvPairs);
- return new SimpleOrderedMap<>(newList);
+ return new SimpleOrderedMap<T>(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<Object> 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<? extends String, ? extends T> 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<String> keySet() {
+ return new InnerMap().keySet();
+ }
+
+ /**
+ * return a {@link Collection} of all values in the map.
+ *
+ * @return {@link Collection} of all values in the map
+ */
+ @Override
+ public Collection<T> values() {
+ return new InnerMap().values();
+ }
+
+ /**
+ * Returns a {@link Set} view of the mappings contained in this map.
+ *
+ * @return {@link Set} view of mappings
+ */
+ @Override
+ public Set<Entry<String, T>> entrySet() {
+
+ return new AbstractSet<>() {
+ @Override
+ public Iterator<Entry<String, T>> iterator() {
+ return SimpleOrderedMap.this.iterator();
+ }
+
+ @Override
+ public int size() {
+ return SimpleOrderedMap.this.size();
+ }
+ };
+ }
+
+ /**
+ * Returns an immutable instance of {@link SimpleOrderedMap} with a single
key-value pair.
+ *
+ * @return {@link SimpleOrderedMap} containing one key-value pair
*/
public static <T> SimpleOrderedMap<T> of(String name, T val) {
- return new SimpleOrderedMap<>(List.of(name, val));
+ return new SimpleOrderedMap<T>(List.of(name, val));
+ }
+
+ /**
+ * Returns a shared, empty, and immutable instance of {@link
SimpleOrderedMap}.
+ *
+ * @return Empty {@link SimpleOrderedMap} (immutable)
+ */
+ public static SimpleOrderedMap<Object> of() {
+ return EMPTY;
+ }
+
+ private class InnerMap extends AbstractMap<String, T> {
Review Comment:
I better appreciate why this is here. I don't love the name but I don't
recommend something else. I think it should have a comment explaining why this
is here. I'd say "SimpleOrderedMap can't extend AbstractMap, sadly. This
inner class can, and allows us to implement some of Map's methods via one-liner
calls out to this.
##########
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<Integer> map = new SimpleOrderedMap<>();
Review Comment:
Ha; thanks for educating me! Jeesh; I should know that. Any way, for a
trivial line, I'd just put it at the top of every test method but it's not a
big deal; do as you wish.
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
