From a633f75247c05913b1643eb52d519957296fe521 Mon Sep 17 00:00:00 2001 From: Christine Poerschke Date: Mon, 26 Jul 2021 19:14:54 +0100 Subject: [PATCH 1/9] SOLR-15546: ShortestPathStream to handle ids with colons. --- solr/CHANGES.txt | 2 ++ .../solrj/io/graph/ShortestPathStream.java | 2 +- .../solr/client/solrj/io/graph/GraphTest.java | 19 +++++++++++++++++++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index a5d5293aaeb..10f3c48e850 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -394,6 +394,8 @@ Bug Fixes * SOLR-15531: SignificantTerms streaming function should not fail on empty collections (Benedikt Arnold via Mike Drob) +* SOLR-15546: ShortestPathStream to handle ids with colons. (Kenny Knecht, Christine Poerschke) + Other Changes --------------------- (No changes) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java b/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java index f19b279c635..f62180ca63d 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java @@ -433,7 +433,7 @@ public List call() { StringBuffer nodeQuery = new StringBuffer(); for(String node : nodes) { - nodeQuery.append(node).append(" "); + nodeQuery.append('"').append(node).append('"').append(" "); } String q = fromField + ":(" + nodeQuery.toString().trim() + ")"; diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java index 038bd0ea005..e3f4abd66dd 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java @@ -221,6 +221,25 @@ public void testShortestPathStream() throws Exception { assertTrue(paths.contains("[jim, stan, mary, steve]")); + // SOLR-15546: fromNode or toNode contains colon + for (String fromNode : new String[] { "foo", "https://foo" }) { + for (String toNode : new String[] { "bar", "https://bar" }) { + final ShortestPathStream spStream = new ShortestPathStream(zkHost, + "collection1", + fromNode, + toNode, + "from_s", + "to_s", + StreamingTest.mapParams("fq", "predicate_s:knows"), + 10, + 3, + 6); + + spStream.setStreamContext(context); + assertTrue(getTuples(spStream).isEmpty()); + } + } + cache.close(); } From add797e5ca146144d20adc271b0c84c20aeb940c Mon Sep 17 00:00:00 2001 From: Christine Poerschke Date: Thu, 29 Jul 2021 17:40:20 +0100 Subject: [PATCH 2/9] Revert "SOLR-15546: ShortestPathStream to handle ids with colons." This reverts commit a633f75247c05913b1643eb52d519957296fe521. --- solr/CHANGES.txt | 2 -- .../solrj/io/graph/ShortestPathStream.java | 2 +- .../solr/client/solrj/io/graph/GraphTest.java | 19 ------------------- 3 files changed, 1 insertion(+), 22 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 10f3c48e850..a5d5293aaeb 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -394,8 +394,6 @@ Bug Fixes * SOLR-15531: SignificantTerms streaming function should not fail on empty collections (Benedikt Arnold via Mike Drob) -* SOLR-15546: ShortestPathStream to handle ids with colons. (Kenny Knecht, Christine Poerschke) - Other Changes --------------------- (No changes) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java b/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java index f62180ca63d..f19b279c635 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java @@ -433,7 +433,7 @@ public List call() { StringBuffer nodeQuery = new StringBuffer(); for(String node : nodes) { - nodeQuery.append('"').append(node).append('"').append(" "); + nodeQuery.append(node).append(" "); } String q = fromField + ":(" + nodeQuery.toString().trim() + ")"; diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java index e3f4abd66dd..038bd0ea005 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java @@ -221,25 +221,6 @@ public void testShortestPathStream() throws Exception { assertTrue(paths.contains("[jim, stan, mary, steve]")); - // SOLR-15546: fromNode or toNode contains colon - for (String fromNode : new String[] { "foo", "https://foo" }) { - for (String toNode : new String[] { "bar", "https://bar" }) { - final ShortestPathStream spStream = new ShortestPathStream(zkHost, - "collection1", - fromNode, - toNode, - "from_s", - "to_s", - StreamingTest.mapParams("fq", "predicate_s:knows"), - 10, - 3, - 6); - - spStream.setStreamContext(context); - assertTrue(getTuples(spStream).isEmpty()); - } - } - cache.close(); } From a6d00cefc17beb6adb7ba88371712e815cc2d3f4 Mon Sep 17 00:00:00 2001 From: Christine Poerschke Date: Thu, 29 Jul 2021 18:00:34 +0100 Subject: [PATCH 3/9] add tests --- .../solr/client/solrj/io/graph/GraphTest.java | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java index 038bd0ea005..a8eff13edb0 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java @@ -88,6 +88,8 @@ public void testShortestPathStream() throws Exception { .add(id, "11", "from_s", "mary", "to_s", "max", "predicate_s", "knows") .add(id, "12", "from_s", "mary", "to_s", "jim", "predicate_s", "knows") .add(id, "13", "from_s", "mary", "to_s", "steve", "predicate_s", "knows") + .add(id, "14", "from_s", "https://aaa", "to_s", "https://bbb", "predicate_s", "links") + .add(id, "15", "from_s", "https://bbb", "to_s", "https://ccc", "predicate_s", "links") .commit(cluster.getSolrClient(), COLLECTION); List tuples = null; @@ -221,6 +223,56 @@ public void testShortestPathStream() throws Exception { assertTrue(paths.contains("[jim, stan, mary, steve]")); + final boolean with_SOLR_15546_fix = false; + + // SOLR-15546: fromNode and toNode contains colon + stream = new ShortestPathStream(zkHost, + "collection1", + with_SOLR_15546_fix && random().nextBoolean() ? "https://aaa" : "\"https://aaa\"", + "https://bbb", + "from_s", + "to_s", + StreamingTest.mapParams("fq", "predicate_s:links"), + 10, + 3, + 6); + + stream.setStreamContext(context); + paths = new HashSet<>(); + tuples = getTuples(stream); + assertTrue(tuples.size() == 1); + + for(Tuple tuple : tuples) { + paths.add(tuple.getStrings("path").toString()); + } + + assertTrue(paths.contains("[https://aaa, https://bbb]")); + + if (with_SOLR_15546_fix) { + // SOLR-15546: fromNode and toNode and interim node contains colon + stream = new ShortestPathStream(zkHost, + "collection1", + with_SOLR_15546_fix && random().nextBoolean() ? "https://aaa" : "\"https://aaa\"", + "https://ccc", + "from_s", + "to_s", + StreamingTest.mapParams("fq", "predicate_s:links"), + 10, + 3, + 6); + + stream.setStreamContext(context); + paths = new HashSet<>(); + tuples = getTuples(stream); + assertTrue(tuples.size() == 1); + + for(Tuple tuple : tuples) { + paths.add(tuple.getStrings("path").toString()); + } + + assertTrue(paths.contains("[https://aaa, https://bbb, https://ccc]")); + } + cache.close(); } From e4df59013b558bf941b3145176e257ee791b68c6 Mon Sep 17 00:00:00 2001 From: Christine Poerschke Date: Thu, 29 Jul 2021 18:02:40 +0100 Subject: [PATCH 4/9] change code --- .../solr/client/solrj/io/graph/ShortestPathStream.java | 6 +++++- .../org/apache/solr/client/solrj/io/graph/GraphTest.java | 8 ++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java b/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java index f19b279c635..ea311aa8221 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java @@ -433,7 +433,11 @@ public List call() { StringBuffer nodeQuery = new StringBuffer(); for(String node : nodes) { - nodeQuery.append(node).append(" "); + if (node.startsWith("\"") && node.endsWith("\"")) { + nodeQuery.append(node).append(" "); + } else { + nodeQuery.append('"').append(node).append('"').append(" "); + } } String q = fromField + ":(" + nodeQuery.toString().trim() + ")"; diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java index a8eff13edb0..c4c32fa25e7 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java @@ -223,12 +223,10 @@ public void testShortestPathStream() throws Exception { assertTrue(paths.contains("[jim, stan, mary, steve]")); - final boolean with_SOLR_15546_fix = false; - // SOLR-15546: fromNode and toNode contains colon stream = new ShortestPathStream(zkHost, "collection1", - with_SOLR_15546_fix && random().nextBoolean() ? "https://aaa" : "\"https://aaa\"", + random().nextBoolean() ? "https://aaa" : "\"https://aaa\"", "https://bbb", "from_s", "to_s", @@ -248,11 +246,10 @@ with_SOLR_15546_fix && random().nextBoolean() ? "https://aaa" : "\"https://aaa\" assertTrue(paths.contains("[https://aaa, https://bbb]")); - if (with_SOLR_15546_fix) { // SOLR-15546: fromNode and toNode and interim node contains colon stream = new ShortestPathStream(zkHost, "collection1", - with_SOLR_15546_fix && random().nextBoolean() ? "https://aaa" : "\"https://aaa\"", + random().nextBoolean() ? "https://aaa" : "\"https://aaa\"", "https://ccc", "from_s", "to_s", @@ -271,7 +268,6 @@ with_SOLR_15546_fix && random().nextBoolean() ? "https://aaa" : "\"https://aaa\" } assertTrue(paths.contains("[https://aaa, https://bbb, https://ccc]")); - } cache.close(); } From 66460c7ec1f9ebf34c0f81284ca58f80deb01a91 Mon Sep 17 00:00:00 2001 From: Christine Poerschke Date: Fri, 30 Jul 2021 18:44:22 +0100 Subject: [PATCH 5/9] Revert "change code" This reverts commit e4df59013b558bf941b3145176e257ee791b68c6. --- .../solr/client/solrj/io/graph/ShortestPathStream.java | 6 +----- .../org/apache/solr/client/solrj/io/graph/GraphTest.java | 8 ++++++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java b/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java index ea311aa8221..f19b279c635 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java @@ -433,11 +433,7 @@ public List call() { StringBuffer nodeQuery = new StringBuffer(); for(String node : nodes) { - if (node.startsWith("\"") && node.endsWith("\"")) { - nodeQuery.append(node).append(" "); - } else { - nodeQuery.append('"').append(node).append('"').append(" "); - } + nodeQuery.append(node).append(" "); } String q = fromField + ":(" + nodeQuery.toString().trim() + ")"; diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java index c4c32fa25e7..a8eff13edb0 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java @@ -223,10 +223,12 @@ public void testShortestPathStream() throws Exception { assertTrue(paths.contains("[jim, stan, mary, steve]")); + final boolean with_SOLR_15546_fix = false; + // SOLR-15546: fromNode and toNode contains colon stream = new ShortestPathStream(zkHost, "collection1", - random().nextBoolean() ? "https://aaa" : "\"https://aaa\"", + with_SOLR_15546_fix && random().nextBoolean() ? "https://aaa" : "\"https://aaa\"", "https://bbb", "from_s", "to_s", @@ -246,10 +248,11 @@ public void testShortestPathStream() throws Exception { assertTrue(paths.contains("[https://aaa, https://bbb]")); + if (with_SOLR_15546_fix) { // SOLR-15546: fromNode and toNode and interim node contains colon stream = new ShortestPathStream(zkHost, "collection1", - random().nextBoolean() ? "https://aaa" : "\"https://aaa\"", + with_SOLR_15546_fix && random().nextBoolean() ? "https://aaa" : "\"https://aaa\"", "https://ccc", "from_s", "to_s", @@ -268,6 +271,7 @@ public void testShortestPathStream() throws Exception { } assertTrue(paths.contains("[https://aaa, https://bbb, https://ccc]")); + } cache.close(); } From d72d82c846b3f6298a1b60fde0dda2690f53f45e Mon Sep 17 00:00:00 2001 From: Christine Poerschke Date: Fri, 30 Jul 2021 18:51:23 +0100 Subject: [PATCH 6/9] add test coverage (space or quotes in id) and reduce test coverage (surrounding quotes back compat) --- .../solr/client/solrj/io/graph/GraphTest.java | 57 +++++++++++++++++-- 1 file changed, 52 insertions(+), 5 deletions(-) diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java index a8eff13edb0..e0cd1c420c8 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java @@ -72,6 +72,11 @@ public void cleanIndex() throws Exception { // commented 15-Sep-2018 @LuceneTestCase.BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028") // 2-Aug-2018 public void testShortestPathStream() throws Exception { + final boolean with_SOLR_15546_fix = false; + + final String ORIGINAL_TITLE = random().nextBoolean() ? "Original 'P' paperback" : "Original \"H\" hardback"; + final String TRANSLATED_TITLE = "Translation"; + new UpdateRequest() .add(id, "0", "from_s", "jim", "to_s", "mike", "predicate_s", "knows") .add(id, "1", "from_s", "jim", "to_s", "dave", "predicate_s", "knows") @@ -88,8 +93,16 @@ public void testShortestPathStream() throws Exception { .add(id, "11", "from_s", "mary", "to_s", "max", "predicate_s", "knows") .add(id, "12", "from_s", "mary", "to_s", "jim", "predicate_s", "knows") .add(id, "13", "from_s", "mary", "to_s", "steve", "predicate_s", "knows") + // SOLR-15546: fromNode and toNode contains colon .add(id, "14", "from_s", "https://aaa", "to_s", "https://bbb", "predicate_s", "links") .add(id, "15", "from_s", "https://bbb", "to_s", "https://ccc", "predicate_s", "links") + // SOLR-15546: fromNode and toNode contains space + .add(id, "16", "from_s", "Author", "to_s", TRANSLATED_TITLE, "predicate_s", "author_to_book") + .add(id, "17", "from_s", TRANSLATED_TITLE, "to_s", "Translator", "predicate_s", "book_to_translator") + .add(id, "18", "from_s", "Ann Author", "to_s", TRANSLATED_TITLE, "predicate_s", "author_to_book") + .add(id, "19", "from_s", TRANSLATED_TITLE, "to_s", "Tim Translator", "predicate_s", "book_to_translator") + .add(id, "22", "from_s", "Ann Author", "to_s", ORIGINAL_TITLE, "predicate_s", "author_to_book") + .add(id, "23", "from_s", ORIGINAL_TITLE, "to_s", "Tim Translator", "predicate_s", "book_to_translator") .commit(cluster.getSolrClient(), COLLECTION); List tuples = null; @@ -223,12 +236,11 @@ public void testShortestPathStream() throws Exception { assertTrue(paths.contains("[jim, stan, mary, steve]")); - final boolean with_SOLR_15546_fix = false; - + if (with_SOLR_15546_fix) { // SOLR-15546: fromNode and toNode contains colon stream = new ShortestPathStream(zkHost, "collection1", - with_SOLR_15546_fix && random().nextBoolean() ? "https://aaa" : "\"https://aaa\"", + "https://aaa", "https://bbb", "from_s", "to_s", @@ -248,11 +260,10 @@ with_SOLR_15546_fix && random().nextBoolean() ? "https://aaa" : "\"https://aaa\" assertTrue(paths.contains("[https://aaa, https://bbb]")); - if (with_SOLR_15546_fix) { // SOLR-15546: fromNode and toNode and interim node contains colon stream = new ShortestPathStream(zkHost, "collection1", - with_SOLR_15546_fix && random().nextBoolean() ? "https://aaa" : "\"https://aaa\"", + "https://aaa", "https://ccc", "from_s", "to_s", @@ -273,6 +284,42 @@ with_SOLR_15546_fix && random().nextBoolean() ? "https://aaa" : "\"https://aaa\" assertTrue(paths.contains("[https://aaa, https://bbb, https://ccc]")); } + // SOLR-15546: fromNode and toNode contains colon + stream = new ShortestPathStream(zkHost, + "collection1", + "Ann Author", + "Tim Translator", + "from_s", + "to_s", + StreamingTest.mapParams(), + 10, + 3, + 6); + + stream.setStreamContext(context); + paths = new HashSet<>(); + tuples = getTuples(stream); + + for(Tuple tuple : tuples) { + paths.add(tuple.getStrings("path").toString()); + } + + if (with_SOLR_15546_fix) { + if (ORIGINAL_TITLE.contains("\"")) { + assertEquals(1, tuples.size()); + // double quotes in the interim ORIGINAL_TITLE node were not matched + assertTrue(paths.contains("[Ann Author, "+TRANSLATED_TITLE+", Tim Translator]")); + } else { + assertEquals(2, tuples.size()); + assertTrue(paths.contains("[Ann Author, "+ORIGINAL_TITLE+", Tim Translator]")); + assertTrue(paths.contains("[Ann Author, "+TRANSLATED_TITLE+", Tim Translator]")); + } + } else { + assertEquals(1, tuples.size()); + // "Ann Author" fromNode got interpreted as "Ann" OR "Author" + assertTrue(paths.contains("[Author, "+TRANSLATED_TITLE+", Tim Translator]")); + } + cache.close(); } From f18d80ebc2ad1f32e55fd7d9a2c4bb0d4a1dad2a Mon Sep 17 00:00:00 2001 From: Christine Poerschke Date: Fri, 30 Jul 2021 19:27:09 +0100 Subject: [PATCH 7/9] change code (more simply) --- .../apache/solr/client/solrj/io/graph/ShortestPathStream.java | 2 +- .../test/org/apache/solr/client/solrj/io/graph/GraphTest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java b/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java index f19b279c635..f62180ca63d 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java @@ -433,7 +433,7 @@ public List call() { StringBuffer nodeQuery = new StringBuffer(); for(String node : nodes) { - nodeQuery.append(node).append(" "); + nodeQuery.append('"').append(node).append('"').append(" "); } String q = fromField + ":(" + nodeQuery.toString().trim() + ")"; diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java index e0cd1c420c8..efaa943b61e 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java @@ -72,7 +72,7 @@ public void cleanIndex() throws Exception { // commented 15-Sep-2018 @LuceneTestCase.BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028") // 2-Aug-2018 public void testShortestPathStream() throws Exception { - final boolean with_SOLR_15546_fix = false; + final boolean with_SOLR_15546_fix = true; final String ORIGINAL_TITLE = random().nextBoolean() ? "Original 'P' paperback" : "Original \"H\" hardback"; final String TRANSLATED_TITLE = "Translation"; From 3440e35308873c03799beee36a1ad47934883f8b Mon Sep 17 00:00:00 2001 From: Christine Poerschke Date: Thu, 26 Aug 2021 20:38:29 +0100 Subject: [PATCH 8/9] add legacyJoin boolean flag --- .../solrj/io/graph/ShortestPathStream.java | 52 +++++++++++++++++-- .../solr/client/solrj/io/graph/GraphTest.java | 16 ++++-- 2 files changed, 60 insertions(+), 8 deletions(-) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java b/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java index f62180ca63d..2b195e34a7f 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java @@ -74,6 +74,33 @@ public class ShortestPathStream extends TupleStream implements Expressible { private int threads; private SolrParams queryParams; + private boolean legacyJoin = false; + + public ShortestPathStream(String zkHost, + String collection, + String fromNode, + String toNode, + String fromField, + String toField, + SolrParams queryParams, + int joinBatchSize, + int threads, + int maxDepth, + boolean legacyJoin) { + + init(zkHost, + collection, + fromNode, + toNode, + fromField, + toField, + queryParams, + joinBatchSize, + threads, + maxDepth, + legacyJoin); + } + public ShortestPathStream(String zkHost, String collection, String fromNode, @@ -94,7 +121,8 @@ public ShortestPathStream(String zkHost, queryParams, joinBatchSize, threads, - maxDepth); + maxDepth, + false /* legacyJoin */); } public ShortestPathStream(StreamExpression expression, StreamFactory factory) throws IOException { @@ -169,12 +197,21 @@ public ShortestPathStream(StreamExpression expression, StreamFactory factory) th maxDepth = Integer.parseInt(((StreamExpressionValue) depthExpression.getParameter()).getValue()); } + boolean legacyJoin = false; + + StreamExpressionNamedParameter legacyJoinExpression = factory.getNamedOperand(expression, "legacyJoin"); + + if(legacyJoinExpression != null) { + legacyJoin = Boolean.valueOf(((StreamExpressionValue)legacyJoinExpression.getParameter()).getValue()); + } + ModifiableSolrParams params = new ModifiableSolrParams(); for(StreamExpressionNamedParameter namedParam : namedParams){ if(!namedParam.getName().equals("zkHost") && !namedParam.getName().equals("to") && !namedParam.getName().equals("from") && !namedParam.getName().equals("edge") && + !namedParam.getName().equals("legacyJoin") && !namedParam.getName().equals("maxDepth") && !namedParam.getName().equals("threads") && !namedParam.getName().equals("partitionSize")) @@ -199,7 +236,7 @@ public ShortestPathStream(StreamExpression expression, StreamFactory factory) th } // We've got all the required items - init(zkHost, collectionName, fromNode, toNode, fromField, toField, params, partitionSize, threads, maxDepth); + init(zkHost, collectionName, fromNode, toNode, fromField, toField, params, partitionSize, threads, maxDepth, legacyJoin); } private void init(String zkHost, @@ -211,7 +248,8 @@ private void init(String zkHost, SolrParams queryParams, int joinBatchSize, int threads, - int maxDepth) { + int maxDepth, + boolean legacyJoin) { this.zkHost = zkHost; this.collection = collection; this.fromNode = fromNode; @@ -222,6 +260,7 @@ private void init(String zkHost, this.joinBatchSize = joinBatchSize; this.threads = threads; this.maxDepth = maxDepth; + this.legacyJoin = legacyJoin; } @Override @@ -252,6 +291,7 @@ public StreamExpressionParameter toExpression(StreamFactory factory) throws IOEx expression.addParameter(new StreamExpressionNamedParameter("from", fromNode)); expression.addParameter(new StreamExpressionNamedParameter("to", toNode)); expression.addParameter(new StreamExpressionNamedParameter("edge", fromField+"="+toField)); + expression.addParameter(new StreamExpressionNamedParameter("legacyJoin", Boolean.toString(legacyJoin))); return expression; } @@ -433,7 +473,11 @@ public List call() { StringBuffer nodeQuery = new StringBuffer(); for(String node : nodes) { - nodeQuery.append('"').append(node).append('"').append(" "); + if (legacyJoin) { + nodeQuery.append(node).append(" "); + } else { + nodeQuery.append('"').append(node).append('"').append(" "); + } } String q = fromField + ":(" + nodeQuery.toString().trim() + ")"; diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java index efaa943b61e..7b3a0e1695b 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java @@ -71,8 +71,15 @@ public void cleanIndex() throws Exception { @Test // commented 15-Sep-2018 @LuceneTestCase.BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028") // 2-Aug-2018 public void testShortestPathStream() throws Exception { + implTestShortestPathStream(false); + } - final boolean with_SOLR_15546_fix = true; + @Test + public void testLegacyShortestPathStream() throws Exception { + implTestShortestPathStream(true); + } + + private void implTestShortestPathStream(final boolean legacyJoin) throws Exception { final String ORIGINAL_TITLE = random().nextBoolean() ? "Original 'P' paperback" : "Original \"H\" hardback"; final String TRANSLATED_TITLE = "Translation"; @@ -236,7 +243,7 @@ public void testShortestPathStream() throws Exception { assertTrue(paths.contains("[jim, stan, mary, steve]")); - if (with_SOLR_15546_fix) { + if (!legacyJoin) { // SOLR-15546: fromNode and toNode contains colon stream = new ShortestPathStream(zkHost, "collection1", @@ -294,7 +301,8 @@ public void testShortestPathStream() throws Exception { StreamingTest.mapParams(), 10, 3, - 6); + 6, + legacyJoin); stream.setStreamContext(context); paths = new HashSet<>(); @@ -304,7 +312,7 @@ public void testShortestPathStream() throws Exception { paths.add(tuple.getStrings("path").toString()); } - if (with_SOLR_15546_fix) { + if (!legacyJoin) { if (ORIGINAL_TITLE.contains("\"")) { assertEquals(1, tuples.size()); // double quotes in the interim ORIGINAL_TITLE node were not matched From a0583774a442b8a9ac374fd95775bbc56a3ce487 Mon Sep 17 00:00:00 2001 From: Christine Poerschke Date: Tue, 31 Aug 2021 17:20:15 +0100 Subject: [PATCH 9/9] Apply suggestions from (self) code review --- .../apache/solr/client/solrj/io/graph/ShortestPathStream.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java b/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java index 2b195e34a7f..a9f6e771a4a 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java @@ -76,6 +76,7 @@ public class ShortestPathStream extends TupleStream implements Expressible { private boolean legacyJoin = false; + @Deprecated public ShortestPathStream(String zkHost, String collection, String fromNode, @@ -291,7 +292,7 @@ public StreamExpressionParameter toExpression(StreamFactory factory) throws IOEx expression.addParameter(new StreamExpressionNamedParameter("from", fromNode)); expression.addParameter(new StreamExpressionNamedParameter("to", toNode)); expression.addParameter(new StreamExpressionNamedParameter("edge", fromField+"="+toField)); - expression.addParameter(new StreamExpressionNamedParameter("legacyJoin", Boolean.toString(legacyJoin))); + if (legacyJoin) expression.addParameter(new StreamExpressionNamedParameter("legacyJoin", Boolean.toString(legacyJoin))); return expression; }