From ea08bd3b67ff6b35d6264054d2131a87bbe9b870 Mon Sep 17 00:00:00 2001 From: Ishan Chattopadhyaya Date: Mon, 9 Apr 2018 16:36:07 +0530 Subject: [PATCH] SOLR-12096: Fixed inconsistent results format of subquery transformer for distributed search (multi-shard) --- SOLR-12096.patch | 217 ++++++++++++++++++ solr/CHANGES.txt | 3 + .../solr/response/GeoJSONResponseWriter.java | 3 +- .../solr/response/JSONResponseWriter.java | 6 +- .../apache/solr/response/JSONWriterTest.java | 24 +- .../TestSubQueryTransformerDistrib.java | 59 +++-- 6 files changed, 289 insertions(+), 23 deletions(-) create mode 100644 SOLR-12096.patch diff --git a/SOLR-12096.patch b/SOLR-12096.patch new file mode 100644 index 00000000000..9ed1ad73660 --- /dev/null +++ b/SOLR-12096.patch @@ -0,0 +1,217 @@ +diff --git a/solr/core/src/java/org/apache/solr/response/GeoJSONResponseWriter.java b/solr/core/src/java/org/apache/solr/response/GeoJSONResponseWriter.java +index 43fd7b4..012290e 100644 +--- a/solr/core/src/java/org/apache/solr/response/GeoJSONResponseWriter.java ++++ b/solr/core/src/java/org/apache/solr/response/GeoJSONResponseWriter.java +@@ -166,7 +166,8 @@ class GeoJSONWriter extends JSONWriter { + + // SolrDocument will now have multiValued fields represented as a Collection, + // even if only a single value is returned for this document. +- if (val instanceof List) { ++ // For SolrDocumentList, use writeVal instead of writeArray ++ if (!(val instanceof SolrDocumentList) && val instanceof List) { + // shortcut this common case instead of going through writeVal again + writeArray(name,((Iterable)val).iterator()); + } else { +diff --git a/solr/core/src/java/org/apache/solr/response/JSONResponseWriter.java b/solr/core/src/java/org/apache/solr/response/JSONResponseWriter.java +index 513df4e..5f6e2f2 100644 +--- a/solr/core/src/java/org/apache/solr/response/JSONResponseWriter.java ++++ b/solr/core/src/java/org/apache/solr/response/JSONResponseWriter.java +@@ -25,10 +25,11 @@ import java.util.Map; + import java.util.Set; + + import org.apache.solr.common.IteratorWriter; ++import org.apache.solr.common.MapWriter; + import org.apache.solr.common.MapWriter.EntryWriter; + import org.apache.solr.common.PushWriter; + import org.apache.solr.common.SolrDocument; +-import org.apache.solr.common.MapWriter; ++import org.apache.solr.common.SolrDocumentList; + import org.apache.solr.common.params.SolrParams; + import org.apache.solr.common.util.NamedList; + import org.apache.solr.common.util.SimpleOrderedMap; +@@ -367,7 +368,8 @@ class JSONWriter extends TextResponseWriter { + + // SolrDocument will now have multiValued fields represented as a Collection, + // even if only a single value is returned for this document. +- if (val instanceof List) { ++ // For SolrDocumentList, use writeVal instead of writeArray ++ if (!(val instanceof SolrDocumentList) && val instanceof List) { + // shortcut this common case instead of going through writeVal again + writeArray(name,((Iterable)val).iterator()); + } else { +diff --git a/solr/core/src/test/org/apache/solr/response/JSONWriterTest.java b/solr/core/src/test/org/apache/solr/response/JSONWriterTest.java +index 1b53150..68cebd2 100644 +--- a/solr/core/src/test/org/apache/solr/response/JSONWriterTest.java ++++ b/solr/core/src/test/org/apache/solr/response/JSONWriterTest.java +@@ -22,7 +22,10 @@ import java.lang.reflect.Method; + import java.lang.reflect.Modifier; + import java.nio.charset.StandardCharsets; + import java.util.ArrayList; ++import java.util.Arrays; + import java.util.List; ++ ++import org.apache.solr.JSONTestUtil; + import org.apache.solr.SolrTestCaseJ4; + import org.apache.solr.common.SolrDocument; + import org.apache.solr.common.SolrDocumentList; +@@ -130,9 +133,9 @@ public class JSONWriterTest extends SolrTestCaseJ4 { + } + + @Test +- public void testJSONSolrDocument() throws IOException { ++ public void testJSONSolrDocument() throws Exception { + SolrQueryRequest req = req(CommonParams.WT,"json", +- CommonParams.FL,"id,score"); ++ CommonParams.FL,"id,score,_children_,path"); + SolrQueryResponse rsp = new SolrQueryResponse(); + JSONResponseWriter w = new JSONResponseWriter(); + +@@ -141,11 +144,22 @@ public class JSONWriterTest extends SolrTestCaseJ4 { + + StringWriter buf = new StringWriter(); + ++ SolrDocument childDoc = new SolrDocument(); ++ childDoc.addField("id", "2"); ++ childDoc.addField("score", "0.4"); ++ childDoc.addField("path", Arrays.asList("a>b", "a>b>c")); ++ ++ SolrDocumentList childList = new SolrDocumentList(); ++ childList.setNumFound(1); ++ childList.setStart(0); ++ childList.add(childDoc); ++ + SolrDocument solrDoc = new SolrDocument(); + solrDoc.addField("id", "1"); + solrDoc.addField("subject", "hello2"); + solrDoc.addField("title", "hello3"); + solrDoc.addField("score", "0.7"); ++ solrDoc.setField("_children_", childList); + + SolrDocumentList list = new SolrDocumentList(); + list.setNumFound(1); +@@ -163,8 +177,12 @@ public class JSONWriterTest extends SolrTestCaseJ4 { + result.contains("\"title\"")); + assertTrue("response doesn't contain expected fields: " + result, + result.contains("\"id\"") && +- result.contains("\"score\"")); ++ result.contains("\"score\"") && result.contains("_children_")); + ++ String expectedResult = "{'response':{'numFound':1,'start':0,'maxScore':0.7,'docs':[{'id':'1', 'score':'0.7'," + ++ " '_children_':{'numFound':1,'start':0,'docs':[{'id':'2', 'score':'0.4', 'path':['a>b', 'a>b>c']}] }}] }}"; ++ String error = JSONTestUtil.match(result, "=="+expectedResult); ++ assertNull("response validation failed with error: " + error, error); + + req.close(); + } +diff --git a/solr/core/src/test/org/apache/solr/response/transform/TestSubQueryTransformerDistrib.java b/solr/core/src/test/org/apache/solr/response/transform/TestSubQueryTransformerDistrib.java +index 620cac0..f6d0a38 100644 +--- a/solr/core/src/test/org/apache/solr/response/transform/TestSubQueryTransformerDistrib.java ++++ b/solr/core/src/test/org/apache/solr/response/transform/TestSubQueryTransformerDistrib.java +@@ -16,7 +16,11 @@ + */ + package org.apache.solr.response.transform; + ++import java.io.ByteArrayOutputStream; + import java.io.IOException; ++import java.io.InputStream; ++import java.net.URL; ++import java.nio.charset.Charset; + import java.nio.file.Path; + import java.nio.file.Paths; + import java.util.ArrayList; +@@ -26,6 +30,8 @@ import java.util.List; + import java.util.Map; + import java.util.Random; + ++import org.apache.commons.io.IOUtils; ++import org.apache.solr.JSONTestUtil; + import org.apache.solr.client.solrj.SolrServerException; + import org.apache.solr.client.solrj.impl.CloudSolrClient; + import org.apache.solr.client.solrj.request.CollectionAdminRequest; +@@ -37,10 +43,12 @@ import org.apache.solr.cloud.SolrCloudTestCase; + import org.apache.solr.common.SolrDocument; + import org.apache.solr.common.SolrDocumentList; + import org.apache.solr.common.cloud.ZkStateReader; ++import org.apache.solr.common.params.ModifiableSolrParams; + import org.apache.solr.common.util.ContentStreamBase; + import org.junit.BeforeClass; + import org.junit.Test; + ++@org.apache.solr.SolrTestCaseJ4.SuppressSSL() + public class TestSubQueryTransformerDistrib extends SolrCloudTestCase { + + private static final String support = "These guys help customers"; +@@ -92,7 +100,7 @@ public class TestSubQueryTransformerDistrib extends SolrCloudTestCase { + + @SuppressWarnings("serial") + @Test +- public void test() throws SolrServerException, IOException { ++ public void test() throws Exception { + int peopleMultiplier = atLeast(1); + int deptMultiplier = atLeast(1); + +@@ -100,24 +108,26 @@ public class TestSubQueryTransformerDistrib extends SolrCloudTestCase { + + Random random1 = random(); + ++ final ModifiableSolrParams params = params( ++ new String[]{"q","name_s:dave", "indent","true", ++ "fl","*,depts:[subquery "+((random1.nextBoolean() ? "" : "separator=,"))+"]", ++ "rows","" + peopleMultiplier, ++ "depts.q","{!terms f=dept_id_s v=$row.dept_ss_dv "+((random1.nextBoolean() ? "" : "separator=,"))+"}", ++ "depts.fl","text_t"+(differentUniqueId?",id:notid":""), ++ "depts.indent","true", ++ "depts.collection","departments", ++ differentUniqueId ? "depts.distrib.singlePass":"notnecessary","true", ++ "depts.rows",""+(deptMultiplier*2), ++ "depts.logParamsList","q,fl,rows,row.dept_ss_dv", ++ random().nextBoolean()?"depts.wt":"whatever",anyWt(), ++ random().nextBoolean()?"wt":"whatever",anyWt()}); ++ ++ final SolrDocumentList hits; + { +- +- final QueryRequest qr = new QueryRequest(params( +- new String[]{"q","name_s:dave", "indent","true", +- "fl","*,depts:[subquery "+((random1.nextBoolean() ? "" : "separator=,"))+"]", +- "rows","" + peopleMultiplier, +- "depts.q","{!terms f=dept_id_s v=$row.dept_ss_dv "+((random1.nextBoolean() ? "" : "separator=,"))+"}", +- "depts.fl","text_t"+(differentUniqueId?",id:notid":""), +- "depts.indent","true", +- "depts.collection","departments", +- differentUniqueId ? "depts.distrib.singlePass":"notnecessary","true", +- "depts.rows",""+(deptMultiplier*2), +- "depts.logParamsList","q,fl,rows,row.dept_ss_dv", +- random().nextBoolean()?"depts.wt":"whatever",anyWt(), +- random().nextBoolean()?"wt":"whatever",anyWt()})); ++ final QueryRequest qr = new QueryRequest(params); + final QueryResponse rsp = new QueryResponse(); +- rsp.setResponse(cluster.getSolrClient().request(qr, people)); +- final SolrDocumentList hits = rsp.getResults(); ++ rsp.setResponse(cluster.getSolrClient().request(qr, people+","+depts)); ++ hits = rsp.getResults(); + + assertEquals(peopleMultiplier, hits.getNumFound()); + +@@ -140,6 +150,21 @@ public class TestSubQueryTransformerDistrib extends SolrCloudTestCase { + } + assertEquals(hits.toString(), engineerCount, supportCount); + } ++ ++ params.set("wt", "json"); ++ final URL node = new URL(cluster.getRandomJetty(random()).getBaseUrl().toString() ++ +"/"+people+"/select"+params.toQueryString()); ++ ++ try(final InputStream jsonResponse = node.openStream()){ ++ final ByteArrayOutputStream outBuffer = new ByteArrayOutputStream(); ++ IOUtils.copy(jsonResponse, outBuffer); ++ ++ final Object expected = ((SolrDocumentList) hits.get(0).getFieldValue("depts")).get(0).get("text_t"); ++ final String err = JSONTestUtil.match("/response/docs/[0]/depts/docs/[0]/text_t" ++ ,outBuffer.toString(Charset.forName("UTF-8").toString()), ++ "\""+expected+"\""); ++ assertNull(err,err); ++ } + + } + diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index f9102244704..9b9905521de 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -114,6 +114,9 @@ Bug Fixes * SOLR-12199: TestReplicationHandler.doTestRepeater(): TEST_PORT interpolation failure: Server refused connection at: http://127.0.0.1:TEST_PORT/solr (Mikhail Khludnev, Dawid Weiss, Steve Rowe) +* SOLR-12096: Fixed inconsistent results format of subquery transformer for distributed search (multi-shard). + (Munendra S N, Mikhail Khludnev via Ishan Chattopadhyaya) + Optimizations ---------------------- diff --git a/solr/core/src/java/org/apache/solr/response/GeoJSONResponseWriter.java b/solr/core/src/java/org/apache/solr/response/GeoJSONResponseWriter.java index 43fd7b48ed6..012290eb5df 100644 --- a/solr/core/src/java/org/apache/solr/response/GeoJSONResponseWriter.java +++ b/solr/core/src/java/org/apache/solr/response/GeoJSONResponseWriter.java @@ -166,7 +166,8 @@ class GeoJSONWriter extends JSONWriter { // SolrDocument will now have multiValued fields represented as a Collection, // even if only a single value is returned for this document. - if (val instanceof List) { + // For SolrDocumentList, use writeVal instead of writeArray + if (!(val instanceof SolrDocumentList) && val instanceof List) { // shortcut this common case instead of going through writeVal again writeArray(name,((Iterable)val).iterator()); } else { diff --git a/solr/core/src/java/org/apache/solr/response/JSONResponseWriter.java b/solr/core/src/java/org/apache/solr/response/JSONResponseWriter.java index 513df4eed53..5f6e2f27bc4 100644 --- a/solr/core/src/java/org/apache/solr/response/JSONResponseWriter.java +++ b/solr/core/src/java/org/apache/solr/response/JSONResponseWriter.java @@ -25,10 +25,11 @@ import java.util.Map; import java.util.Set; import org.apache.solr.common.IteratorWriter; +import org.apache.solr.common.MapWriter; import org.apache.solr.common.MapWriter.EntryWriter; import org.apache.solr.common.PushWriter; import org.apache.solr.common.SolrDocument; -import org.apache.solr.common.MapWriter; +import org.apache.solr.common.SolrDocumentList; import org.apache.solr.common.params.SolrParams; import org.apache.solr.common.util.NamedList; import org.apache.solr.common.util.SimpleOrderedMap; @@ -367,7 +368,8 @@ class JSONWriter extends TextResponseWriter { // SolrDocument will now have multiValued fields represented as a Collection, // even if only a single value is returned for this document. - if (val instanceof List) { + // For SolrDocumentList, use writeVal instead of writeArray + if (!(val instanceof SolrDocumentList) && val instanceof List) { // shortcut this common case instead of going through writeVal again writeArray(name,((Iterable)val).iterator()); } else { diff --git a/solr/core/src/test/org/apache/solr/response/JSONWriterTest.java b/solr/core/src/test/org/apache/solr/response/JSONWriterTest.java index 1b531505d0f..68cebd24416 100644 --- a/solr/core/src/test/org/apache/solr/response/JSONWriterTest.java +++ b/solr/core/src/test/org/apache/solr/response/JSONWriterTest.java @@ -22,7 +22,10 @@ import java.lang.reflect.Method; import java.lang.reflect.Modifier; import java.nio.charset.StandardCharsets; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; + +import org.apache.solr.JSONTestUtil; import org.apache.solr.SolrTestCaseJ4; import org.apache.solr.common.SolrDocument; import org.apache.solr.common.SolrDocumentList; @@ -130,9 +133,9 @@ public class JSONWriterTest extends SolrTestCaseJ4 { } @Test - public void testJSONSolrDocument() throws IOException { + public void testJSONSolrDocument() throws Exception { SolrQueryRequest req = req(CommonParams.WT,"json", - CommonParams.FL,"id,score"); + CommonParams.FL,"id,score,_children_,path"); SolrQueryResponse rsp = new SolrQueryResponse(); JSONResponseWriter w = new JSONResponseWriter(); @@ -141,11 +144,22 @@ public class JSONWriterTest extends SolrTestCaseJ4 { StringWriter buf = new StringWriter(); + SolrDocument childDoc = new SolrDocument(); + childDoc.addField("id", "2"); + childDoc.addField("score", "0.4"); + childDoc.addField("path", Arrays.asList("a>b", "a>b>c")); + + SolrDocumentList childList = new SolrDocumentList(); + childList.setNumFound(1); + childList.setStart(0); + childList.add(childDoc); + SolrDocument solrDoc = new SolrDocument(); solrDoc.addField("id", "1"); solrDoc.addField("subject", "hello2"); solrDoc.addField("title", "hello3"); solrDoc.addField("score", "0.7"); + solrDoc.setField("_children_", childList); SolrDocumentList list = new SolrDocumentList(); list.setNumFound(1); @@ -163,8 +177,12 @@ public class JSONWriterTest extends SolrTestCaseJ4 { result.contains("\"title\"")); assertTrue("response doesn't contain expected fields: " + result, result.contains("\"id\"") && - result.contains("\"score\"")); + result.contains("\"score\"") && result.contains("_children_")); + String expectedResult = "{'response':{'numFound':1,'start':0,'maxScore':0.7,'docs':[{'id':'1', 'score':'0.7'," + + " '_children_':{'numFound':1,'start':0,'docs':[{'id':'2', 'score':'0.4', 'path':['a>b', 'a>b>c']}] }}] }}"; + String error = JSONTestUtil.match(result, "=="+expectedResult); + assertNull("response validation failed with error: " + error, error); req.close(); } diff --git a/solr/core/src/test/org/apache/solr/response/transform/TestSubQueryTransformerDistrib.java b/solr/core/src/test/org/apache/solr/response/transform/TestSubQueryTransformerDistrib.java index 620cac0a942..f6d0a383fc6 100644 --- a/solr/core/src/test/org/apache/solr/response/transform/TestSubQueryTransformerDistrib.java +++ b/solr/core/src/test/org/apache/solr/response/transform/TestSubQueryTransformerDistrib.java @@ -16,7 +16,11 @@ */ package org.apache.solr.response.transform; +import java.io.ByteArrayOutputStream; import java.io.IOException; +import java.io.InputStream; +import java.net.URL; +import java.nio.charset.Charset; import java.nio.file.Path; import java.nio.file.Paths; import java.util.ArrayList; @@ -26,6 +30,8 @@ import java.util.List; import java.util.Map; import java.util.Random; +import org.apache.commons.io.IOUtils; +import org.apache.solr.JSONTestUtil; import org.apache.solr.client.solrj.SolrServerException; import org.apache.solr.client.solrj.impl.CloudSolrClient; import org.apache.solr.client.solrj.request.CollectionAdminRequest; @@ -37,10 +43,12 @@ import org.apache.solr.cloud.SolrCloudTestCase; import org.apache.solr.common.SolrDocument; import org.apache.solr.common.SolrDocumentList; import org.apache.solr.common.cloud.ZkStateReader; +import org.apache.solr.common.params.ModifiableSolrParams; import org.apache.solr.common.util.ContentStreamBase; import org.junit.BeforeClass; import org.junit.Test; +@org.apache.solr.SolrTestCaseJ4.SuppressSSL() public class TestSubQueryTransformerDistrib extends SolrCloudTestCase { private static final String support = "These guys help customers"; @@ -92,7 +100,7 @@ public class TestSubQueryTransformerDistrib extends SolrCloudTestCase { @SuppressWarnings("serial") @Test - public void test() throws SolrServerException, IOException { + public void test() throws Exception { int peopleMultiplier = atLeast(1); int deptMultiplier = atLeast(1); @@ -100,24 +108,26 @@ public class TestSubQueryTransformerDistrib extends SolrCloudTestCase { Random random1 = random(); + final ModifiableSolrParams params = params( + new String[]{"q","name_s:dave", "indent","true", + "fl","*,depts:[subquery "+((random1.nextBoolean() ? "" : "separator=,"))+"]", + "rows","" + peopleMultiplier, + "depts.q","{!terms f=dept_id_s v=$row.dept_ss_dv "+((random1.nextBoolean() ? "" : "separator=,"))+"}", + "depts.fl","text_t"+(differentUniqueId?",id:notid":""), + "depts.indent","true", + "depts.collection","departments", + differentUniqueId ? "depts.distrib.singlePass":"notnecessary","true", + "depts.rows",""+(deptMultiplier*2), + "depts.logParamsList","q,fl,rows,row.dept_ss_dv", + random().nextBoolean()?"depts.wt":"whatever",anyWt(), + random().nextBoolean()?"wt":"whatever",anyWt()}); + + final SolrDocumentList hits; { - - final QueryRequest qr = new QueryRequest(params( - new String[]{"q","name_s:dave", "indent","true", - "fl","*,depts:[subquery "+((random1.nextBoolean() ? "" : "separator=,"))+"]", - "rows","" + peopleMultiplier, - "depts.q","{!terms f=dept_id_s v=$row.dept_ss_dv "+((random1.nextBoolean() ? "" : "separator=,"))+"}", - "depts.fl","text_t"+(differentUniqueId?",id:notid":""), - "depts.indent","true", - "depts.collection","departments", - differentUniqueId ? "depts.distrib.singlePass":"notnecessary","true", - "depts.rows",""+(deptMultiplier*2), - "depts.logParamsList","q,fl,rows,row.dept_ss_dv", - random().nextBoolean()?"depts.wt":"whatever",anyWt(), - random().nextBoolean()?"wt":"whatever",anyWt()})); + final QueryRequest qr = new QueryRequest(params); final QueryResponse rsp = new QueryResponse(); - rsp.setResponse(cluster.getSolrClient().request(qr, people)); - final SolrDocumentList hits = rsp.getResults(); + rsp.setResponse(cluster.getSolrClient().request(qr, people+","+depts)); + hits = rsp.getResults(); assertEquals(peopleMultiplier, hits.getNumFound()); @@ -140,6 +150,21 @@ public class TestSubQueryTransformerDistrib extends SolrCloudTestCase { } assertEquals(hits.toString(), engineerCount, supportCount); } + + params.set("wt", "json"); + final URL node = new URL(cluster.getRandomJetty(random()).getBaseUrl().toString() + +"/"+people+"/select"+params.toQueryString()); + + try(final InputStream jsonResponse = node.openStream()){ + final ByteArrayOutputStream outBuffer = new ByteArrayOutputStream(); + IOUtils.copy(jsonResponse, outBuffer); + + final Object expected = ((SolrDocumentList) hits.get(0).getFieldValue("depts")).get(0).get("text_t"); + final String err = JSONTestUtil.match("/response/docs/[0]/depts/docs/[0]/text_t" + ,outBuffer.toString(Charset.forName("UTF-8").toString()), + "\""+expected+"\""); + assertNull(err,err); + } }