From 6c273e192b87c1e4db0a4eca11b80c1a5cf952a3 Mon Sep 17 00:00:00 2001 From: Nick Shanin Date: Sun, 4 Oct 2026 01:38:22 -0400 Subject: [PATCH 1/8] SOLR-7520: complete the post filter in distributed grouping shard searches Hypothetical, unrun regression test; see SOLR-7520-TESTING.md. --- .../apache/solr/search/grouping/CommandHandler.java | 10 +++++++++- .../apache/solr/search/AnalyticsMergeStrategyTest.java | 10 ++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/solr/core/src/java/org/apache/solr/search/grouping/CommandHandler.java b/solr/core/src/java/org/apache/solr/search/grouping/CommandHandler.java index 4c11e6eca47f..901a9f103ba2 100644 --- a/solr/core/src/java/org/apache/solr/search/grouping/CommandHandler.java +++ b/solr/core/src/java/org/apache/solr/search/grouping/CommandHandler.java @@ -257,7 +257,15 @@ private void searchWithTimeLimiter(Query query, ProcessedFilter filter, Collecto } try { - searcher.search(query, collector); + try { + searcher.search(query, collector); + } finally { + // let a post filter (e.g. an AnalyticsQuery collector) report its results, as + // SolrIndexSearcher does for non grouped searches + if (filter.postFilter != null) { + filter.postFilter.complete(); + } + } } catch (ExitableDirectoryReader.ExitingReaderException x) { log.warn("Query: {}; ", query, x); } diff --git a/solr/core/src/test/org/apache/solr/search/AnalyticsMergeStrategyTest.java b/solr/core/src/test/org/apache/solr/search/AnalyticsMergeStrategyTest.java index c2b255824651..688681360d3b 100644 --- a/solr/core/src/test/org/apache/solr/search/AnalyticsMergeStrategyTest.java +++ b/solr/core/src/test/org/apache/solr/search/AnalyticsMergeStrategyTest.java @@ -88,6 +88,16 @@ public void test() throws Exception { setDistributedParams(params); rsp = queryRandomShard(params); assertCount(rsp, 4); + + // SOLR-7520: the post filter must also be completed when grouping across shards + params = new ModifiableSolrParams(); + params.add("q", "*:*"); + params.add("fq", "{!count}"); + params.add("group", "true"); + params.add("group.field", "sort_i"); + setDistributedParams(params); + rsp = queryRandomShard(params); + assertNotNull("analytics section missing for grouped query", rsp.getResponse().get("analytics")); } private void assertCountOnly(QueryResponse rsp, int count) throws Exception { From b9b571d0a7e196469c8d00e2af8ea2a087855184 Mon Sep 17 00:00:00 2001 From: Nick Shanin Date: Sun, 4 Oct 2026 01:38:23 -0400 Subject: [PATCH 2/8] SOLR-7520: add changelog fragment --- .../SOLR-7520-grouping-postfilter-complete.yml | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 changelog/unreleased/SOLR-7520-grouping-postfilter-complete.yml diff --git a/changelog/unreleased/SOLR-7520-grouping-postfilter-complete.yml b/changelog/unreleased/SOLR-7520-grouping-postfilter-complete.yml new file mode 100644 index 000000000000..01b42469bd62 --- /dev/null +++ b/changelog/unreleased/SOLR-7520-grouping-postfilter-complete.yml @@ -0,0 +1,10 @@ +title: > + A post filter collector is now completed on shards when a distributed grouping request uses it, + so its complete() results (for example an AnalyticsQuery) are no longer lost. +type: fixed +authors: + - name: Nick Shanin + nick: nick-boss-tech +links: + - name: SOLR-7520 + url: https://issues.apache.org/jira/browse/SOLR-7520 From 3abc1f0bacdaf5d89ef141373879d36b400d81f6 Mon Sep 17 00:00:00 2001 From: Nick Shanin Date: Sun, 4 Oct 2026 01:38:24 -0400 Subject: [PATCH 3/8] SOLR-7520: add hypothetical-reproduction handoff doc --- SOLR-7520-TESTING.md | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 SOLR-7520-TESTING.md diff --git a/SOLR-7520-TESTING.md b/SOLR-7520-TESTING.md new file mode 100644 index 000000000000..d2433de1c281 --- /dev/null +++ b/SOLR-7520-TESTING.md @@ -0,0 +1,30 @@ +# SOLR-7520 - hypothetical-reproduction handoff + +**Read this first: nothing on this branch was compiled or run.** The research/implement pipeline has no Gradle access. The fix and test are best-guess; treat them as hypotheses. + +- JIRA: https://issues.apache.org/jira/browse/SOLR-7520 - "Post filter DelegatingCollector.finish not called for multi-shard queries specifying grouping" (Eric Wheeler). The reporter suggested calling the post filter's finish at the end of `CommandHandler.searchWithTimeLimiter`; the method was later renamed `complete()`. +- Branch: `solr-7520-submit` off `apache/solr` main `14c7aac0d15` +- Commits: fix + test, changelog fragment, this file (drop the doc before a PR) + +## The bug, as understood +`SolrIndexSearcher` (non grouped) and `Grouping` (non distributed grouping) call `DelegatingCollector.complete()` after the search. `grouping.CommandHandler.searchWithTimeLimiter` (used for the shard side of distributed grouping) wraps the collector with `filter.postFilter` but never completes it, so post filters that emit results in `complete()` produce nothing. Still true on main. + +## What the branch changes +- `CommandHandler.searchWithTimeLimiter`: after `searcher.search(...)`, call `filter.postFilter.complete()` (in a `finally`, inside the existing `ExitingReaderException` catch, mirroring `SolrIndexSearcher`). +- `AnalyticsMergeStrategyTest`: extra grouped query (`group=true&group.field=sort_i`) with `fq={!count}`; asserts the `analytics` section exists in the response. + +## What was guessed (verify these first) +1. Grouping runs the first-phase search and the second-phase search separately, so `complete()` can run more than once per shard and `TestAnalyticsCollector.complete()` would add the `analytics` entry twice on the shard response. The test only checks presence; exact `mycount` is not asserted because of this. +2. The coordinator may not run the merge strategy for grouped responses, in which case `analytics` might still be missing at the coordinator even with the fix. If so the test needs a different assertion (e.g. a shard-side check with `distrib=false`). +3. `sort_i` is a valid single-valued group field in `schema15.xml` (used for sorting in this test already). +4. `DelegatingCollector.complete()` is public and throws `IOException` (as used in `SolrIndexSearcher`). + +## How to verify (Gradle required; not run here) +``` +.\gradlew :solr:core:spotlessApply +.\gradlew :solr:core:test --tests "org.apache.solr.search.AnalyticsMergeStrategyTest" +``` +Fail-before: revert only `CommandHandler.java`. + +## Not done +No JIRA comment, no PR. Changelog author is `Nick Shanin` per the ICLA note. From 3cad95879cfa101b16e108e6aba409939ef1a2b1 Mon Sep 17 00:00:00 2001 From: Nick Shanin Date: Sun, 4 Oct 2026 23:15:49 +0000 Subject: [PATCH 4/8] SOLR-7520: fix the changelog author entry --- changelog/unreleased/SOLR-7520-grouping-postfilter-complete.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/changelog/unreleased/SOLR-7520-grouping-postfilter-complete.yml b/changelog/unreleased/SOLR-7520-grouping-postfilter-complete.yml index 01b42469bd62..ce03b19e78d1 100644 --- a/changelog/unreleased/SOLR-7520-grouping-postfilter-complete.yml +++ b/changelog/unreleased/SOLR-7520-grouping-postfilter-complete.yml @@ -4,7 +4,6 @@ title: > type: fixed authors: - name: Nick Shanin - nick: nick-boss-tech links: - name: SOLR-7520 url: https://issues.apache.org/jira/browse/SOLR-7520 From 10b6931e1c058d76392c4ba6d53b45bee251e3d2 Mon Sep 17 00:00:00 2001 From: Nick Shanin Date: Mon, 5 Oct 2026 10:38:54 +0000 Subject: [PATCH 5/8] SOLR-7520: assert the grouped post filter completion on the shard response The shipped test asserted on the coordinator response, but merge strategies do not run for grouped responses at the coordinator, so that assertion cannot pass on this change alone. Assert on a first phase shard response instead, which is the behavior this change fixes. Also narrow the changelog title to what the change delivers, and drop the handoff doc. --- SOLR-7520-TESTING.md | 30 ------------------- ...SOLR-7520-grouping-postfilter-complete.yml | 2 +- .../search/AnalyticsMergeStrategyTest.java | 14 ++++++--- 3 files changed, 11 insertions(+), 35 deletions(-) delete mode 100644 SOLR-7520-TESTING.md diff --git a/SOLR-7520-TESTING.md b/SOLR-7520-TESTING.md deleted file mode 100644 index d2433de1c281..000000000000 --- a/SOLR-7520-TESTING.md +++ /dev/null @@ -1,30 +0,0 @@ -# SOLR-7520 - hypothetical-reproduction handoff - -**Read this first: nothing on this branch was compiled or run.** The research/implement pipeline has no Gradle access. The fix and test are best-guess; treat them as hypotheses. - -- JIRA: https://issues.apache.org/jira/browse/SOLR-7520 - "Post filter DelegatingCollector.finish not called for multi-shard queries specifying grouping" (Eric Wheeler). The reporter suggested calling the post filter's finish at the end of `CommandHandler.searchWithTimeLimiter`; the method was later renamed `complete()`. -- Branch: `solr-7520-submit` off `apache/solr` main `14c7aac0d15` -- Commits: fix + test, changelog fragment, this file (drop the doc before a PR) - -## The bug, as understood -`SolrIndexSearcher` (non grouped) and `Grouping` (non distributed grouping) call `DelegatingCollector.complete()` after the search. `grouping.CommandHandler.searchWithTimeLimiter` (used for the shard side of distributed grouping) wraps the collector with `filter.postFilter` but never completes it, so post filters that emit results in `complete()` produce nothing. Still true on main. - -## What the branch changes -- `CommandHandler.searchWithTimeLimiter`: after `searcher.search(...)`, call `filter.postFilter.complete()` (in a `finally`, inside the existing `ExitingReaderException` catch, mirroring `SolrIndexSearcher`). -- `AnalyticsMergeStrategyTest`: extra grouped query (`group=true&group.field=sort_i`) with `fq={!count}`; asserts the `analytics` section exists in the response. - -## What was guessed (verify these first) -1. Grouping runs the first-phase search and the second-phase search separately, so `complete()` can run more than once per shard and `TestAnalyticsCollector.complete()` would add the `analytics` entry twice on the shard response. The test only checks presence; exact `mycount` is not asserted because of this. -2. The coordinator may not run the merge strategy for grouped responses, in which case `analytics` might still be missing at the coordinator even with the fix. If so the test needs a different assertion (e.g. a shard-side check with `distrib=false`). -3. `sort_i` is a valid single-valued group field in `schema15.xml` (used for sorting in this test already). -4. `DelegatingCollector.complete()` is public and throws `IOException` (as used in `SolrIndexSearcher`). - -## How to verify (Gradle required; not run here) -``` -.\gradlew :solr:core:spotlessApply -.\gradlew :solr:core:test --tests "org.apache.solr.search.AnalyticsMergeStrategyTest" -``` -Fail-before: revert only `CommandHandler.java`. - -## Not done -No JIRA comment, no PR. Changelog author is `Nick Shanin` per the ICLA note. diff --git a/changelog/unreleased/SOLR-7520-grouping-postfilter-complete.yml b/changelog/unreleased/SOLR-7520-grouping-postfilter-complete.yml index ce03b19e78d1..8007c6d5d80d 100644 --- a/changelog/unreleased/SOLR-7520-grouping-postfilter-complete.yml +++ b/changelog/unreleased/SOLR-7520-grouping-postfilter-complete.yml @@ -1,6 +1,6 @@ title: > A post filter collector is now completed on shards when a distributed grouping request uses it, - so its complete() results (for example an AnalyticsQuery) are no longer lost. + so its complete() results (for example an AnalyticsQuery) are included in the shard response. type: fixed authors: - name: Nick Shanin diff --git a/solr/core/src/test/org/apache/solr/search/AnalyticsMergeStrategyTest.java b/solr/core/src/test/org/apache/solr/search/AnalyticsMergeStrategyTest.java index 688681360d3b..f6faa864ee2b 100644 --- a/solr/core/src/test/org/apache/solr/search/AnalyticsMergeStrategyTest.java +++ b/solr/core/src/test/org/apache/solr/search/AnalyticsMergeStrategyTest.java @@ -20,6 +20,7 @@ import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope.Scope; import org.apache.solr.BaseDistributedSearchTestCase; import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.client.solrj.request.QueryRequest; import org.apache.solr.client.solrj.response.QueryResponse; import org.apache.solr.common.params.ModifiableSolrParams; import org.apache.solr.common.util.NamedList; @@ -89,15 +90,20 @@ public void test() throws Exception { rsp = queryRandomShard(params); assertCount(rsp, 4); - // SOLR-7520: the post filter must also be completed when grouping across shards + // SOLR-7520: the post filter must also be completed on shards when grouping. + // Merge strategies do not run for grouped responses at the coordinator, so + // assert on a shard response, using the first phase request shape that the + // coordinator sends to each shard. params = new ModifiableSolrParams(); params.add("q", "*:*"); params.add("fq", "{!count}"); params.add("group", "true"); params.add("group.field", "sort_i"); - setDistributedParams(params); - rsp = queryRandomShard(params); - assertNotNull("analytics section missing for grouped query", rsp.getResponse().get("analytics")); + params.add("group.distributed.first", "true"); + QueryResponse shardRsp = new QueryRequest(params).process(clients.get(0)); + assertNotNull( + "analytics section missing for grouped shard query", + shardRsp.getResponse().get("analytics")); } private void assertCountOnly(QueryResponse rsp, int count) throws Exception { From 209cb27f1a486197566e52157cb2e30b21e344dd Mon Sep 17 00:00:00 2001 From: Nick Shanin Date: Thu, 15 Oct 2026 03:00:52 -0400 Subject: [PATCH 6/8] SOLR-7520: check the shard count for both grouped phases and the coordinator groups --- .../search/AnalyticsMergeStrategyTest.java | 44 ++++++++++++++++--- 1 file changed, 37 insertions(+), 7 deletions(-) diff --git a/solr/core/src/test/org/apache/solr/search/AnalyticsMergeStrategyTest.java b/solr/core/src/test/org/apache/solr/search/AnalyticsMergeStrategyTest.java index f6faa864ee2b..96c0a2008899 100644 --- a/solr/core/src/test/org/apache/solr/search/AnalyticsMergeStrategyTest.java +++ b/solr/core/src/test/org/apache/solr/search/AnalyticsMergeStrategyTest.java @@ -90,10 +90,8 @@ public void test() throws Exception { rsp = queryRandomShard(params); assertCount(rsp, 4); - // SOLR-7520: the post filter must also be completed on shards when grouping. - // Merge strategies do not run for grouped responses at the coordinator, so - // assert on a shard response, using the first phase request shape that the - // coordinator sends to each shard. + // Merge strategies do not run for grouped responses at the coordinator, so the grouped + // analytics checks read shard responses. Shard 0 holds docs 1, 2 and 7. params = new ModifiableSolrParams(); params.add("q", "*:*"); params.add("fq", "{!count}"); @@ -101,9 +99,41 @@ public void test() throws Exception { params.add("group.field", "sort_i"); params.add("group.distributed.first", "true"); QueryResponse shardRsp = new QueryRequest(params).process(clients.get(0)); - assertNotNull( - "analytics section missing for grouped shard query", - shardRsp.getResponse().get("analytics")); + assertShardCount(shardRsp, "analytics section missing for grouped shard query", 3); + + params = new ModifiableSolrParams(); + params.add("q", "*:*"); + params.add("fq", "{!count}"); + params.add("group", "true"); + params.add("group.field", "sort_i"); + params.add("group.distributed.second", "true"); + params.add("group.topgroups.sort_i", "5"); + params.add("group.topgroups.sort_i", "50"); + shardRsp = new QueryRequest(params).process(clients.get(0)); + assertShardCount( + shardRsp, "analytics section missing for grouped second phase shard query", 3); + + params = new ModifiableSolrParams(); + params.add("q", "*:*"); + params.add("fq", "{!count}"); + params.add("group", "true"); + params.add("group.field", "sort_i"); + params.add("group.ngroups", "true"); + setDistributedParams(params); + rsp = queryRandomShard(params); + assertEquals(11, rsp.getGroupResponse().getValues().get(0).getNGroups().intValue()); + assertEquals(11, rsp.getGroupResponse().getValues().get(0).getMatches()); + } + + private void assertShardCount(QueryResponse shardRsp, String missingMessage, int count) + throws Exception { + @SuppressWarnings({"rawtypes"}) + NamedList analytics = (NamedList) shardRsp.getResponse().get("analytics"); + assertNotNull(missingMessage, analytics); + Integer c = (Integer) analytics.get("mycount"); + if (c != count) { + throw new Exception("Shard count is not correct:" + count + ":" + c); + } } private void assertCountOnly(QueryResponse rsp, int count) throws Exception { From a6dc792b365b8bbbfd61b1f43555a26a322ba91a Mon Sep 17 00:00:00 2001 From: Nick Shanin Date: Thu, 15 Oct 2026 03:32:00 -0400 Subject: [PATCH 7/8] SOLR-7520: give the grouped second phase its own test method --- .../search/AnalyticsMergeStrategyTest.java | 62 +++++++++++-------- 1 file changed, 36 insertions(+), 26 deletions(-) diff --git a/solr/core/src/test/org/apache/solr/search/AnalyticsMergeStrategyTest.java b/solr/core/src/test/org/apache/solr/search/AnalyticsMergeStrategyTest.java index 96c0a2008899..c578823a8dd3 100644 --- a/solr/core/src/test/org/apache/solr/search/AnalyticsMergeStrategyTest.java +++ b/solr/core/src/test/org/apache/solr/search/AnalyticsMergeStrategyTest.java @@ -46,21 +46,7 @@ public static void setUpBeforeClass() throws Exception { @Test @ShardsFixed(num = 3) public void test() throws Exception { - del("*:*"); - - index_specific(0, "id", "1", "sort_i", "5"); - index_specific(0, "id", "2", "sort_i", "50"); - index_specific(1, "id", "5", "sort_i", "4"); - index_specific(1, "id", "6", "sort_i", "10"); - index_specific(0, "id", "7", "sort_i", "1"); - index_specific(1, "id", "8", "sort_i", "2"); - index_specific(2, "id", "9", "sort_i", "1000"); - index_specific(2, "id", "10", "sort_i", "1500"); - index_specific(2, "id", "11", "sort_i", "1300"); - index_specific(1, "id", "12", "sort_i", "15"); - index_specific(1, "id", "13", "sort_i", "16"); - - commit(); + indexDocs(); /* * The count qparser plugin is pointing to AnalyticsTestQParserPlugin. This class defines a @@ -106,23 +92,47 @@ public void test() throws Exception { params.add("fq", "{!count}"); params.add("group", "true"); params.add("group.field", "sort_i"); + params.add("group.ngroups", "true"); + setDistributedParams(params); + rsp = queryRandomShard(params); + assertEquals(11, rsp.getGroupResponse().getValues().get(0).getNGroups().intValue()); + assertEquals(11, rsp.getGroupResponse().getValues().get(0).getMatches()); + } + + @Test + @ShardsFixed(num = 3) + public void testGroupedSecondPhase() throws Exception { + indexDocs(); + + ModifiableSolrParams params = new ModifiableSolrParams(); + params.add("q", "*:*"); + params.add("fq", "{!count}"); + params.add("group", "true"); + params.add("group.field", "sort_i"); params.add("group.distributed.second", "true"); params.add("group.topgroups.sort_i", "5"); params.add("group.topgroups.sort_i", "50"); - shardRsp = new QueryRequest(params).process(clients.get(0)); + QueryResponse shardRsp = new QueryRequest(params).process(clients.get(0)); assertShardCount( shardRsp, "analytics section missing for grouped second phase shard query", 3); + } - params = new ModifiableSolrParams(); - params.add("q", "*:*"); - params.add("fq", "{!count}"); - params.add("group", "true"); - params.add("group.field", "sort_i"); - params.add("group.ngroups", "true"); - setDistributedParams(params); - rsp = queryRandomShard(params); - assertEquals(11, rsp.getGroupResponse().getValues().get(0).getNGroups().intValue()); - assertEquals(11, rsp.getGroupResponse().getValues().get(0).getMatches()); + private void indexDocs() throws Exception { + del("*:*"); + + index_specific(0, "id", "1", "sort_i", "5"); + index_specific(0, "id", "2", "sort_i", "50"); + index_specific(1, "id", "5", "sort_i", "4"); + index_specific(1, "id", "6", "sort_i", "10"); + index_specific(0, "id", "7", "sort_i", "1"); + index_specific(1, "id", "8", "sort_i", "2"); + index_specific(2, "id", "9", "sort_i", "1000"); + index_specific(2, "id", "10", "sort_i", "1500"); + index_specific(2, "id", "11", "sort_i", "1300"); + index_specific(1, "id", "12", "sort_i", "15"); + index_specific(1, "id", "13", "sort_i", "16"); + + commit(); } private void assertShardCount(QueryResponse shardRsp, String missingMessage, int count) From 15cc9ba5a63cd79518a72bda1fc67539956f5c31 Mon Sep 17 00:00:00 2001 From: Nick Shanin Date: Thu, 15 Oct 2026 21:21:46 +0000 Subject: [PATCH 8/8] SOLR-7520: apply tidy formatting to AnalyticsMergeStrategyTest --- .../org/apache/solr/search/AnalyticsMergeStrategyTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/solr/core/src/test/org/apache/solr/search/AnalyticsMergeStrategyTest.java b/solr/core/src/test/org/apache/solr/search/AnalyticsMergeStrategyTest.java index c578823a8dd3..93821846308b 100644 --- a/solr/core/src/test/org/apache/solr/search/AnalyticsMergeStrategyTest.java +++ b/solr/core/src/test/org/apache/solr/search/AnalyticsMergeStrategyTest.java @@ -113,8 +113,7 @@ public void testGroupedSecondPhase() throws Exception { params.add("group.topgroups.sort_i", "5"); params.add("group.topgroups.sort_i", "50"); QueryResponse shardRsp = new QueryRequest(params).process(clients.get(0)); - assertShardCount( - shardRsp, "analytics section missing for grouped second phase shard query", 3); + assertShardCount(shardRsp, "analytics section missing for grouped second phase shard query", 3); } private void indexDocs() throws Exception {