You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "kotman12 (via GitHub)" <gi...@apache.org> on 2026/04/12 02:17:57 UTC
[PR] Solr-18194: fix nested docs detection false positive [solr]
kotman12 opened a new pull request, #4279:
URL: https://github.com/apache/solr/pull/4279
# Description
The upgrade core index endpoint doesn't currently support child docs and so it guards against running on an index with such documents. The logic to check for the existence of child docs contingent upon the `_root_` field existing could be improved. If you just compare the doc count of `_root_` vs the unique count (current behavior), it will falsely flag any segment with updated docs (where the update happened within one segment) as containing child docs when in actuality it may have no child docs. I actually ran into such a scenario devising my own version of this check.
# Solution
Compare terms size of id field with the terms size of the `_root_` field to ensure no documents share a root. We do this for each segment separately as was done for the previous check.
# Tests
1. testNestedDocsDetection_nonNestedJustAdd — Plain docs, no updates. Verifies no false positive.
2. testNestedDocsDetection_nestedJustAdd — Actual nested docs. Verifies detection works.
3. testNestedDocsDetection_nonNestedWithWithinCommitUpdates — Non-nested docs where some are re-added (same id) before commit. The deleted+re-added docs share _root_ values within the segment, which previously caused false positives.
4. testNestedDocsDetection_nestedWithWithinCommitUpdates — Same within-commit update pattern but with real nested docs present.
5. testNestedDocsDetection_nonNestedWithWithinCommitDeletesAndUpdates — Non-nested docs with deletes AND updates before commit. Deleted entries in the segment can also trigger false positives.
6. testNestedDocsDetection_nestedWithWithinCommitDeletesAndUpdates — Same delete+update pattern with real nested docs.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "rahulgoswami (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "rahulgoswami (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "rahulgoswami (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "rahulgoswami (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "rahulgoswami (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Solr-18194: fix nested docs detection false positive [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "rahulgoswami (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "rahulgoswami (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
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<SolrIndexSearcher> 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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "rahulgoswami (via GitHub)" <gi...@apache.org>.
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.
+ *
+ * <p>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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "rahulgoswami (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "rahulgoswami (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18194: fix nested docs detection false positive [solr]
Posted by "rahulgoswami (via GitHub)" <gi...@apache.org>.
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_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.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org