dsmiley commented on code in PR #3207:
URL: https://github.com/apache/solr/pull/3207#discussion_r1976523606
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/JavaBinUpdateRequestCodec.java:
##########
@@ -49,15 +42,6 @@
* @since solr 1.4
*/
public class JavaBinUpdateRequestCodec {
- private boolean readStringAsCharSeq = false;
-
- public JavaBinUpdateRequestCodec setReadStringAsCharSeq(boolean flag) {
Review Comment:
unused; always false
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/JavaBinUpdateRequestCodec.java:
##########
@@ -277,67 +208,52 @@ public List<Object> readIterator(DataInputInputStream
fis) throws IOException {
// special treatment for first outermost Iterator
// (the list of documents)
seenOuterMostDocIterator = true;
- return readOuterMostDocIterator(fis);
+ readDocs(fis);
+ return List.of(); // bogus; already processed
}
- private List<Object> readOuterMostDocIterator(DataInputInputStream fis)
throws IOException {
- if (namedList[0] == null) namedList[0] = new NamedList<>();
- NamedList<?> params = (NamedList<?>) namedList[0].get("params");
- if (params == null) params = new NamedList<>();
- updateRequest.setParams(new ModifiableSolrParams(params.toSolrParams()));
- if (handler == null) return super.readIterator(fis);
- Integer commitWithin = null;
- Boolean overwrite = null;
- Object o = null;
- super.readStringAsCharSeq =
JavaBinUpdateRequestCodec.this.readStringAsCharSeq;
- try {
- while (true) {
- if (o == null) {
- o = readVal(fis);
- }
-
- if (o == END_OBJ) {
- break;
- }
+ private void readDocs(DataInputInputStream fis) throws IOException {
+ if (resultNamedList == null) resultNamedList = new NamedList<>();
- SolrInputDocument sdoc = null;
- if (o instanceof List) {
- @SuppressWarnings("unchecked")
- List<NamedList<?>> list = (List<NamedList<?>>) o;
- sdoc = listToSolrInputDocument(list);
- } else if (o instanceof NamedList) {
- UpdateRequest req = new UpdateRequest();
- req.setParams(new ModifiableSolrParams(((NamedList)
o).toSolrParams()));
- handler.update(null, req, null, null);
- } else if (o instanceof Map.Entry) {
- @SuppressWarnings("unchecked")
- Map.Entry<SolrInputDocument, Map<?, ?>> entry =
- (Map.Entry<SolrInputDocument, Map<?, ?>>) o;
- sdoc = entry.getKey();
- Map<?, ?> p = entry.getValue();
- if (p != null) {
- commitWithin = (Integer) p.get(UpdateRequest.COMMIT_WITHIN);
- overwrite = (Boolean) p.get(UpdateRequest.OVERWRITE);
- }
- } else if (o instanceof SolrInputDocument) {
- sdoc = (SolrInputDocument) o;
- } else if (o instanceof Map) {
- sdoc = convertMapToSolrInputDoc((Map) o);
- }
+ UpdateRequest updateRequest = new UpdateRequest();
+ NamedList<?> params = (NamedList<?>) resultNamedList.get("params"); //
always precedes docs
+ if (params != null) {
+
updateRequest.setParams(ModifiableSolrParams.of(params.toSolrParams()));
+ }
- // peek at the next object to see if we're at the end
- o = readVal(fis);
- if (o == END_OBJ) {
- // indicate that we've hit the last doc in the batch, used to
enable optimizations when
- // doing replication
- updateRequest.lastDocInBatch();
+ Object o = readVal(fis);
+ while (o != END_OBJ) {
+ Integer commitWithin = null;
+ Boolean overwrite = null;
+
+ SolrInputDocument sdoc;
+ if (o instanceof Map.Entry) { // doc + options. UpdateRequest
"docsMap"
+ @SuppressWarnings("unchecked")
+ Map.Entry<SolrInputDocument, Map<?, ?>> entry =
+ (Map.Entry<SolrInputDocument, Map<?, ?>>) o;
+ sdoc = entry.getKey();
+ Map<?, ?> p = entry.getValue();
+ if (p != null) {
+ commitWithin = (Integer) p.get(UpdateRequest.COMMIT_WITHIN);
+ overwrite = (Boolean) p.get(UpdateRequest.OVERWRITE);
}
+ } else if (o instanceof SolrInputDocument d) { // doc. UpdateRequest
"docs""
+ sdoc = d;
+ } else if (o instanceof Map<?, ?> m) { // doc. To imitate JSON style.
SOLR-13731
+ sdoc = convertMapToSolrInputDoc(m);
+ } else {
+ throw new IllegalStateException("Unexpected data type: " +
o.getClass());
Review Comment:
Not being silent about the unexpected! I'll change this to
`SolrException(ErrorCode.BAD_REQUEST`
##########
solr/solrj/src/test/org/apache/solr/client/solrj/request/TestUpdateRequestCodec.java:
##########
@@ -247,13 +247,14 @@ public void testBackCompat4_5() throws IOException {
InputStream is =
getClass().getResourceAsStream("/solrj/updateReq_4_5.bin");
assertNotNull("updateReq_4_5.bin was not found", is);
+ List<SolrInputDocument> unmarshalledDocs = new ArrayList<>();
UpdateRequest updateUnmarshalled =
new JavaBinUpdateRequestCodec()
.unmarshal(
is,
(document, req, commitWithin, override) -> {
if (commitWithin == null) {
- req.add(document);
Review Comment:
The test needed an update because JavaBinUpdateRequestCodec no longer uses
the same UpdateRequest between the add-docs and delete-docs parts, which
doesn't matter. Arguably, the callback shouldn't even take an UpdateRequest
anyway.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/JavaBinUpdateRequestCodec.java:
##########
@@ -277,67 +208,52 @@ public List<Object> readIterator(DataInputInputStream
fis) throws IOException {
// special treatment for first outermost Iterator
// (the list of documents)
seenOuterMostDocIterator = true;
- return readOuterMostDocIterator(fis);
+ readDocs(fis);
+ return List.of(); // bogus; already processed
}
- private List<Object> readOuterMostDocIterator(DataInputInputStream fis)
throws IOException {
- if (namedList[0] == null) namedList[0] = new NamedList<>();
- NamedList<?> params = (NamedList<?>) namedList[0].get("params");
- if (params == null) params = new NamedList<>();
- updateRequest.setParams(new ModifiableSolrParams(params.toSolrParams()));
- if (handler == null) return super.readIterator(fis);
- Integer commitWithin = null;
- Boolean overwrite = null;
- Object o = null;
- super.readStringAsCharSeq =
JavaBinUpdateRequestCodec.this.readStringAsCharSeq;
- try {
- while (true) {
- if (o == null) {
- o = readVal(fis);
- }
-
- if (o == END_OBJ) {
- break;
- }
+ private void readDocs(DataInputInputStream fis) throws IOException {
+ if (resultNamedList == null) resultNamedList = new NamedList<>();
- SolrInputDocument sdoc = null;
- if (o instanceof List) {
- @SuppressWarnings("unchecked")
- List<NamedList<?>> list = (List<NamedList<?>>) o;
- sdoc = listToSolrInputDocument(list);
- } else if (o instanceof NamedList) {
- UpdateRequest req = new UpdateRequest();
- req.setParams(new ModifiableSolrParams(((NamedList)
o).toSolrParams()));
- handler.update(null, req, null, null);
Review Comment:
removed
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/JavaBinUpdateRequestCodec.java:
##########
@@ -277,67 +208,52 @@ public List<Object> readIterator(DataInputInputStream
fis) throws IOException {
// special treatment for first outermost Iterator
// (the list of documents)
seenOuterMostDocIterator = true;
- return readOuterMostDocIterator(fis);
+ readDocs(fis);
+ return List.of(); // bogus; already processed
}
- private List<Object> readOuterMostDocIterator(DataInputInputStream fis)
throws IOException {
- if (namedList[0] == null) namedList[0] = new NamedList<>();
- NamedList<?> params = (NamedList<?>) namedList[0].get("params");
- if (params == null) params = new NamedList<>();
- updateRequest.setParams(new ModifiableSolrParams(params.toSolrParams()));
- if (handler == null) return super.readIterator(fis);
- Integer commitWithin = null;
- Boolean overwrite = null;
- Object o = null;
- super.readStringAsCharSeq =
JavaBinUpdateRequestCodec.this.readStringAsCharSeq;
- try {
- while (true) {
- if (o == null) {
- o = readVal(fis);
- }
-
- if (o == END_OBJ) {
- break;
- }
+ private void readDocs(DataInputInputStream fis) throws IOException {
+ if (resultNamedList == null) resultNamedList = new NamedList<>();
- SolrInputDocument sdoc = null;
- if (o instanceof List) {
- @SuppressWarnings("unchecked")
- List<NamedList<?>> list = (List<NamedList<?>>) o;
- sdoc = listToSolrInputDocument(list);
Review Comment:
removed
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/JavaBinUpdateRequestCodec.java:
##########
@@ -69,32 +53,32 @@ public JavaBinUpdateRequestCodec
setReadStringAsCharSeq(boolean flag) {
*/
public void marshal(UpdateRequest updateRequest, OutputStream os) throws
IOException {
Review Comment:
simply moving code around for clarity in this method
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/JavaBinUpdateRequestCodec.java:
##########
@@ -114,65 +98,48 @@ public void marshal(UpdateRequest updateRequest,
OutputStream os) throws IOExcep
@SuppressWarnings({"unchecked"})
public UpdateRequest unmarshal(InputStream is, final StreamingUpdateHandler
handler)
throws IOException {
- final UpdateRequest updateRequest = new UpdateRequest();
- List<List<NamedList<?>>> doclist;
- List<Entry<SolrInputDocument, Map<Object, Object>>> docMap;
- List<String> delById;
- Map<String, Map<String, Object>> delByIdMap;
- List<String> delByQ;
- final NamedList<?>[] namedList = new NamedList<?>[1];
- try (JavaBinCodec codec = new StreamingCodec(namedList, updateRequest,
handler)) {
- codec.unmarshal(is);
+ final NamedList<Object> namedList;
+
+ // process documents:
+
+ // reads documents, sending to handler. Other data is in NamedList
+ try (var codec = new StreamingCodec(handler)) {
+ namedList = codec.unmarshal(is);
}
- // NOTE: if the update request contains only delete commands the params
- // must be loaded now
- if (updateRequest.getParams().iterator().hasNext() == false) { // no params
- NamedList<?> params = (NamedList<?>) namedList[0].get("params");
+ // process deletes:
+
+ final UpdateRequest updateRequest = new UpdateRequest();
+ {
+ NamedList<?> params = (NamedList<?>) namedList.get("params");
if (params != null) {
- updateRequest.setParams(new
ModifiableSolrParams(params.toSolrParams()));
+
updateRequest.setParams(ModifiableSolrParams.of(params.toSolrParams()));
}
}
- delById = (List<String>) namedList[0].get("delById");
- delByIdMap = (Map<String, Map<String, Object>>)
namedList[0].get("delByIdMap");
- delByQ = (List<String>) namedList[0].get("delByQ");
- doclist = (List) namedList[0].get("docs");
- Object docsMapObj = namedList[0].get("docsMap");
-
- if (docsMapObj instanceof Map) { // SOLR-5762
- docMap = new ArrayList<>(((Map) docsMapObj).entrySet());
- } else {
- docMap = (List<Entry<SolrInputDocument, Map<Object, Object>>>)
docsMapObj;
- }
- // we don't add any docs, because they were already processed
- // deletes are handled later, and must be passed back on the UpdateRequest
Review Comment:
the code improvements made this comment needless
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/JavaBinUpdateRequestCodec.java:
##########
@@ -114,65 +98,48 @@ public void marshal(UpdateRequest updateRequest,
OutputStream os) throws IOExcep
@SuppressWarnings({"unchecked"})
public UpdateRequest unmarshal(InputStream is, final StreamingUpdateHandler
handler)
throws IOException {
- final UpdateRequest updateRequest = new UpdateRequest();
- List<List<NamedList<?>>> doclist;
- List<Entry<SolrInputDocument, Map<Object, Object>>> docMap;
- List<String> delById;
- Map<String, Map<String, Object>> delByIdMap;
- List<String> delByQ;
- final NamedList<?>[] namedList = new NamedList<?>[1];
- try (JavaBinCodec codec = new StreamingCodec(namedList, updateRequest,
handler)) {
- codec.unmarshal(is);
+ final NamedList<Object> namedList;
+
+ // process documents:
+
+ // reads documents, sending to handler. Other data is in NamedList
+ try (var codec = new StreamingCodec(handler)) {
+ namedList = codec.unmarshal(is);
Review Comment:
big improvement, simplifying the request/response contract here with the
internal StreamingCodec. No need to have the array of NamedList or pass that
in or pass an UpdateRequest. Now a simple NamedList response.
--
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]
