dsmiley commented on code in PR #2686:
URL: https://github.com/apache/lucene-solr/pull/2686#discussion_r1798739134
##########
solr/core/src/java/org/apache/solr/update/SolrIndexSplitter.java:
##########
@@ -130,9 +131,26 @@ public SolrIndexSplitter(SplitIndexCommand cmd) {
}
routeFieldName = cmd.routeFieldName;
if (routeFieldName == null) {
- field = searcher.getSchema().getUniqueKeyField();
+ // To support routing child documents, use the root field if it exists
(which would be populated with unique field),
+ // otherwise use the unique key field
+ field = searcher.getSchema().getFieldOrNull(IndexSchema.ROOT_FIELD_NAME);
+ if(field == null) {
+ field = searcher.getSchema().getUniqueKeyField();
+ }
} else {
- field = searcher.getSchema().getField(routeFieldName);
+ SchemaField uniqueField = searcher.getSchema().getUniqueKeyField();
+ if (uniqueField.getName().equals(routeFieldName)) {
+ // Explicitly routing based on unique field
+ // To support routing child documents, use the root field if it exists
(which would be populated with unique field),
+ // otherwise use the unique key field
+ field =
searcher.getSchema().getFieldOrNull(IndexSchema.ROOT_FIELD_NAME);
+ if (field == null) {
+ field = searcher.getSchema().getUniqueKeyField();
+ }
+ } else {
+ // Custom routing
+ field = searcher.getSchema().getField(routeFieldName);
+ }
Review Comment:
I don't think we should modify the logic for when a routerFieldName is
provided; should simply be unsupported. In my experience, it's rare to use
that. I'd prefer the code read simpler if you remove the support; you
duplicated code for it.
##########
solr/core/src/test/org/apache/solr/update/SolrIndexSplitterTest.java:
##########
@@ -364,7 +364,104 @@ private void
doTestSplitByRouteKey(SolrIndexSplitter.SplitMethod splitMethod) th
}
}
- private List<DocRouter.Range> getRanges(String id1, String id2) throws
UnsupportedEncodingException {
+ @Test
+ public void testSplitWithChildDocs() throws Exception {
+ doTestSplitWithChildDocs(SolrIndexSplitter.SplitMethod.REWRITE);
+ }
+
+ @Test
+ public void testSplitWithChildDocsLink() throws Exception {
+ doTestSplitWithChildDocs(SolrIndexSplitter.SplitMethod.LINK);
+ }
+
+ public void doTestSplitWithChildDocs(SolrIndexSplitter.SplitMethod
splitMethod) throws Exception {
+ // Overall test/split pattern copied from doTestSplitByRouteKey
Review Comment:
Okay; has an appearance of an old test but I can sympathize being consistent
with a similar test.
##########
solr/core/src/test/org/apache/solr/update/SolrIndexSplitterTest.java:
##########
@@ -364,7 +364,104 @@ private void
doTestSplitByRouteKey(SolrIndexSplitter.SplitMethod splitMethod) th
}
}
- private List<DocRouter.Range> getRanges(String id1, String id2) throws
UnsupportedEncodingException {
+ @Test
+ public void testSplitWithChildDocs() throws Exception {
+ doTestSplitWithChildDocs(SolrIndexSplitter.SplitMethod.REWRITE);
+ }
+
+ @Test
+ public void testSplitWithChildDocsLink() throws Exception {
+ doTestSplitWithChildDocs(SolrIndexSplitter.SplitMethod.LINK);
+ }
+
+ public void doTestSplitWithChildDocs(SolrIndexSplitter.SplitMethod
splitMethod) throws Exception {
+ // Overall test/split pattern copied from doTestSplitByRouteKey
+ File indexDir = createTempDir().toFile();
+
+ CompositeIdRouter r1 = new CompositeIdRouter();
+ String routeKeyBase = "sea-line!";
+
+ List<DocRouter.Range> twoRanges = r1.partitionRange(2,
r1.keyHashRange(routeKeyBase));
+
+ // Insert other doc for contrast
+ String otherDocId = routeKeyBase + "6"; // Will hash into first range
+ assertU(adoc("id", otherDocId));
+
+ // Insert child doc
+ String parentId = routeKeyBase + "1"; // Will hash into second range
+ String childId = routeKeyBase + "5"; // Will hash into first range
+ SolrInputDocument doc = sdoc("id", parentId, "myChild", sdocs(sdoc("id",
childId, "child_s", "child")));
+ assertU(adoc(doc));
+
+ assertU(commit());
+ assertJQ(req("q", "*:*"), "/response/numFound==3");
+
+ // Able to query child.
+ assertJQ(req("q", "id:"+parentId, "fl", "*, [child]"),
+ "/response/numFound==1",
+ "/response/docs/[0]/myChild/[0]/id=='" + childId+"'",
+ "/response/docs/[0]/myChild/[0]/_root_=='" + parentId + "'" // Child
has parent root to route with
+ );
+
+
+ SolrCore core1 = null, core2 = null;
+ try {
+ core1 = h.getCoreContainer().create("split1",
+ ImmutableMap.of("dataDir", indexDir1.getAbsolutePath(), "configSet",
"cloud-minimal"));
+ core2 = h.getCoreContainer().create("split2",
+ ImmutableMap.of("dataDir", indexDir2.getAbsolutePath(), "configSet",
"cloud-minimal"));
+
+ LocalSolrQueryRequest request = null;
+ try {
+ request = lrf.makeRequest("q", "dummy");
+ SolrQueryResponse rsp = new SolrQueryResponse();
+ SplitIndexCommand command = new SplitIndexCommand(request, rsp, null,
Lists.newArrayList(core1, core2), twoRanges,
+ new CompositeIdRouter(), null, null, splitMethod);
+ doSplit(command);
+ } finally {
+ if (request != null) request.close();
+ }
+ @SuppressWarnings("resource") final EmbeddedSolrServer server1 = new
EmbeddedSolrServer(h.getCoreContainer(), "split1");
+ @SuppressWarnings("resource") final EmbeddedSolrServer server2 = new
EmbeddedSolrServer(h.getCoreContainer(), "split2");
+ server1.commit(true, true);
+ server2.commit(true, true);
+ assertEquals("should be 2 docs in index2", 2, server2.query(new
SolrQuery("*:*")).getResults().getNumFound());
+ assertEquals("parent doc should be in index2", 1, server2.query(new
SolrQuery("id:"+ parentId)).getResults().getNumFound());
+ assertEquals("child doc should be in index2", 1, server2.query(new
SolrQuery("id:"+childId)).getResults().getNumFound());
+ assertEquals("other doc should be in index1", 1, server1.query(new
SolrQuery("id:" + otherDocId)).getResults().getNumFound());
+ } finally {
+ h.getCoreContainer().unload("split2");
+ h.getCoreContainer().unload("split1");
+ }
+ }
+
+ /**
+ * Utility method to find Ids that hash into which ranges
+ */
+ @Test
+ public void testCompositeHashSandbox() {
Review Comment:
doesn't actually test anything; just prints to the console. Shouldn't be
annotated to run as 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]
