Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
janhoy commented on PR #4279: URL: https://github.com/apache/solr/pull/4279#issuecomment-4365867397 > I don't see automation to take straggler/latent unreleased changelog yamls and get them added to an existing generated changelog (e.g. from the first RC). Have you thought of this? Agree there is a gap here. Feels a bit heavy weight to do all the changelog release and generate steps to bring in one extra entry. I think arms have been doing some manual surgical operations not codified in wizard for this. Perhaps we should have some shell/py script that «amends» the changelog in RCs? -- 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-18194: fix nested docs detection false positive [solr]
rahulgoswami commented on PR #4279: URL: https://github.com/apache/solr/pull/4279#issuecomment-4363018852 >@rahulgoswami I have pushed the change to the 9_11 branch. Thanks @kotman12 . I hope you are also planning on cherrypicking this to branch_9x and branch_10x (?) -- 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-18194: fix nested docs detection false positive [solr]
dsmiley commented on PR #4279: URL: https://github.com/apache/solr/pull/4279#issuecomment-4356261168 Changelog management is only for release managers, and yes using scripts. I am deferring that step in my process until the RC to avoid friction in addressing latent entries. So basically you needn't worry about it: if this PR warranted a changelog then it goes where all new changelog entries go -- to unreleased. RM's "release" them. I've only been the RM once before but I'd expect this to be prior to each RC as the changelog is an artifact of the release. @janhoy I don't see automation to take straggler/latent unreleased changelog yamls and get them added to an existing generated changelog (e.g. from the first RC). Have you thought of this? -- 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-18194: fix nested docs detection false positive [solr]
kotman12 commented on PR #4279: URL: https://github.com/apache/solr/pull/4279#issuecomment-4355447567 > I commented over there to ditch the changelog for improvements to unreleased things. But otherwise -- sure; I assume it's what you/Rahul worked out. > > Process for any backporting (release branch or not): > > * git cherry-pick from main (or another I guess) > > * Fix conflicts > * `./gradlew check` , maybe exclude tests depending on your confidence level. > > * Fix issues. If the changes are extensive enough to warrant peer review, switch to a PR. > * git push > > Note that there's a [cherrypick.sh](https://github.com/apache/solr/blob/ec51d13bc14821ac13041c7461abe66fc12b5483/dev-tools/scripts/cherrypick.sh) script that works well as long as there aren't git conflicts. @dsmiley Good call about the changelog. I removed it. Even if we were to include it, should it have been in the unreleased folder? Is there some release management script that puts in the right place? I assume you want in some 9_11 folder at the end. Btw @rahulgoswami I have pushed the change to the 9_11 branch. -- 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-18194: fix nested docs detection false positive [solr]
dsmiley commented on PR #4279: URL: https://github.com/apache/solr/pull/4279#issuecomment-4349560368 I commented over there to ditch the changelog for improvements to unreleased things. But otherwise -- sure; I assume it's what you/Rahul worked out. Process for any backporting (release branch or not): * git cherry-pick from main (or another I guess) * Fix conflicts * `./gradlew check` , maybe exclude tests depending on your confidence level. * Fix issues. If the changes are extensive enough to warrant peer review, switch to a PR. * git push Note that there's a [cherrypick.sh](https://github.com/apache/solr/blob/ec51d13bc14821ac13041c7461abe66fc12b5483/dev-tools/scripts/cherrypick.sh) script that works well as long as there aren't git conflicts. -- 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-18194: fix nested docs detection false positive [solr]
kotman12 commented on PR #4279: URL: https://github.com/apache/solr/pull/4279#issuecomment-4347234396 > +1 to merge to branch_9_11 @dsmiley does [this](https://github.com/kotman12/solr/commit/af1b44a10d6806682cd677c6b7544effa099edef) look good? I gather we push directly to the release branch and given that I've never done this before I wanted a quick sanity check if you don't mind. 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-18194: fix nested docs detection false positive [solr]
dsmiley commented on PR #4279: URL: https://github.com/apache/solr/pull/4279#issuecomment-4339173522 +1 to merge to branch_9_11 -- 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-18194: fix nested docs detection false positive [solr]
kotman12 commented on PR #4279: URL: https://github.com/apache/solr/pull/4279#issuecomment-4330862852 > Thanks for the extra effort of modifying the test cases. This looks good to me. Please go ahead and merge whenever you can :) . I'd also recommend cherry picking to branch_10x and branch_9x please. @dsmiley is going to cut a 9.11 branch any time soon, so would be convenient to have this cherrypicked to branch_9x before that. I am also happy to help out and do this in case you are unable to get to this soon-ish(?). Just let me know. Thanks again for making this better! > Thanks for the extra effort of modifying the test cases. This looks good to me. Please go ahead and merge whenever you can :) . I'd also recommend cherry picking to branch_10x and branch_9x please. @dsmiley is going to cut a 9.11 branch any time soon, so would be convenient to have this cherrypicked to branch_9x before that. I am also happy to help out and do this in case you are unable to get to this soon-ish(?). Just let me know. Thanks again for making this better! Sure thing, thanks for taking a look! I will try to get to this tomorrow as I've been busy this past 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-18194: fix nested docs detection false positive [solr]
kotman12 merged PR #4279: URL: https://github.com/apache/solr/pull/4279 -- 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-18194: fix nested docs detection false positive [solr]
rahulgoswami commented on code in PR #4279:
URL: https://github.com/apache/solr/pull/4279#discussion_r3142995900
##
solr/core/src/test/org/apache/solr/handler/admin/UpgradeCoreIndexActionTest.java:
##
@@ -365,13 +365,108 @@ public void
testUpgradeCoreIndexFailsWithNestedDocuments() throws Exception {
coreName),
resp));
- // Verify the exception message indicates nested documents are not
supported
+ // Verify the exception message indicates child documents are not
supported
assertThat(
thrown.getMessage(),
- containsString("does not support indexes containing nested
documents"));
+ containsString("does not support indexes containing child/nested
documents"));
} finally {
admin.shutdown();
admin.close();
}
}
+
+ @Test
+ public void testChildDocsDetection_noChildDocs() throws Exception {
+addDocsWithRandomUpdatesAndDeletes();
+
+final String coreName = h.getCore().getName();
+CoreAdminHandler admin = new CoreAdminHandler(h.getCoreContainer());
+try {
+ final SolrQueryResponse resp = new SolrQueryResponse();
+ admin.handleRequestBody(
+ req(
+ CoreAdminParams.ACTION,
+ CoreAdminParams.CoreAdminAction.UPGRADECOREINDEX.toString(),
+ CoreAdminParams.CORE,
+ coreName),
+ resp);
+ assertNull("Unexpected exception: " + resp.getException(),
resp.getException());
+} finally {
+ admin.shutdown();
+ admin.close();
+}
+ }
+
+ @Test
+ public void testChildDocsDetection_withChildDocs() throws Exception {
+addChildDoc("100", "101");
+addDocsWithRandomUpdatesAndDeletes();
+
+final String coreName = h.getCore().getName();
+CoreAdminHandler admin = new CoreAdminHandler(h.getCoreContainer());
+try {
+ final SolrQueryResponse resp = new SolrQueryResponse();
+ SolrException thrown =
+ assertThrows(
+ SolrException.class,
+ () ->
+ admin.handleRequestBody(
+ req(
+ CoreAdminParams.ACTION,
+
CoreAdminParams.CoreAdminAction.UPGRADECOREINDEX.toString(),
+ CoreAdminParams.CORE,
+ coreName),
+ resp));
+ assertThat(
+ thrown.getMessage(),
+ containsString("does not support indexes containing child/nested
documents"));
+} finally {
+ admin.shutdown();
+ admin.close();
+}
+ }
+
+ /**
+ * Add non-child docs with a random number of within-commit updates and
deletes. This exercises
+ * the false-positive scenario for child doc detection: updates and deletes
leave behind deleted
+ * entries in the same segment, causing multiple docs to share the same
{@code _root_} value.
+ *
+ * With NoMergePolicy and a 100MB RAM buffer (from SolrIndexConfig
defaults), no flush or merge
+ * occurs mid-batch, guaranteeing co-location in a single segment.
+ */
+ private void addDocsWithRandomUpdatesAndDeletes() {
+int numDocs = 10;
+for (int i = 0; i < numDocs; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "doc" + i));
+}
+int numUpdates = random().nextInt(4);
+for (int i = 0; i < numUpdates; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "updated_doc" + i));
+}
+int numDeletes = random().nextInt(4);
+for (int i = 0; i < numDeletes; i++) {
+ assertU(delI(String.valueOf(numDocs - 1 - i)));
Review Comment:
I like that you took care to not overstep on the updates and have the delete
chunk as a separate set of ids.
--
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-18194: fix nested docs detection false positive [solr]
rahulgoswami commented on PR #4279: URL: https://github.com/apache/solr/pull/4279#issuecomment-4321274771 Thanks for the extra effort of modifying the test cases. This looks good to me. Please go ahead and merge whenever you can :) . I'd also recommend cherry picking to branch_10x and branch_9x please. @dsmiley is going to cut a 9.11 branch any time soon, so would be convenient to have this cherrypicked to branch_9x before that. I am also happy to help out and do this in case you are unable to get to this soon-ish(?). Just let me know. Thanks again for making this better! -- 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-18194: fix nested docs detection false positive [solr]
rahulgoswami commented on code in PR #4279:
URL: https://github.com/apache/solr/pull/4279#discussion_r3142985602
##
solr/core/src/test/org/apache/solr/handler/admin/UpgradeCoreIndexActionTest.java:
##
@@ -365,10 +365,192 @@ public void
testUpgradeCoreIndexFailsWithNestedDocuments() throws Exception {
coreName),
resp));
- // Verify the exception message indicates nested documents are not
supported
+ // Verify the exception message indicates child documents are not
supported
assertThat(
thrown.getMessage(),
- containsString("does not support indexes containing nested
documents"));
+ containsString("does not support indexes containing child
documents"));
+} finally {
+ admin.shutdown();
+ admin.close();
+}
+ }
+
+ // --- Child docs detection tests ---
+ //
+ // These tests verify that the child document detection in the upgrade path
+ // correctly distinguishes between genuine child docs and non-child docs,
+ // even in the presence of updates and deletes that leave deleted documents
+ // in segments (since NoMergePolicy prevents segment merges from purging
them).
+
+ @Test
+ public void testChildDocsDetection_noChildDocsJustAdd() throws Exception {
+for (int i = 0; i < 10; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "doc" + i));
+}
+assertU(commit("openSearcher", "true"));
+
+assertUpgradeDoesNotDetectChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_withChildDocsJustAdd() throws Exception {
+addChildDoc("100", "101");
+addChildDoc("200", "201");
+assertU(commit("openSearcher", "true"));
+
+assertUpgradeDetectsChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_noChildDocsWithWithinCommitUpdates()
throws Exception {
+// Add docs and then update some of them BEFORE committing, so both the old
+// (deleted) and new versions end up in the same flushed segment.
+// With NoMergePolicy and a 100MB RAM buffer (from SolrIndexConfig
defaults),
+// no flush or merge occurs mid-batch, guaranteeing co-location.
+//
+// In the resulting segment, _root_ Terms stats will show:
+// Terms.size() = N (unique _root_ values, one per unique id)
+// Terms.getDocCount() = N + updates (includes deleted doc entries)
+//
+// A naive check (uniqueRootValues < docsWithRoot) may false-positive here
+// because multiple docs share the same _root_ value within the segment.
+for (int i = 0; i < 10; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "doc" + i));
+}
+// Re-add a few docs with the same ids (within-commit updates)
+for (int i = 0; i < 3; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "updated_doc" + i));
+}
+assertU(commit("openSearcher", "true"));
+
+// 10 live docs — the updates replaced 3 docs in-place
+assertQ(req("q", "*:*"), "//result[@numFound='10']");
+assertUpgradeDoesNotDetectChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_withChildDocsWithWithinCommitUpdates()
throws Exception {
Review Comment:
Thanks for systematically explaining each case.
I do understand the update/delete scenarios within the same commit, and your
fix. My question was mainly to understand if there is another case/possible
future refactor that you're thinking of when you said "I think it's possible to
imagine some future code path that only gets exercised when ```unique(id) ==
idTerms.size() ``` (which is the case only if all deletes are via update) or
vice versa."
We are good here 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-18194: fix nested docs detection false positive [solr]
kotman12 commented on code in PR #4279:
URL: https://github.com/apache/solr/pull/4279#discussion_r3110855110
##
solr/core/src/test/org/apache/solr/handler/admin/UpgradeCoreIndexActionTest.java:
##
@@ -365,10 +365,192 @@ public void
testUpgradeCoreIndexFailsWithNestedDocuments() throws Exception {
coreName),
resp));
- // Verify the exception message indicates nested documents are not
supported
+ // Verify the exception message indicates child documents are not
supported
assertThat(
thrown.getMessage(),
- containsString("does not support indexes containing nested
documents"));
+ containsString("does not support indexes containing child
documents"));
+} finally {
+ admin.shutdown();
+ admin.close();
+}
+ }
+
+ // --- Child docs detection tests ---
+ //
+ // These tests verify that the child document detection in the upgrade path
+ // correctly distinguishes between genuine child docs and non-child docs,
+ // even in the presence of updates and deletes that leave deleted documents
+ // in segments (since NoMergePolicy prevents segment merges from purging
them).
+
+ @Test
+ public void testChildDocsDetection_noChildDocsJustAdd() throws Exception {
+for (int i = 0; i < 10; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "doc" + i));
+}
+assertU(commit("openSearcher", "true"));
+
+assertUpgradeDoesNotDetectChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_withChildDocsJustAdd() throws Exception {
+addChildDoc("100", "101");
+addChildDoc("200", "201");
+assertU(commit("openSearcher", "true"));
+
+assertUpgradeDetectsChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_noChildDocsWithWithinCommitUpdates()
throws Exception {
+// Add docs and then update some of them BEFORE committing, so both the old
+// (deleted) and new versions end up in the same flushed segment.
+// With NoMergePolicy and a 100MB RAM buffer (from SolrIndexConfig
defaults),
+// no flush or merge occurs mid-batch, guaranteeing co-location.
+//
+// In the resulting segment, _root_ Terms stats will show:
+// Terms.size() = N (unique _root_ values, one per unique id)
+// Terms.getDocCount() = N + updates (includes deleted doc entries)
+//
+// A naive check (uniqueRootValues < docsWithRoot) may false-positive here
+// because multiple docs share the same _root_ value within the segment.
+for (int i = 0; i < 10; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "doc" + i));
+}
+// Re-add a few docs with the same ids (within-commit updates)
+for (int i = 0; i < 3; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "updated_doc" + i));
+}
+assertU(commit("openSearcher", "true"));
+
+// 10 live docs — the updates replaced 3 docs in-place
+assertQ(req("q", "*:*"), "//result[@numFound='10']");
+assertUpgradeDoesNotDetectChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_withChildDocsWithWithinCommitUpdates()
throws Exception {
Review Comment:
Consider three statistics that are similar but subtly different for
"id-like" fields: `terms.size`, `terms.docCount`, `unique(id)` (i.e. facet via
searcher). When all your deletions are via updates, meaning every deletion of a
solr Id is paired with an addition of that same solr Id then you have:
`terms.docCount > terms.size = unique(id)`
However, when you have deletions where none of the deletions are part of
updates then you have:
`terms.docCount = terms.size > unique(id)`
I want to add tests which protect against the wrong kind of refactor which
IMO is not that hard considering the subtelties of the different types of
counts. I know they confused me a bit until I considered all the cases
mentioned above.
--
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-18194: fix nested docs detection false positive [solr]
kotman12 commented on code in PR #4279:
URL: https://github.com/apache/solr/pull/4279#discussion_r3110855110
##
solr/core/src/test/org/apache/solr/handler/admin/UpgradeCoreIndexActionTest.java:
##
@@ -365,10 +365,192 @@ public void
testUpgradeCoreIndexFailsWithNestedDocuments() throws Exception {
coreName),
resp));
- // Verify the exception message indicates nested documents are not
supported
+ // Verify the exception message indicates child documents are not
supported
assertThat(
thrown.getMessage(),
- containsString("does not support indexes containing nested
documents"));
+ containsString("does not support indexes containing child
documents"));
+} finally {
+ admin.shutdown();
+ admin.close();
+}
+ }
+
+ // --- Child docs detection tests ---
+ //
+ // These tests verify that the child document detection in the upgrade path
+ // correctly distinguishes between genuine child docs and non-child docs,
+ // even in the presence of updates and deletes that leave deleted documents
+ // in segments (since NoMergePolicy prevents segment merges from purging
them).
+
+ @Test
+ public void testChildDocsDetection_noChildDocsJustAdd() throws Exception {
+for (int i = 0; i < 10; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "doc" + i));
+}
+assertU(commit("openSearcher", "true"));
+
+assertUpgradeDoesNotDetectChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_withChildDocsJustAdd() throws Exception {
+addChildDoc("100", "101");
+addChildDoc("200", "201");
+assertU(commit("openSearcher", "true"));
+
+assertUpgradeDetectsChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_noChildDocsWithWithinCommitUpdates()
throws Exception {
+// Add docs and then update some of them BEFORE committing, so both the old
+// (deleted) and new versions end up in the same flushed segment.
+// With NoMergePolicy and a 100MB RAM buffer (from SolrIndexConfig
defaults),
+// no flush or merge occurs mid-batch, guaranteeing co-location.
+//
+// In the resulting segment, _root_ Terms stats will show:
+// Terms.size() = N (unique _root_ values, one per unique id)
+// Terms.getDocCount() = N + updates (includes deleted doc entries)
+//
+// A naive check (uniqueRootValues < docsWithRoot) may false-positive here
+// because multiple docs share the same _root_ value within the segment.
+for (int i = 0; i < 10; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "doc" + i));
+}
+// Re-add a few docs with the same ids (within-commit updates)
+for (int i = 0; i < 3; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "updated_doc" + i));
+}
+assertU(commit("openSearcher", "true"));
+
+// 10 live docs — the updates replaced 3 docs in-place
+assertQ(req("q", "*:*"), "//result[@numFound='10']");
+assertUpgradeDoesNotDetectChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_withChildDocsWithWithinCommitUpdates()
throws Exception {
Review Comment:
Consider three statistics that are similar but subtly different for
"id-like" fields: `terms.size`, `terms.docCount`, `unique(id)` (i.e. facet via
searcher). When all your deletions are via updates, meaning every deletion of a
solr docId is paired with an addition of that same solr doc Id then you have:
`terms.docCount > terms.size = unique(id)`
However, when you have deletions where none of the deletions are part of
updates then you have:
`terms.docCount = terms.size > unique(id)`
I want to add tests which protect against the wrong kind of refactor which
IMO is not that hard considering the subtelties of the different types of
counts. I know they confused me a bit until I considered all the cases
mentioned above.
--
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-18194: fix nested docs detection false positive [solr]
rahulgoswami commented on code in PR #4279:
URL: https://github.com/apache/solr/pull/4279#discussion_r3097812268
##
solr/core/src/test/org/apache/solr/handler/admin/UpgradeCoreIndexActionTest.java:
##
@@ -365,10 +365,192 @@ public void
testUpgradeCoreIndexFailsWithNestedDocuments() throws Exception {
coreName),
resp));
- // Verify the exception message indicates nested documents are not
supported
+ // Verify the exception message indicates child documents are not
supported
assertThat(
thrown.getMessage(),
- containsString("does not support indexes containing nested
documents"));
+ containsString("does not support indexes containing child
documents"));
+} finally {
+ admin.shutdown();
+ admin.close();
+}
+ }
+
+ // --- Child docs detection tests ---
+ //
+ // These tests verify that the child document detection in the upgrade path
+ // correctly distinguishes between genuine child docs and non-child docs,
+ // even in the presence of updates and deletes that leave deleted documents
+ // in segments (since NoMergePolicy prevents segment merges from purging
them).
+
+ @Test
+ public void testChildDocsDetection_noChildDocsJustAdd() throws Exception {
+for (int i = 0; i < 10; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "doc" + i));
+}
+assertU(commit("openSearcher", "true"));
+
+assertUpgradeDoesNotDetectChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_withChildDocsJustAdd() throws Exception {
+addChildDoc("100", "101");
+addChildDoc("200", "201");
+assertU(commit("openSearcher", "true"));
+
+assertUpgradeDetectsChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_noChildDocsWithWithinCommitUpdates()
throws Exception {
+// Add docs and then update some of them BEFORE committing, so both the old
+// (deleted) and new versions end up in the same flushed segment.
+// With NoMergePolicy and a 100MB RAM buffer (from SolrIndexConfig
defaults),
+// no flush or merge occurs mid-batch, guaranteeing co-location.
+//
+// In the resulting segment, _root_ Terms stats will show:
+// Terms.size() = N (unique _root_ values, one per unique id)
+// Terms.getDocCount() = N + updates (includes deleted doc entries)
+//
+// A naive check (uniqueRootValues < docsWithRoot) may false-positive here
+// because multiple docs share the same _root_ value within the segment.
+for (int i = 0; i < 10; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "doc" + i));
+}
+// Re-add a few docs with the same ids (within-commit updates)
+for (int i = 0; i < 3; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "updated_doc" + i));
+}
+assertU(commit("openSearcher", "true"));
+
+// 10 live docs — the updates replaced 3 docs in-place
+assertQ(req("q", "*:*"), "//result[@numFound='10']");
+assertUpgradeDoesNotDetectChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_withChildDocsWithWithinCommitUpdates()
throws Exception {
Review Comment:
Totally agree with the idea of a utility function for this check. SolrCore
as @dsmiley suggested seems logical. Luke handler seems fine too since it gives
core level stats.
If you don't mind, can you please elaborate the case of "all deletes via
update" that you mentioned? Trying to make sure I fully comprehend 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-18194: fix nested docs detection false positive [solr]
rahulgoswami commented on code in PR #4279:
URL: https://github.com/apache/solr/pull/4279#discussion_r3097812268
##
solr/core/src/test/org/apache/solr/handler/admin/UpgradeCoreIndexActionTest.java:
##
@@ -365,10 +365,192 @@ public void
testUpgradeCoreIndexFailsWithNestedDocuments() throws Exception {
coreName),
resp));
- // Verify the exception message indicates nested documents are not
supported
+ // Verify the exception message indicates child documents are not
supported
assertThat(
thrown.getMessage(),
- containsString("does not support indexes containing nested
documents"));
+ containsString("does not support indexes containing child
documents"));
+} finally {
+ admin.shutdown();
+ admin.close();
+}
+ }
+
+ // --- Child docs detection tests ---
+ //
+ // These tests verify that the child document detection in the upgrade path
+ // correctly distinguishes between genuine child docs and non-child docs,
+ // even in the presence of updates and deletes that leave deleted documents
+ // in segments (since NoMergePolicy prevents segment merges from purging
them).
+
+ @Test
+ public void testChildDocsDetection_noChildDocsJustAdd() throws Exception {
+for (int i = 0; i < 10; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "doc" + i));
+}
+assertU(commit("openSearcher", "true"));
+
+assertUpgradeDoesNotDetectChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_withChildDocsJustAdd() throws Exception {
+addChildDoc("100", "101");
+addChildDoc("200", "201");
+assertU(commit("openSearcher", "true"));
+
+assertUpgradeDetectsChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_noChildDocsWithWithinCommitUpdates()
throws Exception {
+// Add docs and then update some of them BEFORE committing, so both the old
+// (deleted) and new versions end up in the same flushed segment.
+// With NoMergePolicy and a 100MB RAM buffer (from SolrIndexConfig
defaults),
+// no flush or merge occurs mid-batch, guaranteeing co-location.
+//
+// In the resulting segment, _root_ Terms stats will show:
+// Terms.size() = N (unique _root_ values, one per unique id)
+// Terms.getDocCount() = N + updates (includes deleted doc entries)
+//
+// A naive check (uniqueRootValues < docsWithRoot) may false-positive here
+// because multiple docs share the same _root_ value within the segment.
+for (int i = 0; i < 10; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "doc" + i));
+}
+// Re-add a few docs with the same ids (within-commit updates)
+for (int i = 0; i < 3; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "updated_doc" + i));
+}
+assertU(commit("openSearcher", "true"));
+
+// 10 live docs — the updates replaced 3 docs in-place
+assertQ(req("q", "*:*"), "//result[@numFound='10']");
+assertUpgradeDoesNotDetectChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_withChildDocsWithWithinCommitUpdates()
throws Exception {
Review Comment:
Totally agree with the idea of a utility function for this check. SolrCore
as @dsmiley suggested seems logical. Luke handler seems fine too since it gives
core level stats.
If you don't mind, can you please elaborate the case of "all deletes via
update" that you mentioned? I am not sure if I fully comprehend it.
##
solr/core/src/test/org/apache/solr/handler/admin/UpgradeCoreIndexActionTest.java:
##
@@ -365,10 +365,192 @@ public void
testUpgradeCoreIndexFailsWithNestedDocuments() throws Exception {
coreName),
resp));
- // Verify the exception message indicates nested documents are not
supported
+ // Verify the exception message indicates child documents are not
supported
assertThat(
thrown.getMessage(),
- containsString("does not support indexes containing nested
documents"));
+ containsString("does not support indexes containing child
documents"));
+} finally {
+ admin.shutdown();
+ admin.close();
+}
+ }
+
+ // --- Child docs detection tests ---
+ //
+ // These tests verify that the child document detection in the upgrade path
+ // correctly distinguishes between genuine child docs and non-child docs,
+ // even in the presence of updates and deletes that leave deleted documents
+ // in segments (since NoMergePolicy prevents segment merges from purging
them).
+
+ @Test
+ public void testChildDocsDetection_noChildDocsJustAdd() throws Exception {
+for (int i = 0; i < 10; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "doc" + i));
+}
+assertU(commit("openSearcher", "true"));
+
+assertUpgradeDoesNotDetectChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_withChildDo
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
kotman12 commented on code in PR #4279:
URL: https://github.com/apache/solr/pull/4279#discussion_r3087264699
##
solr/core/src/test/org/apache/solr/handler/admin/UpgradeCoreIndexActionTest.java:
##
@@ -365,10 +365,192 @@ public void
testUpgradeCoreIndexFailsWithNestedDocuments() throws Exception {
coreName),
resp));
- // Verify the exception message indicates nested documents are not
supported
+ // Verify the exception message indicates child documents are not
supported
assertThat(
thrown.getMessage(),
- containsString("does not support indexes containing nested
documents"));
+ containsString("does not support indexes containing child
documents"));
+} finally {
+ admin.shutdown();
+ admin.close();
+}
+ }
+
+ // --- Child docs detection tests ---
+ //
+ // These tests verify that the child document detection in the upgrade path
+ // correctly distinguishes between genuine child docs and non-child docs,
+ // even in the presence of updates and deletes that leave deleted documents
+ // in segments (since NoMergePolicy prevents segment merges from purging
them).
+
+ @Test
+ public void testChildDocsDetection_noChildDocsJustAdd() throws Exception {
+for (int i = 0; i < 10; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "doc" + i));
+}
+assertU(commit("openSearcher", "true"));
+
+assertUpgradeDoesNotDetectChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_withChildDocsJustAdd() throws Exception {
+addChildDoc("100", "101");
+addChildDoc("200", "201");
+assertU(commit("openSearcher", "true"));
+
+assertUpgradeDetectsChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_noChildDocsWithWithinCommitUpdates()
throws Exception {
+// Add docs and then update some of them BEFORE committing, so both the old
+// (deleted) and new versions end up in the same flushed segment.
+// With NoMergePolicy and a 100MB RAM buffer (from SolrIndexConfig
defaults),
+// no flush or merge occurs mid-batch, guaranteeing co-location.
+//
+// In the resulting segment, _root_ Terms stats will show:
+// Terms.size() = N (unique _root_ values, one per unique id)
+// Terms.getDocCount() = N + updates (includes deleted doc entries)
+//
+// A naive check (uniqueRootValues < docsWithRoot) may false-positive here
+// because multiple docs share the same _root_ value within the segment.
+for (int i = 0; i < 10; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "doc" + i));
+}
+// Re-add a few docs with the same ids (within-commit updates)
+for (int i = 0; i < 3; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "updated_doc" + i));
+}
+assertU(commit("openSearcher", "true"));
+
+// 10 live docs — the updates replaced 3 docs in-place
+assertQ(req("q", "*:*"), "//result[@numFound='10']");
+assertUpgradeDoesNotDetectChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_withChildDocsWithWithinCommitUpdates()
throws Exception {
Review Comment:
In the current algorithm they are equivalent but things can change so this
is more for regression prevention. I think it's possible to imagine some future
code path that only gets exercised when `unique(id) == idTerms.size()` (which
is the case only if all deletes are via update) or vice versa. We can also
randomize the counts of docs added vs deleted vs updated if we are really
worried about the incremental build time here. All that being said, it feels
like overkill for the `UpgradeCoreIndex` tests because this isn't even the main
point of this class.
That's why I proposed moving the child doc check to a central location
(where they can also be exposed by some info endpoint as well). I know @dsmiley
expressed interest in exposing the child doc check via some info endpoint in
the original PR (luke or maybe in core/collection status?). If we were to go
that direction then we'd probably want to move this check to some centralized
utility.
--
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-18194: fix nested docs detection false positive [solr]
rahulgoswami commented on code in PR #4279:
URL: https://github.com/apache/solr/pull/4279#discussion_r3070165258
##
solr/core/src/test/org/apache/solr/handler/admin/UpgradeCoreIndexActionTest.java:
##
@@ -365,10 +365,192 @@ public void
testUpgradeCoreIndexFailsWithNestedDocuments() throws Exception {
coreName),
resp));
- // Verify the exception message indicates nested documents are not
supported
+ // Verify the exception message indicates child documents are not
supported
assertThat(
thrown.getMessage(),
- containsString("does not support indexes containing nested
documents"));
+ containsString("does not support indexes containing child
documents"));
+} finally {
+ admin.shutdown();
+ admin.close();
+}
+ }
+
+ // --- Child docs detection tests ---
+ //
+ // These tests verify that the child document detection in the upgrade path
+ // correctly distinguishes between genuine child docs and non-child docs,
+ // even in the presence of updates and deletes that leave deleted documents
+ // in segments (since NoMergePolicy prevents segment merges from purging
them).
+
+ @Test
+ public void testChildDocsDetection_noChildDocsJustAdd() throws Exception {
+for (int i = 0; i < 10; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "doc" + i));
+}
+assertU(commit("openSearcher", "true"));
+
+assertUpgradeDoesNotDetectChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_withChildDocsJustAdd() throws Exception {
+addChildDoc("100", "101");
+addChildDoc("200", "201");
+assertU(commit("openSearcher", "true"));
+
+assertUpgradeDetectsChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_noChildDocsWithWithinCommitUpdates()
throws Exception {
+// Add docs and then update some of them BEFORE committing, so both the old
+// (deleted) and new versions end up in the same flushed segment.
+// With NoMergePolicy and a 100MB RAM buffer (from SolrIndexConfig
defaults),
+// no flush or merge occurs mid-batch, guaranteeing co-location.
+//
+// In the resulting segment, _root_ Terms stats will show:
+// Terms.size() = N (unique _root_ values, one per unique id)
+// Terms.getDocCount() = N + updates (includes deleted doc entries)
+//
+// A naive check (uniqueRootValues < docsWithRoot) may false-positive here
+// because multiple docs share the same _root_ value within the segment.
+for (int i = 0; i < 10; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "doc" + i));
+}
+// Re-add a few docs with the same ids (within-commit updates)
+for (int i = 0; i < 3; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "updated_doc" + i));
+}
+assertU(commit("openSearcher", "true"));
+
+// 10 live docs — the updates replaced 3 docs in-place
+assertQ(req("q", "*:*"), "//result[@numFound='10']");
+assertUpgradeDoesNotDetectChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_withChildDocsWithWithinCommitUpdates()
throws Exception {
Review Comment:
I'd argue that all tests of the pattern
testChildDocsDetection_withChildDocs* are getting covered through the existing
test testUpgradeCoreIndexFailsWithChildDocuments() since the with vs
within(CommitUpdates) condition is not a real differentiator in that case.
testChildDocsDetection_noChildDocsWithWithinCommitUpdates() and
testChildDocsDetection_noChildDocsWithWithinCommitDeletesAndUpdates() is all we
need?
--
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-18194: fix nested docs detection false positive [solr]
rahulgoswami commented on code in PR #4279:
URL: https://github.com/apache/solr/pull/4279#discussion_r3070165258
##
solr/core/src/test/org/apache/solr/handler/admin/UpgradeCoreIndexActionTest.java:
##
@@ -365,10 +365,192 @@ public void
testUpgradeCoreIndexFailsWithNestedDocuments() throws Exception {
coreName),
resp));
- // Verify the exception message indicates nested documents are not
supported
+ // Verify the exception message indicates child documents are not
supported
assertThat(
thrown.getMessage(),
- containsString("does not support indexes containing nested
documents"));
+ containsString("does not support indexes containing child
documents"));
+} finally {
+ admin.shutdown();
+ admin.close();
+}
+ }
+
+ // --- Child docs detection tests ---
+ //
+ // These tests verify that the child document detection in the upgrade path
+ // correctly distinguishes between genuine child docs and non-child docs,
+ // even in the presence of updates and deletes that leave deleted documents
+ // in segments (since NoMergePolicy prevents segment merges from purging
them).
+
+ @Test
+ public void testChildDocsDetection_noChildDocsJustAdd() throws Exception {
+for (int i = 0; i < 10; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "doc" + i));
+}
+assertU(commit("openSearcher", "true"));
+
+assertUpgradeDoesNotDetectChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_withChildDocsJustAdd() throws Exception {
+addChildDoc("100", "101");
+addChildDoc("200", "201");
+assertU(commit("openSearcher", "true"));
+
+assertUpgradeDetectsChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_noChildDocsWithWithinCommitUpdates()
throws Exception {
+// Add docs and then update some of them BEFORE committing, so both the old
+// (deleted) and new versions end up in the same flushed segment.
+// With NoMergePolicy and a 100MB RAM buffer (from SolrIndexConfig
defaults),
+// no flush or merge occurs mid-batch, guaranteeing co-location.
+//
+// In the resulting segment, _root_ Terms stats will show:
+// Terms.size() = N (unique _root_ values, one per unique id)
+// Terms.getDocCount() = N + updates (includes deleted doc entries)
+//
+// A naive check (uniqueRootValues < docsWithRoot) may false-positive here
+// because multiple docs share the same _root_ value within the segment.
+for (int i = 0; i < 10; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "doc" + i));
+}
+// Re-add a few docs with the same ids (within-commit updates)
+for (int i = 0; i < 3; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "updated_doc" + i));
+}
+assertU(commit("openSearcher", "true"));
+
+// 10 live docs — the updates replaced 3 docs in-place
+assertQ(req("q", "*:*"), "//result[@numFound='10']");
+assertUpgradeDoesNotDetectChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_withChildDocsWithWithinCommitUpdates()
throws Exception {
Review Comment:
I'd argue that all tests of the pattern
testChildDocsDetection_withChildDocs* are getting covered through the existing
test testUpgradeCoreIndexFailsWithChildDocuments() since the with vs
within(CommitUpdates) condition is not a real differentiator in that case.
testChildDocsDetection_noChildDocsWithWithinCommitUpdates() and
testChildDocsDetection_noChildDocsWithWithinCommitDeletesAndUpdates() is all we
need.
--
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-18194: fix nested docs detection false positive [solr]
rahulgoswami commented on code in PR #4279:
URL: https://github.com/apache/solr/pull/4279#discussion_r3070136016
##
solr/core/src/test/org/apache/solr/handler/admin/UpgradeCoreIndexActionTest.java:
##
@@ -365,10 +365,192 @@ public void
testUpgradeCoreIndexFailsWithNestedDocuments() throws Exception {
coreName),
resp));
- // Verify the exception message indicates nested documents are not
supported
+ // Verify the exception message indicates child documents are not
supported
assertThat(
thrown.getMessage(),
- containsString("does not support indexes containing nested
documents"));
+ containsString("does not support indexes containing child
documents"));
+} finally {
+ admin.shutdown();
+ admin.close();
+}
+ }
+
+ // --- Child docs detection tests ---
+ //
+ // These tests verify that the child document detection in the upgrade path
+ // correctly distinguishes between genuine child docs and non-child docs,
+ // even in the presence of updates and deletes that leave deleted documents
+ // in segments (since NoMergePolicy prevents segment merges from purging
them).
+
+ @Test
+ public void testChildDocsDetection_noChildDocsJustAdd() throws Exception {
Review Comment:
I understand the intent that we are trying to be explicit about the edge
cases of the child docs check. In certain cases however, I'd argue that certain
tests like this one are effectively testing the happy path which is already
getting covered inherently in existing tests like
testNoUpgradeNeededWhenAllSegmentsCurrent().
I appreciate the thorough tests, but I think we have an opportunity to keep
the coverage tight here by relying on certain existing tests like above.
Another reasoning is that every test is "just another test" by itself, but can
gradually add to the build times.
--
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-18194: fix nested docs detection false positive [solr]
rahulgoswami commented on code in PR #4279:
URL: https://github.com/apache/solr/pull/4279#discussion_r3070141353
##
solr/core/src/test/org/apache/solr/handler/admin/UpgradeCoreIndexActionTest.java:
##
@@ -365,10 +365,192 @@ public void
testUpgradeCoreIndexFailsWithNestedDocuments() throws Exception {
coreName),
resp));
- // Verify the exception message indicates nested documents are not
supported
+ // Verify the exception message indicates child documents are not
supported
assertThat(
thrown.getMessage(),
- containsString("does not support indexes containing nested
documents"));
+ containsString("does not support indexes containing child
documents"));
+} finally {
+ admin.shutdown();
+ admin.close();
+}
+ }
+
+ // --- Child docs detection tests ---
+ //
+ // These tests verify that the child document detection in the upgrade path
+ // correctly distinguishes between genuine child docs and non-child docs,
+ // even in the presence of updates and deletes that leave deleted documents
+ // in segments (since NoMergePolicy prevents segment merges from purging
them).
+
+ @Test
+ public void testChildDocsDetection_noChildDocsJustAdd() throws Exception {
+for (int i = 0; i < 10; i++) {
+ assertU(adoc("id", String.valueOf(i), "title", "doc" + i));
+}
+assertU(commit("openSearcher", "true"));
+
+assertUpgradeDoesNotDetectChildDocs();
+ }
+
+ @Test
+ public void testChildDocsDetection_withChildDocsJustAdd() throws Exception {
Review Comment:
Same argument on duplicate coverage as above through existing test
testUpgradeCoreIndexFailsWithNestedDocuments() (now renamed to
testUpgradeCoreIndexFailsWithChildDocuments() in this 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-18194: fix nested docs detection false positive [solr]
rahulgoswami commented on code in PR #4279:
URL: https://github.com/apache/solr/pull/4279#discussion_r3070136016
##
solr/core/src/test/org/apache/solr/handler/admin/UpgradeCoreIndexActionTest.java:
##
@@ -365,10 +365,192 @@ public void
testUpgradeCoreIndexFailsWithNestedDocuments() throws Exception {
coreName),
resp));
- // Verify the exception message indicates nested documents are not
supported
+ // Verify the exception message indicates child documents are not
supported
assertThat(
thrown.getMessage(),
- containsString("does not support indexes containing nested
documents"));
+ containsString("does not support indexes containing child
documents"));
+} finally {
+ admin.shutdown();
+ admin.close();
+}
+ }
+
+ // --- Child docs detection tests ---
+ //
+ // These tests verify that the child document detection in the upgrade path
+ // correctly distinguishes between genuine child docs and non-child docs,
+ // even in the presence of updates and deletes that leave deleted documents
+ // in segments (since NoMergePolicy prevents segment merges from purging
them).
+
+ @Test
+ public void testChildDocsDetection_noChildDocsJustAdd() throws Exception {
Review Comment:
I understand the intent that we are trying to be explicit about the edge
cases of the child docs check. In certain cases however, I'd argue that certain
tests like this one are effectively testing the happy path which is already
getting covered inherently in existing tests like
testNoUpgradeNeededWhenAllSegmentsCurrent().
I appreciate the thorough tests, but I think we have an opportunity to keep
the coverage tight here by piggy backing on certain existing tests like above.
Another reasoning is that every test is "just another test" by itself, but can
gradually add to the build times.
--
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-18194: fix nested docs detection false positive [solr]
dsmiley commented on code in PR #4279:
URL: https://github.com/apache/solr/pull/4279#discussion_r3069594061
##
solr/core/src/test/org/apache/solr/handler/admin/UpgradeCoreIndexActionTest.java:
##
@@ -365,10 +365,192 @@ public void
testUpgradeCoreIndexFailsWithNestedDocuments() throws Exception {
coreName),
resp));
- // Verify the exception message indicates nested documents are not
supported
+ // Verify the exception message indicates child documents are not
supported
assertThat(
thrown.getMessage(),
- containsString("does not support indexes containing nested
documents"));
+ containsString("does not support indexes containing child
documents"));
+} finally {
+ admin.shutdown();
+ admin.close();
+}
+ }
+
+ // --- Child docs detection tests ---
Review Comment:
Thanks for explaining why the tests are what they are. Seems perfect then
:-)
No; not aesthetic reasons. Well okay slightly but whatever.
I love the idea of a utility function somewhere! SolrCore? I hate to
suggest that place given SolrCore's size but it would at least have some
logical sense to its placement there, and discoverability, which matters.
--
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-18194: fix nested docs detection false positive [solr]
kotman12 commented on code in PR #4279:
URL: https://github.com/apache/solr/pull/4279#discussion_r3069514510
##
solr/core/src/test/org/apache/solr/handler/admin/UpgradeCoreIndexActionTest.java:
##
@@ -365,10 +365,192 @@ public void
testUpgradeCoreIndexFailsWithNestedDocuments() throws Exception {
coreName),
resp));
- // Verify the exception message indicates nested documents are not
supported
+ // Verify the exception message indicates child documents are not
supported
assertThat(
thrown.getMessage(),
- containsString("does not support indexes containing nested
documents"));
+ containsString("does not support indexes containing child
documents"));
+} finally {
+ admin.shutdown();
+ admin.close();
+}
+ }
+
+ // --- Child docs detection tests ---
Review Comment:
These are indeed LLM generated but I was very intentional about what
scenarios to test, i.e. hasChildDocs X hasUpdates X hasDeletes. These variables
have different effects on the distinct count vs `docCount` of both `id` and
`_root_` and would like any hypothetical future refactoring to take into
account each since the logic is a function of these variables. I am not
convinced a randomized test would have been effective here without knowing what
edge cases you are looking for a priori. I suppose in theory it could have
caught _other_ things I hadn't considered although _seems_ unlikely given the
relatively simple logic and few moving parts (famous last words). Thus, there
is advantage to deterministically validating these known edge cases with each
build. That being said, it feels like overkill to test these in
`UpgradeCoreIndex` as this check is a rather small component of the overall
operation. Perhaps this check should live in some utility function? Then we
could _also_ call it
from, say, the LukeHandler and expose a `hasNested/ChildDocs` in the index
info. The benefit of such thorough testing would be more obvious in a targeted
utility. What do you think?
As an aside, is your reflexive urge to use randomized testing here motivated
by _aesthetic_ reasons? If so, I could consider parameterizing them. I think it
should be possible.
--
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-18194: fix nested docs detection false positive [solr]
dsmiley commented on code in PR #4279:
URL: https://github.com/apache/solr/pull/4279#discussion_r3068929321
##
solr/core/src/java/org/apache/solr/handler/admin/api/UpgradeCoreIndex.java:
##
@@ -149,11 +149,11 @@ private UpgradeCoreIndexResponse performUpgradeImpl(
RefCounted searcherRef = core.getSearcher();
try {
-// Check for nested documents before processing - we don't support them
-if (indexContainsNestedDocs(searcherRef.get())) {
+// Check for child documents before processing - we don't support them
+if (indexContainsChildDocs(searcherRef.get())) {
throw new SolrException(
BAD_REQUEST,
- "UPGRADECOREINDEX does not support indexes containing nested
documents. "
+ "UPGRADECOREINDEX does not support indexes containing child
documents. "
Review Comment:
```suggestion
"UPGRADECOREINDEX does not support indexes containing
child/nested documents. "
```
Even though nested docs *is* child docs... "nested" should nonetheless be
most common.
##
solr/core/src/test/org/apache/solr/handler/admin/UpgradeCoreIndexActionTest.java:
##
@@ -365,10 +365,192 @@ public void
testUpgradeCoreIndexFailsWithNestedDocuments() throws Exception {
coreName),
resp));
- // Verify the exception message indicates nested documents are not
supported
+ // Verify the exception message indicates child documents are not
supported
assertThat(
thrown.getMessage(),
- containsString("does not support indexes containing nested
documents"));
+ containsString("does not support indexes containing child
documents"));
+} finally {
+ admin.shutdown();
+ admin.close();
+}
+ }
+
+ // --- Child docs detection tests ---
Review Comment:
[0] Wow lots of tests... and they seem only slightly tweaked amongst each.
My observation is that Lucene/Solr old-timers (like me) prefer to write a
minimal number of randomized test instead of writing many repeated tests that
tweak something. I noticed LLMs prefer the latter. But it's fine, really!
--
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-18194: fix nested docs detection false positive [solr]
kotman12 commented on PR #4279: URL: https://github.com/apache/solr/pull/4279#issuecomment-4230616318 Btw @dsmiley I got Claude to factor this as an EmbeddedSolrServerTest [here](https://github.com/kotman12/solr/commit/2f851b94da5d3562bdee0eaef216bbfeba2fb8d9) but I am not sure what to think about it. Part of me thinks the collection-centric language, i.e. `.newCollection()` is a bit weird to put in a test of a feature that is explicitly not supported in cloud mode. That being said it is just a naming thing and the test does work. So its perhaps more of a problem with the solrTestRule. Perhaps I have some trouble grasping the finer details as I've only ever interacted with Solr in Cloud mode. -- 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]
