[
https://issues.apache.org/jira/browse/SOLR-18541?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Nick Shanin updated SOLR-18541:
-------------------------------
Description:
🤖 _AI text below_ 🤖 _(posted on behalf of Nick Shanin)_
Combined queries merge their per-query shard results in
{{CombinedQueryComponent.mergeIds}}, and three defects sit in the totals that
merge reports. First, a partial shard that sends a per-query entry is counted
in that query's totals before the partial skip is tested, so its documents are
dropped but its hits are not: in a constructed partial-response case run on
unmodified main, the merged {{numFound}} was 7 instead of 3 and
{{approximateTotalHits}} was 15 instead of 5. Second, when a shard sends no
{{approximateTotalHits}} header, the fallback adds the running per-query total,
which already includes the current shard, so two shards with 2 and 3 documents
merged to 7 instead of 5. Third, the {{hitCountIsExact}} fold is inverted: an
all-exact merge reports {{numFoundExact=false}} and an all-inexact merge
reports true, both confirmed by run on unmodified main.
The fix I recommend is to test the skip before accumulating, with the other two
corrections in the same change: the fallback should add the shard's own count
instead of the running total, and the fold should keep the merged flag true
only when every shard reports its count exact. Correcting the totals after the
skip fires instead would mean reconstructing what the skipped shard added, and
the exactness fold cannot be un-folded without per-shard tracking. One question
for the maintainer: when several sub-queries contribute, {{numFound}} is
currently the largest single sub-query count; should it instead count the
distinct documents that match any sub-query, and should {{numFoundExact}} be
false whenever more than one sub-query contributes?
AI agents assisted with research, implementation, review, and drafting. Nick
Shanin directed the work and takes responsibility for the above text.
was:
h2. Symptom
Combined queries (the hybrid search path that fuses the result sets of several
queries, for example with reciprocal rank fusion) merge their per-query shard
results in \{{CombinedQueryComponent.mergeIds}} (from a grep, line 285). Three
defects sit in the totals that merge reports:
 * When a shard response for a query is flagged partial and carries a per-query
entry for that query but no \{{sort_values_<q>}}, and the sort needs
unmarshalling, the merging node drops that shard's documents for the query, but
the shard was already counted in the query's totals before the skip was tested
(from code reading; confirmed by run only for constructed responses; see
Reproduction). That query's \{{numFound}} includes hits from a shard that
contributed no documents to it (7 instead of 3, confirmed by run, case P
below), and its \{{approximateTotalHits}} includes the skipped shard's
approximate count (15 instead of 5, confirmed by run, case P2 below). The real
\{{timeAllowed}} exit is not the shape that produces this (from code reading,
not run). In the one exit shape settled by code reading, where the exit stops
the shard while it prepares the first query's sort values and the sort needs
unmarshalling, the shard adds no per-query entry for that query at all, so the
merging node folds neither the shard's \{{numFound}} nor its exactness for that
query, and the \{{approximateTotalHits}} value carried in its response header,
or with no header the running total, can still be added at the merge for it
(from code reading, not run). Two other exit shapes are not settled: under a
score-only sort an exit cannot stop in sort-value preparation and stops in the
search itself (from code reading, not run), and an exit on a later query leaves
that shard's per-query list shorter than the query count (from code reading,
not run). Whether the merge folds, skips or fails in those shapes is not
established, and neither outcome is claimed here.
 * When a shard response carries no \{{approximateTotalHits}} header, the merge
falls back to adding the running per-query total, which already includes the
current shard when that shard sent a per-query entry (from code reading; see
Mechanism). Headerless shards therefore accumulate cumulatively instead of once
each: two shards with 2 and 3 documents and no \{{approximateTotalHits}} header
merge to an approximate total of 7 instead of 5 in arrival order A then B
(confirmed by run, fallback case below, where a shard flags
maxHitsTerminatedEarly so the total reaches the response; A the shard with 2
documents, B the shard with 3; in the other order the total is 8, from code
reading, not run). The \{{numFound}} for that query is correct in that case (5,
confirmed by run, fallback case below); the approximate total is wrong there (7
instead of 5, confirmed by run, same case); case F does not state the merged
numFoundExact, so this sentence makes no claim about it.
 * The \{{hitCountIsExact}} fold is inverted: it keeps the merged flag true
only while shards report their counts as not exact (from code reading, line
343). Where a single sub-query contributes, an all-exact merge of its shards
reports \{{numFoundExact=false}}, and an all-inexact merge reports
\{{numFoundExact=true}} (confirmed by run, cases X1 and X2 below). Only a mixed
merge comes out right (from code reading, not run). What the flag should report
when several sub-queries contribute is part of the open question in Fix options.
The response header does carry \{{partialResults=true}} when a shard is
partial, so callers know the result set is partial; nothing tells them the
counts are not the counts of the merged result (from code reading, not run).
h2. Mechanism
{\{CombinedQueryComponent.mergeIds}} (main,
\{{solr/core/src/java/org/apache/solr/handler/component/CombinedQueryComponent.java}})
loops over the queries to combine (line 328), and inside that over the shard
responses (line 334). The per-query totals \{{queryNumFound}} and
\{{queryApproximateTotalHits}} reset for each query (lines 330 to 331).
\{{hitCountIsExact}} is declared once, outside the query loop (line 315), so
one flag serves every query, and it is written to the response at line 449
(\{{responseDocs.setNumFoundExact(hitCountIsExact)}}). The per-query totals
fold into the merged totals at lines 441 to 442, numFound is written at line
448, and approximateTotalHits reaches the response header only at lines 479 to
487, when maxHitsTerminatedEarly is set, and each shard that sends a per-query
entry has its contribution folded in before the partial-results skip (every
line reference in this paragraph and in the list below is from code reading;
see Reproduction):
 * \{{queryNumFound += docs.getNumFound()}} at line 342
 * the \{{hitCountIsExact}} fold at line 343: \{{hitCountIsExact =
hitCountIsExact && Boolean.FALSE.equals(docs.getNumFoundExact())}}
 * the exception \{{continue}} at lines 348 to 350 also comes after those folds
 * the partial flag is read from the response header at lines 359 to 364
 * the approximate block at lines 381 to 387: the header value is added at line
386, and when the header is absent the fallback at line 384 adds
\{{queryNumFound}}
 * the \{{sort_values_<q>}} lookup is at lines 390 to 396, and
\{{needsUnmarshalling}} is computed at line 398
 * the skip itself is at lines 399 to 400: \{{if (thisResponseIsPartial &&
sortFieldValues.size() == 0 && needsUnmarshalling) continue;}}
So a skipped shard that carries a per-query entry for the query has already
contributed its count, its exactness and its approximate count to the per-query
totals before the \{{continue}} fires (from code reading; confirmed by run only
for constructed responses; see Reproduction).
The fallback operand is the second defect. \{{queryNumFound}} at line 384
already includes the current shard when that shard sent a per-query entry,
because line 342 ran earlier in the same iteration, so each headerless shard
adds the running total over all shards seen so far, headered or not (from code
reading; the resulting total is confirmed by run in the fallback case below).
With only headerless shards the approximate total is the sum over shards of
each shard's count multiplied by the number of headerless shards arriving at or
after it that complete normally (no shard exception; a headerless partial
response counts even when the skip drops it), which is why the value depends on
arrival order (from code reading, not run; responses arrive in completion
order, HttpShardHandler.java line 440). The sibling fallback in
\{{QueryComponent}} has the same line shape with the opposite error: there the
running total is read before the current shard is folded in, so two shards of 2
and 3 in arrival order A then B give 2 instead of 5 (from code reading, not
run); here the same order gives 7 instead of 5 (confirmed by run, fallback case
below).
The fold at line 343 is the third defect. \{{SolrDocumentList.numFoundExact}}
defaults to true
(\{{solr/solrj/src/java/org/apache/solr/common/SolrDocumentList.java}}, line
50), and a shard writes true when its count is exact. The written form keeps
the merged flag true only while shards report FALSE, the inverse of the meaning
the flag carries at line 449. \{{QueryComponent}} uses the other form (lines
1173 to 1174 at main): \{{if (hitCountIsExact &&
Boolean.FALSE.equals(docs.getNumFoundExact())) hitCountIsExact = false;}},
which clears the flag when a shard reports not exact and otherwise leaves it.
(from code reading, not run)
Nothing in the skip depends on a total (from code reading, not run): its inputs
are the partial flag, the sort values and \{{needsUnmarshalling}}, so the skip
can be decided inside each (query, shard) iteration before any fold, the same
reorder shape as the \{{QueryComponent}} fix.
h2. Reproduction
Confirmed by run on unmodified main \{{d76fc14b9857}} with a scratch probe
harness: the class \{{CombinedQueryComponentMergeTotalsTest}} in the
component's own test package, driving fabricated shard responses through
\{{mergeIds}}, seed \{{C0FFEE42C0FFEE42}}. The probe is a scratch harness used
to confirm the defects; it is not a proposed test. All seven probe cases failed
on unmodified main; for P, P2, F, X1 and X2 each failure was an assertion
carrying the predicted wrong value (M1 and M2 outcomes are at line 68). The
seven cases are P and P2 (the partial-skip case below, asserting \{{numFound}}
and \{{approximateTotalHits}} as two cases), F (the fallback case below), X1
(one sub-query; all shards exact, merged flag false) and X2 (one sub-query; all
shards inexact, merged flag true), the exactness cases below and M1 and M2 (the
two maximum-fold cases, whose outcomes are stated with the open question in Fix
options):
 * Partial skip, one query, sorted on a field so sort values need
unmarshalling: a partial shard with an \{{approximateTotalHits}} header of 10,
\{{numFound=4}} and no \{{sort_values_0}}; a complete shard with a header of 5
and \{{numFound=3}}. Merged \{{numFound}} was 7 instead of 3, and merged
\{{approximateTotalHits}} was 15 instead of 5 (case P2). This case is
constructed: it hands the merge a partial response that carries a per-query
entry but no sort values, which is the skip condition itself, and it is not the
shape a real \{{timeAllowed}} exit produces (from code reading, not run; see
Symptom).
 * Fallback, two complete shards with 2 and 3 documents, neither sending an
\{{approximateTotalHits}} header, the first flagging
\{{maxHitsTerminatedEarly}}: merged \{{numFound}} was 5 (correct), and merged
\{{approximateTotalHits}} was 7 instead of 5 (2, then the running 2 + 3 folded
again).
 * Exactness, case X1 (one sub-query): two shards whose per-query lists both
report \{{numFoundExact=true}}: the merged flag was false instead of true. Case
X2 (one sub-query): with both reporting false, the merged flag was true instead
of false.
None of the component's existing tests asserts \{{numFoundExact}} or
\{{approximateTotalHits}} on the merged response (from a grep of the
component's test classes, not run).
End to end, no traced shard path produces the constructed shape (from code
reading, not run). In the exit shape settled by code reading, a shard's
\{{timeAllowed}} expires while it prepares the first query's sort values and
the sort needs unmarshalling: it stops before adding that query's per-query
entry, the search handler returns the response flagged partial with no
per-query entry for that query, and the merging node folds neither that shard's
\{{numFound}} nor its exactness for the unprepared query, though the
\{{approximateTotalHits}} value in the shard's response header, or with no
header the running total, can still be added for it at the merge (from code
reading, not run). Two other exit shapes are not settled, and nothing here
claims their outcomes: under a score-only sort the exit cannot stop in
sort-value preparation, so it stops in the search itself with the same search
handler products (from code reading, not run); and an exit on a later query
leaves the shard's per-query list shorter than the query count (from code
reading, not run), where the merge reads it with no size check: the unguarded
\{{docList.get(queryIndex)}} at line 340 throws IndexOutOfBoundsException at
the first query the list lacks, for a List such as ArrayList (from code
reading, not run; whether a caller of mergeIds catches it is not checked here).
What the reproduction pins is the ordering in the merge itself: any response
that does reach the merge flagged partial, with a per-query entry and no sort
values for a query whose sort needs unmarshalling, is counted in that query's
totals before it is skipped (from code reading; confirmed by run only for
constructed responses). The fallback and the fold need no partial results at
all: any shard response that completes without the \{{approximateTotalHits}}
header exercises the fallback, and the folds at lines 342 and 343 fire on every
merge in which a shard sends a per-query entry (from code reading, not run).
h2. Fix options
Three smallest changes present themselves, one per defect:
 * Test the skip condition before accumulating the shard's per-query totals
(equivalently, move the folds at lines 342 and 343 and the approximate block at
lines 381 to 387 below the skip at lines 399 to 400), so that a skipped shard
would never enter that query's totals. One detail rides with it: after the
reorder, a query whose shards are all skipped would contribute a count of zero,
and how that case bears on the shared exactness flag needs the same care as the
all-skipped case in the \{{QueryComponent}} fix.
 * Correct the fallback operand: when a shard sends no
\{{approximateTotalHits}} header, add that shard's own count
(\{{docs.getNumFound()}}) instead of the running \{{queryNumFound}}, with a
null guard giving 0, because \{{docs}} can be null when a shard has no
per-query entry (from code reading, not run).
 * Correct the fold direction at line 343: \{{hitCountIsExact = hitCountIsExact
&& !Boolean.FALSE.equals(docs.getNumFoundExact())}}, equivalently the \{{if}}
form \{{QueryComponent}} uses at lines 1173 to 1174 (from code reading, not
run), so that, for the lists a single sub-query contributes, the merged flag
would be true only when every contributing list reports its count exact. The
flag is one flag shared by every query (from code reading, line 315), and what
it should report when several sub-queries contribute is part of the open
question below; this change settles the single-sub-query case, which the
reproduction pins.
The first change is the recommended one, with the second and third as part of
the same fix. Correcting the totals when the skip fires instead would mean
reconstructing what the skipped shard added after the fact: for the approximate
total that would mean knowing which branch contributed and, on the fallback
branch, what the running total was at that shard's arrival position, and no
subtraction can repair the exactness flag, which is a fold over every shard
that sends a per-query entry and cannot be un-folded without per-shard tracking
(from code reading, not run). Testing the skip before accumulating would leave
each query's totals describing the same set of shards that query merges. The
second change is required rather than optional: the fallback fires for complete
shards too (confirmed by run, fallback case F in Reproduction), not only
skipped ones, so a reorder alone would leave the double count. The third is one
line in the same accumulation, and without it the merged response keeps
reporting an exact merge as inexact (case X1 confirms this by run for one
sub-query; that the one-line change corrects the fold for shards that are not
skipped is from code reading, not run).
Open question for the maintainer: for a combined query with two or more
sub-queries, the response \{{numFound}} is currently the largest single
sub-query match count (the per-query totals fold into the combined totals by
maximum, at lines 441 to 442), except that a failed queue push lowers the
running numFound first (line 434), so it can end below that count (from code
reading; the two maximum-fold cases below confirm the merged
approximateTotalHits by run, and the draft does not state their per-query
numFound inputs). Sub-queries match their own documents, overlapping or not, so
this number can be smaller than the number of documents the response returns
when the distinct documents the sub-queries match together outnumber the
largest single sub-query match count (from code reading, not run). For example,
two disjoint sub-queries with two hits each return four documents while
\{{numFound}} is 2 (from code reading, not run). The maximum fold is also what
the merged \{{approximateTotalHits}} passes through: in the two maximum-fold
probe cases, per-query approximate totals of 7 and 21 merged to an
\{{approximateTotalHits}} of 21 with a merged \{{numFound}} of 11, and
per-query approximate totals of 7 and 3 merged to 7 with a merged \{{numFound}}
of 5; in each case the merged approximate total equals the larger of the two
per-query approximate totals (run result for the merged values:
approximateTotalHits was read from the response header, which carries it only
when a shard sets maxHitsTerminatedEarly, and numFound was read from the
response docs; the per-query totals are code arithmetic from the probe inputs,
not recorded in the export), so the smaller one does not appear in it (the max
rule is from code reading, line 442, not run). Which count should \{{numFound}}
report: (a) the number of distinct documents that match any sub-query, or (b) a
lower bound such as the largest single sub-query count, floored at the number
of distinct documents returned? Whichever is chosen, should \{{numFoundExact}}
be false whenever more than one sub-query contributes? The answer sets the
expected numbers for the fix and decides whether the exactness fix can land
with it.
Related: SOLR-14758, SOLR-15595.Â
AI agents assisted with research, implementation, review, and drafting. Nick
Shanin directed the work and takes responsibility for the above text.
> CombinedQueryComponent.mergeIds counts a skipped partial shard that sends a
> per-query entry in the per-query totals, adds the running total as the
> approximateTotalHits fallback
> --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: SOLR-18541
> URL: https://issues.apache.org/jira/browse/SOLR-18541
> Project: Solr
> Issue Type: Bug
> Components: query
> Reporter: Nick Shanin
> Priority: Major
>
> 🤖 _AI text below_ 🤖 _(posted on behalf of Nick Shanin)_
> Combined queries merge their per-query shard results in
> {{CombinedQueryComponent.mergeIds}}, and three defects sit in the totals that
> merge reports. First, a partial shard that sends a per-query entry is counted
> in that query's totals before the partial skip is tested, so its documents
> are dropped but its hits are not: in a constructed partial-response case run
> on unmodified main, the merged {{numFound}} was 7 instead of 3 and
> {{approximateTotalHits}} was 15 instead of 5. Second, when a shard sends no
> {{approximateTotalHits}} header, the fallback adds the running per-query
> total, which already includes the current shard, so two shards with 2 and 3
> documents merged to 7 instead of 5. Third, the {{hitCountIsExact}} fold is
> inverted: an all-exact merge reports {{numFoundExact=false}} and an
> all-inexact merge reports true, both confirmed by run on unmodified main.
> The fix I recommend is to test the skip before accumulating, with the other
> two corrections in the same change: the fallback should add the shard's own
> count instead of the running total, and the fold should keep the merged flag
> true only when every shard reports its count exact. Correcting the totals
> after the skip fires instead would mean reconstructing what the skipped shard
> added, and the exactness fold cannot be un-folded without per-shard tracking.
> One question for the maintainer: when several sub-queries contribute,
> {{numFound}} is currently the largest single sub-query count; should it
> instead count the distinct documents that match any sub-query, and should
> {{numFoundExact}} be false whenever more than one sub-query contributes?
> AI agents assisted with research, implementation, review, and drafting. Nick
> Shanin directed the work and takes responsibility for the above text.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
