SOLR-17623: Simple ordered map should implement map #3048

renatoh · 2025-01-20T20:00:08Z

https://issues.apache.org/jira/browse/SOLR-17623

Implementing the interface Map on SimpleOrderedMap and adding Unit-Test for the new methods

Adding unit test for newly added method

…plement-Map

epugh · 2025-01-20T20:04:24Z

Nitpicky, but can you title the PR SOLR-17623:, that is the style we use...

renatoh · 2025-01-20T20:06:52Z

@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

renatoh · 2025-01-20T20:13:58Z

I am curious if there is a way to implement values() and entrySet(0 without an unchecked cast, e.g. (T) nvPairs.get(i)

dsmiley · 2025-01-20T21:08:30Z

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)

solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java

dsmiley · 2025-01-20T21:18:40Z

solr/solrj/src/test/org/apache/solr/common/util/SimpleOrderedMapTest.java

+import org.junit.Assert;
+import org.junit.Test;
+
+public class SimpleOrderedMapTest extends Assert {


No; in Solr we extend SolrTestCase at a minimum for a unit test

solr/solrj/src/test/org/apache/solr/common/util/SimpleOrderedMapTest.java

dsmiley · 2025-01-21T18:06:11Z

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.

renatoh · 2025-01-21T20:57:05Z

@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();
}

dsmiley · 2025-01-21T23:37:29Z

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.

…Set/AbstractMap without creating new datastructure

renatoh · 2025-01-23T19:36:46Z

@dsmiley Please have another look, thanks!

dsmiley

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.

dsmiley · 2025-01-24T02:15:52Z

solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java

  }

  /**
-   * Returns a shared, empty, and immutable instance of SimpleOrderedMap.
+   * Returns {@code true} if this map maps one or more keys to the specified value.


"one or more" -- why more? Any way, don't feel obliged to re-specify javadocs for interface implementations since it's automatically inherited.

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.

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

dsmiley · 2025-01-24T18:49:15Z

solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java

-    return EMPTY;
+  @Override
+  public boolean containsValue(final Object value) {
+    int sz = size();


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

dsmiley · 2025-01-24T18:55:11Z

solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java

  }

  /**
-   * Returns a shared, empty, and immutable instance of SimpleOrderedMap.
+   * Returns {@code true} if this map maps one or more keys to the specified value.


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/test/org/apache/solr/common/util/SimpleOrderedMapTest.java

…impleOrderedMap

solr/solrj/build.gradle

dsmiley

Looks good; thank you! Just needs a CHANGES.txt entry.

My hope is for NamedList.asShallowMap's implementation to go away, ultimately mandating that if you want a Map, you must have a SimpleOrderedMap. That can be a separate issue/PR.

solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java

solr/solrj/build.gradle

renatoh · 2025-01-29T08:24:54Z

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?

dsmiley · 2025-01-29T14:05:37Z

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.

…t.asShallowMap(boolean) private

renatoh · 2025-01-29T15:44:47Z

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?

dsmiley

I 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().

dsmiley · 2025-01-29T17:26:17Z

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


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.

dsmiley · 2025-01-29T17:26:41Z

solr/solrj/src/java/org/apache/solr/common/util/NamedList.java

@@ -403,8 +411,8 @@ public Map<String, T> asShallowMap() {
    return asShallowMap(false);
  }

-  public Map<String, T> asShallowMap(boolean allowDps) {
-    return new Map<String, T>() {
+  private Map<String, T> asShallowMap(boolean allowDps) {


now's not the time to make private but feel free to deprecate.

solr/solrj/src/java/org/apache/solr/common/util/NamedList.java

dsmiley · 2025-01-29T17:29:08Z

solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java

+  }
+
+  @Override
+  public Map<String, T> asShallowMap() {


nitpick but place this up above implementing Map interfaces rather than arbitrarily somewhere in the middle.

renatoh · 2025-01-29T20:06:09Z

Just needs a CHANGES.txt entry.

I've added that under Solr 9.9.0, is that correct?

renatoh · 2025-01-29T21:54:03Z

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 created in org.apache.solr.common.ConfigNode#forEachChild? So the changes do not make a difference here, or do they?

…-Map # Conflicts: # solr/CHANGES.txt

dsmiley · 2025-01-30T19:31:38Z

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!

renatoh · 2025-01-30T20:47:34Z

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

dsmiley · 2025-01-30T23:24:03Z

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.

renatoh · 2025-01-31T13:00:56Z

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:)

Also added NamedList.indexOf(name) convenience method. Does *not* adjust asShallowMap() yet. Coming soon. --------- Co-authored-by: David Smiley <[email protected]> (cherry picked from commit dac5fe1)

renatoh added 4 commits January 19, 2025 20:27

SOLR-17623: implementing java.util.Map interface on SimpleOrderedMap

c7f9cb3

Adding unit test for newly added method

Merge branch 'apache:main' into SOLR-17623-SimpleOrderedMap-should-im…

4b884fb

…plement-Map

SOLR-17623: adding unit test to cover all methods

aeca5f9

SOLR-17623: fix case for contains(null)

eb9792c

github-actions bot added client:solrj tool:build tests cat:search labels Jan 20, 2025

renatoh changed the title ~~Solr 17623 simple ordered map should implement map~~ Solr 17623: Simple ordered map should implement map Jan 20, 2025

dsmiley reviewed Jan 20, 2025

View reviewed changes

renatoh changed the title ~~Solr 17623: Simple ordered map should implement map~~ SOLR-17623: Simple ordered map should implement map Jan 20, 2025

dsmiley mentioned this pull request Jan 21, 2025

SOLR-17552: NamedList.asShallowMap improvements #3050

Draft

renatoh added 2 commits January 22, 2025 09:37

SOLR-17623:Implementing method keySet, values, entrySet with Abstract…

5691fae

…Set/AbstractMap without creating new datastructure

SOLR-17623:adding java doc

b7d979e

SOLR-17623:tidy up java doc

dc052dd

dsmiley reviewed Jan 24, 2025

View reviewed changes

renatoh added 4 commits January 25, 2025 10:09

SOLR-17623: return itself on asShalloMap in case the instance is of S…

4bbdac5

…impleOrderedMap

SOLR-17623: reimplement containns-method; refine javadoc

8e1601e

SOLR-17623: tidy up source code

8a8d177

SOLR-17623: fixing dependency

86f4b0c

cpoerschke reviewed Jan 28, 2025

View reviewed changes

solr/solrj/build.gradle Outdated Show resolved Hide resolved

Update solr/solrj/build.gradle

2e8f902

github-actions bot removed the tool:build label Jan 28, 2025

dsmiley approved these changes Jan 29, 2025

View reviewed changes

solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java Outdated Show resolved Hide resolved

solr/solrj/build.gradle Outdated Show resolved Hide resolved

renatoh added 2 commits January 29, 2025 08:46

SOLR-17623: implementing performance optimization for putAll

217a357

SOLR-17623: small improvements on test

d5a7172

SOLR-17623: return SimpleOrderedMap for asShallowMap; making NamedLis…

6516fc4

…t.asShallowMap(boolean) private

SOLR-17623: add override annotation

7f53e8c

dsmiley reviewed Jan 29, 2025

View reviewed changes

SOLR-17623: fixing dependency

bb62299

dsmiley and others added 2 commits January 30, 2025 01:04

Tweaks

26f6043

Merge branch 'main' into SOLR-17623-SimpleOrderedMap-should-implement…

d270551

…-Map # Conflicts: # solr/CHANGES.txt

SOLR-17623: adding header to test-class

6ac4be0

dsmiley merged commit dac5fe1 into apache:main Jan 31, 2025
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-17623: Simple ordered map should implement map #3048

SOLR-17623: Simple ordered map should implement map #3048

renatoh commented Jan 20, 2025

epugh commented Jan 20, 2025

renatoh commented Jan 20, 2025

renatoh commented Jan 20, 2025

dsmiley commented Jan 20, 2025

dsmiley Jan 20, 2025

dsmiley commented Jan 21, 2025

renatoh commented Jan 21, 2025

dsmiley commented Jan 21, 2025

renatoh commented Jan 23, 2025

dsmiley left a comment

dsmiley Jan 24, 2025

renatoh Jan 24, 2025

dsmiley Jan 24, 2025

dsmiley Jan 24, 2025

dsmiley Jan 24, 2025

dsmiley left a comment

renatoh commented Jan 29, 2025

dsmiley commented Jan 29, 2025

renatoh commented Jan 29, 2025

dsmiley left a comment

dsmiley Jan 29, 2025

dsmiley Jan 29, 2025

dsmiley Jan 29, 2025

renatoh commented Jan 29, 2025

renatoh commented Jan 29, 2025 •

edited

Loading

dsmiley commented Jan 30, 2025

renatoh commented Jan 30, 2025

dsmiley commented Jan 30, 2025

renatoh commented Jan 31, 2025

SOLR-17623: Simple ordered map should implement map #3048

SOLR-17623: Simple ordered map should implement map #3048

Conversation

renatoh commented Jan 20, 2025

epugh commented Jan 20, 2025

renatoh commented Jan 20, 2025

renatoh commented Jan 20, 2025

dsmiley commented Jan 20, 2025

Choose a reason for hiding this comment

dsmiley commented Jan 21, 2025

renatoh commented Jan 21, 2025

dsmiley commented Jan 21, 2025

renatoh commented Jan 23, 2025

dsmiley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsmiley left a comment

Choose a reason for hiding this comment

renatoh commented Jan 29, 2025

dsmiley commented Jan 29, 2025

renatoh commented Jan 29, 2025

dsmiley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renatoh commented Jan 29, 2025

renatoh commented Jan 29, 2025 • edited Loading

dsmiley commented Jan 30, 2025

renatoh commented Jan 30, 2025

dsmiley commented Jan 30, 2025

renatoh commented Jan 31, 2025

renatoh commented Jan 29, 2025 •

edited

Loading