Re: [PR] SOLR-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on PR #3163: URL: https://github.com/apache/solr/pull/3163#issuecomment-3001526628 @dsmiley 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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
dsmiley merged PR #3163: URL: https://github.com/apache/solr/pull/3163 -- 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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
dsmiley commented on PR #3163: URL: https://github.com/apache/solr/pull/3163#issuecomment-2994835849 I merged with main and tweaked the naming a little as I asked. But most notably, made the setting `false` by default, thus is an opt-in compatibility mechanism that can be enabled as we see a need for it. Given the performance risk, I felt this approach is safer. I think this is ready to merge. -- 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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
github-actions[bot] commented on PR #3163: URL: https://github.com/apache/solr/pull/3163#issuecomment-2849597341 This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the [email protected] mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you 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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
dsmiley commented on PR #3163: URL: https://github.com/apache/solr/pull/3163#issuecomment-2701775120 > So what is the point of changing from a LinkedHashMap to LinkedHashMap in a SMO? A LinkedHashMap doesn't implement NamedList. An SMO does. Thus a Solr 10 server could start passing maps and MapWriter things (e.g. SolrParams) to the response in situations that a SolrJ 9 is still anticipating reading a NamedList. With SOM; it's both things in one. The choice of LinkedHashMap vs ArrayList would be a regression for cases where the client code already expected a Map; was assuming O(1) performance of the map if lots of stuff is in it. Because NamedList/Map is nestable, at the time of unmarshalling we have no idea what the actual code looking at the object is casting it as. In a fantasy world if there was just exactly one thing to unmarshall, then there would be maybe readMap vs readNamedList and we wouldn't have concerns that this PR tries to solve. That said, let's not actually change SMO to be based on a LinkedHashMap; at least not soon. I proposed only reading maps as SMO for a more narrow circumstance (9.x talking to 10.x) that would be somewhat temporary / limited, thus ameliorating a performance risk. -- 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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on PR #3163: URL: https://github.com/apache/solr/pull/3163#issuecomment-2701327325 > Map.get, Map.remove, Map.put -- basic operations are O(1) in a hash based Map, but not in a dense array List that SOM is today. I do understand that part, but today, we are umarshalling to a LinkedHashMap, with the current changes we would unmarshall to a SMO (backed by an ArrayList), I do understand the potential performance issues of that change. But if we now changed the SMO to a LinkedHashMap, we would then unmarshall to an LinkedHashMap in a SMO, compared to unmarshall to a LinkedHashmap, as we do today. So what is the point of changing from a LinkedHashMap to LinkedHashMap in a SMO? -- 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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
dsmiley commented on PR #3163: URL: https://github.com/apache/solr/pull/3163#issuecomment-2701019493 Map.get, Map.remove, Map.put -- basic operations are O(1) in a hash based Map, but not in a dense array List that SOM is today. -- 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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on PR #3163: URL: https://github.com/apache/solr/pull/3163#issuecomment-2700819405 > Only an SMO is also a NamedList. A LinkedHashMap itself is not. Just for my understand: What exactly would improve by unmarshalling to a SMO backed by LinkedHashMap compared to just a LinkedHashMap? -- 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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
dsmiley commented on PR #3163: URL: https://github.com/apache/solr/pull/3163#issuecomment-2700120656 > But if SMO is backed by a LinkedHashMap, wouldn't then these changes become obsolete? What is the point of unmarshalling to SMO (backed by a LinkedHashMap) instead of a directly to a LinkedHashMap? Only an SMO is also a NamedList. A LinkedHashMap itself is not. I was thinking about the need for this a bit more. The primary concern is a SolrJ 9 reading data sent from Solr 10, which will see more expanded use of SimpleOrderedMap that was previously a NamedList. Other circumstances can leave this disabled by default. In Solr 9 specifically, we could change BinaryResponseParser to detect the version of the writer in some way TBD and then act accordingly. A separate issue/PR. -- 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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on PR #3163: URL: https://github.com/apache/solr/pull/3163#issuecomment-2700070953 > I have a some doubts that gnaw at me pertaining to the performance risks of O(1) maps suddenly becoming O(N). It doesn't sit well with me. You had suggested changing SimpleOrderedMap to be LinkedHashMap based. That's definitely safer, and whatever extra overhead is involved there should be fairly minor. Maybe we should pause merging this until pursuing that. Pursuing that probably means deprecating/removing indexed access to NamedList, requiring instead callers use iterators. But if SMO is backed by a LinkedHashMap, wouldn't then these changes become obsolete? What is the point of unmarshalling to SMO (backed by a LinkedHashMap) instead of a directly to a LinkedHashMap? -- 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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
dsmiley commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1979525750
##
solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java:
##
@@ -124,6 +124,9 @@ public class JavaBinCodec implements PushWriter {
private boolean alreadyUnmarshalled;
protected boolean readStringAsCharSeq = false;
+ private boolean mapAsNamedList =
+ EnvUtils.getPropertyAsBool("solr.solrj.javabin.mapAsNamedList", true);
Review Comment:
should have "read" in here, in the field & property
--
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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on PR #3163: URL: https://github.com/apache/solr/pull/3163#issuecomment-2698840306 @dsmiley All tests are passing now, are we able to merge? -- 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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
dsmiley commented on PR #3163: URL: https://github.com/apache/solr/pull/3163#issuecomment-2692370151 Let's get #3214 merged first, thus narrowing this PR to just what it needs to do. -- 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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1969287910
##
solr/solrj/src/java/org/apache/solr/common/util/Utils.java:
##
@@ -290,7 +290,7 @@ public static Object fromJSON(byte[] utf8) {
}
public static Object fromJSON(byte[] utf8, int offset, int length) {
-return fromJSON(utf8, offset, length, STANDARDOBJBUILDER);
+return fromJSON(utf8, offset, length, SIMPLEORDEREDMAPOBJBUILDER);
Review Comment:
I see, now the test
ClusterStateProviderTest#testClusterStateProviderEmptySolrVersion is passing.
So the issue was not that one of the DocCollection had a SOM and the other had
a LinkedHashMap, the issue was that they were not equal despite of having the
same entries. Thanks makes sense:)
--
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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
dsmiley commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1966955662
##
solr/solrj/src/java/org/apache/solr/common/util/Utils.java:
##
@@ -290,7 +290,7 @@ public static Object fromJSON(byte[] utf8) {
}
public static Object fromJSON(byte[] utf8, int offset, int length) {
-return fromJSON(utf8, offset, length, STANDARDOBJBUILDER);
+return fromJSON(utf8, offset, length, SIMPLEORDEREDMAPOBJBUILDER);
Review Comment:
This must be changed back; I don't think it makes sense to deserialize JSON
and get a SimpleOrderedMap in there. I debugged and the root of the problem is
that SimpleOrderedMap is not actually fully implementing the contract of a Map
with regards to equals & hashCode. I'll push some changes very soon. This
part should be extracted to it's own PR against the same issue of
SimpleOrderedMap implementing a Map.
--
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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on PR #3163: URL: https://github.com/apache/solr/pull/3163#issuecomment-2670744975 > FYI I'm _very_ eager to examine this in detail but am on vacation this week. No worries, enjoy your vacation, I will off next week and limited to my phone -- 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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
dsmiley commented on PR #3163: URL: https://github.com/apache/solr/pull/3163#issuecomment-2670503495 FYI I'm *very* eager to examine this in detail but am on vacation this week. -- 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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1961300602
##
solr/solrj/src/java/org/apache/solr/common/util/Utils.java:
##
@@ -290,7 +290,7 @@ public static Object fromJSON(byte[] utf8) {
}
public static Object fromJSON(byte[] utf8, int offset, int length) {
-return fromJSON(utf8, offset, length, STANDARDOBJBUILDER);
+return fromJSON(utf8, offset, length, SIMPLEORDEREDMAPOBJBUILDER);
Review Comment:
@dsmiley How do we continue on this issue? Based on what I've described
above, it seems fine to me to run these tests with
solr.solrj.javabin.mapAsNamedList=false. What do you think?
--
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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1954136952
##
solr/solrj/src/java/org/apache/solr/common/util/TextWriter.java:
##
@@ -84,11 +84,11 @@ default void writeVal(String name, Object val, boolean raw)
throws IOException {
}
} else if (val instanceof IteratorWriter) {
writeIterator(name, (IteratorWriter) val, raw);
-} else if (val instanceof MapWriter) {
+} else if (val instanceof MapWriter && !(val instanceof SimpleOrderedMap))
{
writeMap(name, (MapWriter) val);
} else if (val instanceof ReflectWritable) {
writeVal(name, Utils.getReflectWriter(val));
-} else if (val instanceof MapSerializable) {
+} else if (val instanceof MapSerializable && !(val instanceof
SimpleOrderedMap)) {
// todo find a better way to reuse the map more efficiently
writeMap(name, ((MapSerializable) val).toMap(new LinkedHashMap<>()),
false, true);
} else if (val instanceof Map) {
Review Comment:
I've tried rearranging the order before adding the instanceof-checks, but
much code, or at least many tests seem to depend on this order.
If I just move up the 'instanceof Map' check as you suggest, I have 42 new
tests failing.
--
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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1953918318
##
solr/solrj/src/java/org/apache/solr/common/util/Utils.java:
##
@@ -290,7 +290,7 @@ public static Object fromJSON(byte[] utf8) {
}
public static Object fromJSON(byte[] utf8, int offset, int length) {
-return fromJSON(utf8, offset, length, STANDARDOBJBUILDER);
+return fromJSON(utf8, offset, length, SIMPLEORDEREDMAPOBJBUILDER);
Review Comment:
Looking at a test case
ClusterStateProviderTest.testClusterStateProviderEmptySolrVersion,
Http2ClusterStateProvider is using JavaBinCode to deserialize, hence we get now
a SOM, ZkClientClusterStateProvider is using JSON and usees the Utils.fromJson
to deserialize, and it gets a LinkedHashMap. In this test it expects the same
data structure from both Http2ClusterStateProvider and
ZkClientClusterStateProvider, hence it's failing now.
If I change the Utils to return SMO instead of a LinkedHashMap, as I did in
this change, all the tests in ClusterStateProviderTest pass, but it breaks at
least 17 other tests.
--
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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1953918318
##
solr/solrj/src/java/org/apache/solr/common/util/Utils.java:
##
@@ -290,7 +290,7 @@ public static Object fromJSON(byte[] utf8) {
}
public static Object fromJSON(byte[] utf8, int offset, int length) {
-return fromJSON(utf8, offset, length, STANDARDOBJBUILDER);
+return fromJSON(utf8, offset, length, SIMPLEORDEREDMAPOBJBUILDER);
Review Comment:
Looking at a test case
ClusterStateProviderTest.testClusterStateProviderEmptySolrVersion,
Http2ClusterStateProvider is using JavaBinCode to deserialize, hence we get now
a SOM, ZkClientClusterStateProvider is using JSON and use the Utils.fromJson to
deserialize, and it still gets a LinkedHashMap. In this test it expects the
same data structure from both Http2ClusterStateProvider and
ZkClientClusterStateProvider, hence it's failing now.
If I change the Utils to return SMO instead of a LinkedHashMap, as I did in
this change, all the tests in ClusterStateProviderTest pass, but it breaks 17
other tests.
--
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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1953918318
##
solr/solrj/src/java/org/apache/solr/common/util/Utils.java:
##
@@ -290,7 +290,7 @@ public static Object fromJSON(byte[] utf8) {
}
public static Object fromJSON(byte[] utf8, int offset, int length) {
-return fromJSON(utf8, offset, length, STANDARDOBJBUILDER);
+return fromJSON(utf8, offset, length, SIMPLEORDEREDMAPOBJBUILDER);
Review Comment:
Looking at a test case
ClusterStateProviderTest.testClusterStateProviderEmptySolrVersion,
Http2ClusterStateProvider is using JavaBinCode to deserialize, hence we get now
a SOM, ZkClientClusterStateProvider is using JSON and use the Utils.fromJson to
deserialize, hence it still gets LinkedHashMap. In this test it expects the
same data structure from both Http2ClusterStateProvider and
ZkClientClusterStateProvider, hence it's failing now.
If I change the Utils to return SMO instead of a LinkedHashMap, as I did in
this change, all the tests in ClusterStateProviderTest pass, but it breaks 17
other tests.
--
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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1953918318
##
solr/solrj/src/java/org/apache/solr/common/util/Utils.java:
##
@@ -290,7 +290,7 @@ public static Object fromJSON(byte[] utf8) {
}
public static Object fromJSON(byte[] utf8, int offset, int length) {
-return fromJSON(utf8, offset, length, STANDARDOBJBUILDER);
+return fromJSON(utf8, offset, length, SIMPLEORDEREDMAPOBJBUILDER);
Review Comment:
Looking at a test case
ClusterStateProviderTest.testClusterStateProviderEmptySolrVersion,
Http2ClusterStateProvider is using JavaBinCode to deserialize, hence we get now
a SOM, ZkClientClusterStateProvider is using JSON and use the Utils.fromJson to
deserialize, hence it still gets LinkedHashMap. In this test it expects the
same data structure from both Http2ClusterStateProvider and
ZkClientClusterStateProvider, hence it's failing now.
If I change the Utils to return SMO instead of a LinkedHashMap, as I did in
this change, we break other tests.
--
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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
dsmiley commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1953556107
##
solr/solrj/src/java/org/apache/solr/common/util/TextWriter.java:
##
@@ -84,11 +84,11 @@ default void writeVal(String name, Object val, boolean raw)
throws IOException {
}
} else if (val instanceof IteratorWriter) {
writeIterator(name, (IteratorWriter) val, raw);
-} else if (val instanceof MapWriter) {
+} else if (val instanceof MapWriter && !(val instanceof SimpleOrderedMap))
{
writeMap(name, (MapWriter) val);
} else if (val instanceof ReflectWritable) {
writeVal(name, Utils.getReflectWriter(val));
-} else if (val instanceof MapSerializable) {
+} else if (val instanceof MapSerializable && !(val instanceof
SimpleOrderedMap)) {
Review Comment:
why this line? SOM isn't MapSerializable
##
solr/solrj/src/java/org/apache/solr/common/util/TextWriter.java:
##
@@ -84,11 +84,11 @@ default void writeVal(String name, Object val, boolean raw)
throws IOException {
}
} else if (val instanceof IteratorWriter) {
writeIterator(name, (IteratorWriter) val, raw);
-} else if (val instanceof MapWriter) {
+} else if (val instanceof MapWriter && !(val instanceof SimpleOrderedMap))
{
writeMap(name, (MapWriter) val);
} else if (val instanceof ReflectWritable) {
writeVal(name, Utils.getReflectWriter(val));
-} else if (val instanceof MapSerializable) {
+} else if (val instanceof MapSerializable && !(val instanceof
SimpleOrderedMap)) {
// todo find a better way to reuse the map more efficiently
writeMap(name, ((MapSerializable) val).toMap(new LinkedHashMap<>()),
false, true);
} else if (val instanceof Map) {
Review Comment:
this should simply be ranked higher than MapWriter, where it will then apply
to SimpleOrderedMap.
##
solr/solrj/src/java/org/apache/solr/common/util/Utils.java:
##
@@ -290,7 +290,7 @@ public static Object fromJSON(byte[] utf8) {
}
public static Object fromJSON(byte[] utf8, int offset, int length) {
-return fromJSON(utf8, offset, length, STANDARDOBJBUILDER);
+return fromJSON(utf8, offset, length, SIMPLEORDEREDMAPOBJBUILDER);
Review Comment:
No; we should understand why the test or underlying functionality are
breaking and then make decisions based on that.
--
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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1953320836
##
solr/solrj/src/java/org/apache/solr/common/util/Utils.java:
##
@@ -290,7 +290,7 @@ public static Object fromJSON(byte[] utf8) {
}
public static Object fromJSON(byte[] utf8, int offset, int length) {
-return fromJSON(utf8, offset, length, STANDARDOBJBUILDER);
+return fromJSON(utf8, offset, length, SIMPLEORDEREDMAPOBJBUILDER);
Review Comment:
this breaks another 17 tests.
@dsmiley Do you think it is OK to run the test ClusterStateProviderTest with
solr.solrj.javabin.mapAsNamedList=false?
--
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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1947968424
##
solr/solrj/src/test/org/apache/solr/common/util/TestJavaBinCodec.java:
##
@@ -293,7 +306,7 @@ public void testForwardCompat() throws IOException {
for (int i = 1;
i < currentFormatBytes.length;
i++) { // ignore the first byte. It is version information
- assertEquals(newFormatBytes[i], currentFormatBytes[i]);
+ assertEquals("for i:" + i, newFormatBytes[i], currentFormatBytes[i]);
Review Comment:
> ClusterStateProviderTest: looks like just needs to convert both maps to
the same type (e.g. wrap in HashMap
Where would I do that? There is really a lot happening in those tests and I
still have not managed to fully wrap my head around 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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
dsmiley commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1947924419
##
solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java:
##
@@ -863,6 +871,21 @@ protected Map readMap(DataInputInputStream
dis, int sz) throws I
return m;
}
+ @SuppressWarnings({"rawtypes", "unchecked"})
+ protected Map
readMapAsSimpleOrderedMapForStringKeys(DataInputInputStream dis, int sz)
+ throws IOException {
+
+SimpleOrderedMap entries = new SimpleOrderedMap<>(sz);
+
+for (int i = 0; i < sz; i++) {
+ Object key = readVal(dis);
+ assert key instanceof String;
+ Object val = readVal(dis);
+ entries.put((String) key, val);
Review Comment:
It underscores my concern about making SimpleOrderedMap mutable. I still
question 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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1947912506
##
solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java:
##
@@ -863,6 +871,21 @@ protected Map readMap(DataInputInputStream
dis, int sz) throws I
return m;
}
+ @SuppressWarnings({"rawtypes", "unchecked"})
+ protected Map
readMapAsSimpleOrderedMapForStringKeys(DataInputInputStream dis, int sz)
+ throws IOException {
+
+SimpleOrderedMap entries = new SimpleOrderedMap<>(sz);
+
+for (int i = 0; i < sz; i++) {
+ Object key = readVal(dis);
+ assert key instanceof String;
+ Object val = readVal(dis);
+ entries.put((String) key, val);
Review Comment:
oh, I am glad at least one of us knows what we are doing 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: [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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
dsmiley commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1947912403
##
solr/solrj/src/test/org/apache/solr/common/util/TestJavaBinCodec.java:
##
@@ -293,7 +306,7 @@ public void testForwardCompat() throws IOException {
for (int i = 1;
i < currentFormatBytes.length;
i++) { // ignore the first byte. It is version information
- assertEquals(newFormatBytes[i], currentFormatBytes[i]);
+ assertEquals("for i:" + i, newFormatBytes[i], currentFormatBytes[i]);
Review Comment:
TestUpdateRequestCodec: dunno what to make of that without debugging;
hopefully you can make recommendations. Not surprised to see this _kind of
issue_ where some code is doing instanceof checks in a certain order, written
at a time when no NamedList was a Map but now a prominent (and even most
popular, I'd say) is actually one. You might want to search the codebase for
instanceof checks against NamedList to see if there are other areas to improve.
I have some WIP for Utils.java
ClusterStateProviderTest: looks like just needs to convert both maps to the
same type (e.g. wrap in HashMap) ?
--
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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1947903626
##
solr/solrj/src/test/org/apache/solr/common/util/TestJavaBinCodec.java:
##
@@ -293,7 +306,7 @@ public void testForwardCompat() throws IOException {
for (int i = 1;
i < currentFormatBytes.length;
i++) { // ignore the first byte. It is version information
- assertEquals(newFormatBytes[i], currentFormatBytes[i]);
+ assertEquals("for i:" + i, newFormatBytes[i], currentFormatBytes[i]);
Review Comment:
@dsmiley
We have now 9 failing tests, here my findings:
TestFastJavabinDecoder
testSimple-> the assert on line 136 fails due to the JSON with a SOM in it
has some extra line breaks
TestUpdateRequestCodec
testBackCompat4_5() -> failing because /solrj/updateReq_4_5.bin" has a Map
with SolrInputDocument as Key
testStreamableInputDocFormat -> because in
JavaBinUpdateRequestCodec.StreamingCodec#readOuterMostDocIterator line 315
instanceOf NamedList is now true, and it is checked before instanceof Map
ClusterStateProviderTest
6 tests are failing e.g. testClusterStateProvider -> assertThat on line
262 fails. ClusterState is using org.apache.solr.common.util.Utils to
deserialize the Json, which creates a LinkedHashMap and clusterStateHttp has
now a SOM in 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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1947903626
##
solr/solrj/src/test/org/apache/solr/common/util/TestJavaBinCodec.java:
##
@@ -293,7 +306,7 @@ public void testForwardCompat() throws IOException {
for (int i = 1;
i < currentFormatBytes.length;
i++) { // ignore the first byte. It is version information
- assertEquals(newFormatBytes[i], currentFormatBytes[i]);
+ assertEquals("for i:" + i, newFormatBytes[i], currentFormatBytes[i]);
Review Comment:
@dsmiley
We have now 9 failing test, here my findings:
TestFastJavabinDecoder
testSimple-> the assert on line 136 fails due to the JSON with a SOM in it
has some extra line breaks
TestUpdateRequestCodec
testBackCompat4_5() -> failing because /solrj/updateReq_4_5.bin" has a Map
with SolrInputDocument as Key
testStreamableInputDocFormat -> because in
JavaBinUpdateRequestCodec.StreamingCodec#readOuterMostDocIterator line 315
instanceOf NamedList is now true, and it is checked before instanceof Map
ClusterStateProviderTest
6 tests are failing e.g. testClusterStateProvider -> assertThat on line
262 fails. ClusterState is using org.apache.solr.common.util.Utils to
deserialize the Json, which creates a LinkedHashMap and clusterStateHttp has
now a SOM in 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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1947903626
##
solr/solrj/src/test/org/apache/solr/common/util/TestJavaBinCodec.java:
##
@@ -293,7 +306,7 @@ public void testForwardCompat() throws IOException {
for (int i = 1;
i < currentFormatBytes.length;
i++) { // ignore the first byte. It is version information
- assertEquals(newFormatBytes[i], currentFormatBytes[i]);
+ assertEquals("for i:" + i, newFormatBytes[i], currentFormatBytes[i]);
Review Comment:
@dsmiley
We have now 9 failing test, here my findings:
TestFastJavabinDecoder
testSimple-> the assert on line 136 fails due to the JSON with a SOM in it
has some extra line breaks
TestUpdateRequestCodec
testBackCompat4_5() -> failing because /solrj/updateReq_4_5.bin" has a Map
with SolrInputDocument as Key
testStreamableInputDocFormat -> because in
JavaBinUpdateRequestCodec.StreamingCodec#readOuterMostDocIterator line 315
instanceOf NamedList is now true, and it is checked before instanceof Map
ClusterStateProviderTest
6 tests are failing e.g. testClusterStateProvider -> ClusterState is using
org.apache.solr.common.util.Utils to deserialize the Json, which creates a
LinkedHashMap and clusterStateHttp has now a SOM in 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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
dsmiley commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1947895092
##
solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java:
##
@@ -863,6 +871,21 @@ protected Map readMap(DataInputInputStream
dis, int sz) throws I
return m;
}
+ @SuppressWarnings({"rawtypes", "unchecked"})
+ protected Map
readMapAsSimpleOrderedMapForStringKeys(DataInputInputStream dis, int sz)
+ throws IOException {
+
+SimpleOrderedMap entries = new SimpleOrderedMap<>(sz);
+
+for (int i = 0; i < sz; i++) {
+ Object key = readVal(dis);
+ assert key instanceof String;
+ Object val = readVal(dis);
+ entries.put((String) key, val);
Review Comment:
```suggestion
entries.add((String) key, val); // using NL.add() since key won't
repeat
```
##
solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java:
##
@@ -1436,4 +1459,8 @@ public void close() throws IOException {
daos.flushBuffer();
}
}
+
Review Comment:
TODO javadocs.
##
solr/solrj/src/test/org/apache/solr/common/util/TestJavaBinCodec.java:
##
@@ -409,12 +419,17 @@ private static byte[] getBytes(Object o, boolean
readAsCharSeq) throws IOExcepti
}
}
- private static Object getObject(byte[] bytes) throws IOException {
+ private static Object getObject(byte[] bytes, boolean mapAsNamedList) throws
IOException {
Review Comment:
don't need these getObject anymore, I think
--
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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
dsmiley commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1947899623
##
solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java:
##
@@ -863,6 +871,21 @@ protected Map readMap(DataInputInputStream
dis, int sz) throws I
return m;
}
+ @SuppressWarnings({"rawtypes", "unchecked"})
+ protected Map
readMapAsSimpleOrderedMapForStringKeys(DataInputInputStream dis, int sz)
+ throws IOException {
+
+SimpleOrderedMap entries = new SimpleOrderedMap<>(sz);
+
+for (int i = 0; i < sz; i++) {
+ Object key = readVal(dis);
+ assert key instanceof String;
+ Object val = readVal(dis);
+ entries.put((String) key, val);
Review Comment:
That was an `O(N^2)` performance bug, by the way 😱
--
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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
dsmiley commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1947894348
##
solr/solrj/src/test/org/apache/solr/common/util/TestJavaBinCodec.java:
##
@@ -293,7 +306,7 @@ public void testForwardCompat() throws IOException {
for (int i = 1;
i < currentFormatBytes.length;
i++) { // ignore the first byte. It is version information
- assertEquals(newFormatBytes[i], currentFormatBytes[i]);
+ assertEquals("for i:" + i, newFormatBytes[i], currentFormatBytes[i]);
Review Comment:
Agree. Of course there needs to be some test of this specific change being
added. Just one unit test is fine. If that seems lacking, remember that the
totality of Solr tests will also be running with this by default, and thus if
there's an incompatibility we'll probably see 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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1947547033
##
solr/solrj/src/test/org/apache/solr/common/util/TestJavaBinCodec.java:
##
@@ -293,7 +306,7 @@ public void testForwardCompat() throws IOException {
for (int i = 1;
i < currentFormatBytes.length;
i++) { // ignore the first byte. It is version information
- assertEquals(newFormatBytes[i], currentFormatBytes[i]);
+ assertEquals("for i:" + i, newFormatBytes[i], currentFormatBytes[i]);
Review Comment:
All the tests in TestJavaBinCodec do not need to pass with
javabin.setMapAsNamedList=true even though it will be true by default? I would
have thought all these test need to pass in order to ensure we do not break
anything by changing to SOM.
In this case, there is also no need to update /solrj/javabin_backcompat.bin?
--
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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
dsmiley commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1947486318
##
solr/solrj/src/test/org/apache/solr/common/util/TestJavaBinCodec.java:
##
@@ -293,7 +306,7 @@ public void testForwardCompat() throws IOException {
for (int i = 1;
i < currentFormatBytes.length;
i++) { // ignore the first byte. It is version information
- assertEquals(newFormatBytes[i], currentFormatBytes[i]);
+ assertEquals("for i:" + i, newFormatBytes[i], currentFormatBytes[i]);
Review Comment:
TestJavaBinCodec should test without this setting. This is another reason
to make this a setter or something, which is clearer than a system property.
--
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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1947220379
##
solr/solrj/src/test/org/apache/solr/common/util/TestJavaBinCodec.java:
##
@@ -293,7 +306,7 @@ public void testForwardCompat() throws IOException {
for (int i = 1;
i < currentFormatBytes.length;
i++) { // ignore the first byte. It is version information
- assertEquals(newFormatBytes[i], currentFormatBytes[i]);
+ assertEquals("for i:" + i, newFormatBytes[i], currentFormatBytes[i]);
Review Comment:
After rewriting the .bin file with the changed output from
org.apache.solr.common.util.TestJavaBinCodec#generateAllDataTypes tests like
org.apache.solr.common.util.TestJavaBinCodec#testBackCompat are passing now.
But this one testForwardCompat which compares all the bytes fails since the
bytes are not the same anymore.
Is it possible I did something wrong when recreated the bin-file?
So the object in the unmarshalle version are the same, but the bytes are for
some reason not.
Is just took the output from generateAllDataTypes(), called
javabin.marshall(matchObj, output) with it and wrote the result via a
FileOutputStream to a file.
--
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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1947220379
##
solr/solrj/src/test/org/apache/solr/common/util/TestJavaBinCodec.java:
##
@@ -293,7 +306,7 @@ public void testForwardCompat() throws IOException {
for (int i = 1;
i < currentFormatBytes.length;
i++) { // ignore the first byte. It is version information
- assertEquals(newFormatBytes[i], currentFormatBytes[i]);
+ assertEquals("for i:" + i, newFormatBytes[i], currentFormatBytes[i]);
Review Comment:
After rewriting the .bin file with the changed output from
org.apache.solr.common.util.TestJavaBinCodec#generateAllDataTypes tests like
org.apache.solr.common.util.TestJavaBinCodec#testBackCompat are passing now.
But this one testForwardCompat which compares all the bytes fails since the
bytes are not the same anymore.
Is it possible I did something wrong when recreated the bin-file?
Is just took the output from generateAllDataTypes(), called
javabin.marshall(matchObj, output) with it and wrote the result via a
FileOutputStream to a file.
So the object in the unmarshalle version are the same, but the bytes are not
--
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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
dsmiley commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1946576896
##
solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java:
##
@@ -863,6 +868,26 @@ protected Map readMap(DataInputInputStream
dis, int sz) throws I
return m;
}
+ @SuppressWarnings({"rawtypes", "unchecked"})
+ protected Map
readMapAsSimpleOrderedMapForStringKeys(DataInputInputStream dis, int sz)
+ throws IOException {
+Map m = null;
+for (int i = 0; i < sz; i++) {
+ Object key = readVal(dis);
+
+ if (m == null) {
+if (key instanceof String) {
+ m = new SimpleOrderedMap<>(sz);
+} else {
+ m = newMap(sz);
+}
+ }
+ Object val = readVal(dis);
+ m.put(key, val);
+}
+return m;
+ }
+
Review Comment:
Yes; consider this a hypothesis to be tested (by the results of the totality
of Solr's test suite).
--
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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1946046611
##
solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java:
##
@@ -863,6 +868,26 @@ protected Map readMap(DataInputInputStream
dis, int sz) throws I
return m;
}
+ @SuppressWarnings({"rawtypes", "unchecked"})
+ protected Map
readMapAsSimpleOrderedMapForStringKeys(DataInputInputStream dis, int sz)
+ throws IOException {
+Map m = null;
+for (int i = 0; i < sz; i++) {
+ Object key = readVal(dis);
+
+ if (m == null) {
+if (key instanceof String) {
+ m = new SimpleOrderedMap<>(sz);
+} else {
+ m = newMap(sz);
+}
+ }
+ Object val = readVal(dis);
+ m.put(key, val);
+}
+return m;
+ }
+
Review Comment:
Just casting the key to a String, or having an assert on it, will fail three
unit test of TestJavaBinCodec. E.g. the .bin used in the tests -
org.apache.solr.common.util.TestJavaBinCodec#SOLRJ_JAVABIN_BACKCOMPAT_BIN -
holds a Map with Integer as key. So you are saying that this is not
representing a realistic case and had to be changed?
--
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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
dsmiley commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1945550364
##
solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java:
##
@@ -863,6 +868,26 @@ protected Map readMap(DataInputInputStream
dis, int sz) throws I
return m;
}
+ @SuppressWarnings({"rawtypes", "unchecked"})
+ protected Map
readMapAsSimpleOrderedMapForStringKeys(DataInputInputStream dis, int sz)
+ throws IOException {
+Map m = null;
+for (int i = 0; i < sz; i++) {
+ Object key = readVal(dis);
+
+ if (m == null) {
+if (key instanceof String) {
+ m = new SimpleOrderedMap<>(sz);
+} else {
+ m = newMap(sz);
+}
+ }
+ Object val = readVal(dis);
+ m.put(key, val);
+}
+return m;
+ }
+
Review Comment:
Oh man I didn't think of that! Let's say we treat keys as a String no
matter what. I'm suspicious that our response formats would ever have a map
whose key isn't a string. Maybe add an assert if the key isn't already a
String so that we can more readily identify problems. (not sure if may read
CharSequence but we can consider that as a String).
##
solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java:
##
@@ -848,9 +848,14 @@ protected Map newMap(int size) {
return size < 0 ? new LinkedHashMap<>() :
CollectionUtil.newLinkedHashMap(size);
}
- public Map readMap(DataInputInputStream dis) throws
IOException {
+ public Map readMap(DataInputInputStream dis) throws IOException {
int sz = readVInt(dis);
-return readMap(dis, sz);
+
+if (EnvUtils.getPropertyAsBool("solr.solrj.javabin.mapAsNamedList", true))
{
Review Comment:
Reading this setting on _every single map decode_ is quite bad for
performance. This setting should be a field on the codec, and that which could
be toggled with a setter (if useful in a test).
--
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-17635: unmarshalling Map to SimpleOrderedMap if key i of type St… [solr]
renatoh commented on code in PR #3163:
URL: https://github.com/apache/solr/pull/3163#discussion_r1945462495
##
solr/solrj/src/java/org/apache/solr/common/util/JavaBinCodec.java:
##
@@ -863,6 +868,26 @@ protected Map readMap(DataInputInputStream
dis, int sz) throws I
return m;
}
+ @SuppressWarnings({"rawtypes", "unchecked"})
+ protected Map
readMapAsSimpleOrderedMapForStringKeys(DataInputInputStream dis, int sz)
+ throws IOException {
+Map m = null;
+for (int i = 0; i < sz; i++) {
+ Object key = readVal(dis);
+
+ if (m == null) {
+if (key instanceof String) {
+ m = new SimpleOrderedMap<>(sz);
+} else {
+ m = newMap(sz);
+}
+ }
+ Object val = readVal(dis);
+ m.put(key, val);
+}
+return m;
+ }
+
Review Comment:
@dsmiley FYI, this is marked as drafted.
We cannot simply create a SimpleOrderedMap since the key of the Map can also
me something other than a String e.g. an Integer. The only way I can think of
to overcome that is to read the first key and then do an instanceof check on
it. Pretty ugly considering I have to use a raw Map.
--
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]
