This is an automated email from the ASF dual-hosted git repository.
epugh pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/solr-mcp.git
The following commit(s) were added to refs/heads/main by this push:
new 3adac6e9 perf(search): request score and keep Solr's document and
facet order (#234)
3adac6e9 is described below
commit 3adac6e9bec7087df3072f8c8ce3d9a5b0d85400
Author: Aditya Parikh <[email protected]>
AuthorDate: Tue Oct 6 14:41:33 2026 -0400
perf(search): request score and keep Solr's document and facet order (#234)
* perf(search): request score and keep Solr's document and facet order
search never asked Solr for the relevance score, so maxScore was always
null and no document carried a score, although SearchResponse and the
search-collection prompt both describe them. The query now sets
fl=score,* (setIncludeScore); the tool takes no field list, so nothing
caller-supplied is overridden.
getDocs copied every SolrDocument into a new HashMap, an extra copy per
hit that also scrambled field order. SolrDocument already implements
Map<String, Object> over a LinkedHashMap, so the documents are returned
as they are.
getFacets built its buckets in HashMaps, discarding the count-sorted
order Solr returns before it reached the caller. It now uses
LinkedHashMap.
JsonResponseParser's javadoc said JSON decimals become Double; they
become Float, deliberately, because SolrJ response classes cast to it
(SchemaResponse reads the schema version as a Float). The javadoc now
says so.
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Signed-off-by: Aditya Parikh <[email protected]>
* refactor(search): return an unmodifiable view of the documents, not a copy
SolrDocument is already a Map<String, Object>, so the result list is
exposed through Collections.unmodifiableList instead of copied, and
the getDocs helper goes.
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Signed-off-by: Aditya Parikh <[email protected]>
---------
Signed-off-by: Aditya Parikh <[email protected]>
Co-authored-by: Claude Opus 5.5 (1M context) <[email protected]>
---
.../solr/mcp/server/config/JsonResponseParser.java | 4 +-
.../solr/mcp/server/search/SearchService.java | 67 +++++-----------------
.../search/SearchServiceIntegrationTest.java | 19 ++++++
.../solr/mcp/server/search/SearchServiceTest.java | 67 ++++++++++++++++++++++
4 files changed, 102 insertions(+), 55 deletions(-)
diff --git
a/src/main/java/org/apache/solr/mcp/server/config/JsonResponseParser.java
b/src/main/java/org/apache/solr/mcp/server/config/JsonResponseParser.java
index dec1dcab..a2a005db 100644
--- a/src/main/java/org/apache/solr/mcp/server/config/JsonResponseParser.java
+++ b/src/main/java/org/apache/solr/mcp/server/config/JsonResponseParser.java
@@ -54,7 +54,9 @@ import org.springframework.http.MediaType;
* {@link NamedList} (Solr's {@code json.nl=flat} facet encoding)</li>
* <li>All other JSON arrays → {@link List}</li>
* <li>JSON integers → {@link Integer} or {@link Long} (by value size)</li>
- * <li>JSON decimals → {@link Double}</li>
+ * <li>JSON decimals → {@link Float}, matching the JavaBin codec; SolrJ
response
+ * classes cast to it (e.g. {@code SchemaResponse} reads the schema version as
a
+ * {@code Float})</li>
* <li>JSON booleans → {@link Boolean}</li>
* <li>JSON strings → {@link String}</li>
* </ul>
diff --git a/src/main/java/org/apache/solr/mcp/server/search/SearchService.java
b/src/main/java/org/apache/solr/mcp/server/search/SearchService.java
index c9dda135..ac1b2f37 100644
--- a/src/main/java/org/apache/solr/mcp/server/search/SearchService.java
+++ b/src/main/java/org/apache/solr/mcp/server/search/SearchService.java
@@ -18,8 +18,8 @@ package org.apache.solr.mcp.server.search;
import io.micrometer.observation.annotation.Observed;
import java.io.IOException;
-import java.util.ArrayList;
-import java.util.HashMap;
+import java.util.Collections;
+import java.util.LinkedHashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
@@ -28,7 +28,6 @@ import org.apache.solr.client.solrj.SolrServerException;
import org.apache.solr.client.solrj.request.SolrQuery;
import org.apache.solr.client.solrj.response.FacetField;
import org.apache.solr.client.solrj.response.QueryResponse;
-import org.apache.solr.common.SolrDocument;
import org.apache.solr.common.SolrDocumentList;
import org.apache.solr.common.SolrException;
import org.apache.solr.common.params.FacetParams;
@@ -161,64 +160,19 @@ public class SearchService {
this.solrClient = solrClient;
}
- /**
- * Converts a SolrDocumentList to a List of Maps for optimized JSON
- * serialization.
- *
- * <p>
- * This method transforms Solr's native document format into a
structure that
- * can be easily serialized to JSON and consumed by MCP clients. Each
document
- * becomes a flat map of field names to field values, preserving all
data types.
- *
- * <p>
- * <strong>Conversion Process:</strong>
- *
- * <ul>
- * <li>Iterates through each SolrDocument in the list
- * <li>Extracts all field names and their corresponding values
- * <li>Creates a HashMap for each document with field-value pairs
- * <li>Preserves original data types (strings, numbers, dates, arrays)
- * </ul>
- *
- * <p>
- * <strong>Performance Optimization:</strong>
- *
- * <p>
- * Pre-allocates the ArrayList with the known document count to
minimize memory
- * allocations and improve conversion performance for large result sets.
- *
- * @param documents
- * the SolrDocumentList to convert from Solr's native format
- * @return a List of Maps where each Map represents a document with
field names
- * as keys
- * @see SolrDocument
- * @see SolrDocumentList
- */
- private static List<Map<String, Object>> getDocs(SolrDocumentList
documents) {
- List<Map<String, Object>> docs = new
ArrayList<>(documents.size());
- documents.forEach(doc -> {
- Map<String, Object> docMap = new HashMap<>();
- for (String fieldName : doc.getFieldNames()) {
- docMap.put(fieldName,
doc.getFieldValue(fieldName));
- }
- docs.add(docMap);
- });
- return docs;
- }
-
/**
* Extracts facet information from a QueryResponse.
*
* @param queryResponse
* The QueryResponse containing facet results
* @return A Map where keys are facet field names and values are Maps
of facet
- * values to counts
+ * values to counts, both in Solr's original (count-sorted)
order
*/
private static Map<String, Map<String, Long>> getFacets(QueryResponse
queryResponse) {
- Map<String, Map<String, Long>> facets = new HashMap<>();
+ Map<String, Map<String, Long>> facets = new LinkedHashMap<>();
if (queryResponse.getFacetFields() != null &&
!queryResponse.getFacetFields().isEmpty()) {
queryResponse.getFacetFields().forEach(facetField -> {
- Map<String, Long> facetValues = new HashMap<>();
+ Map<String, Long> facetValues = new
LinkedHashMap<>();
for (FacetField.Count count :
facetField.getValues()) {
facetValues.put(count.getName(),
count.getCount());
}
@@ -313,6 +267,9 @@ public class SearchService {
solrQuery.setQuery(query);
}
+ // fl=score,* — Solr only returns maxScore and per-document
score when asked
+ solrQuery.setIncludeScore(true);
+
// filter queries
if (!CollectionUtils.isEmpty(filterQueries)) {
solrQuery.setFilterQueries(filterQueries.toArray(new
String[0]));
@@ -351,7 +308,8 @@ public class SearchService {
final SolrDocumentList documents = queryResponse.getResults();
// Convert SolrDocuments to Maps
- final var docs = getDocs(documents);
+ // SolrDocument is a Map in Solr's field order, so no
per-document copy
+ final List<Map<String, Object>> docs =
Collections.unmodifiableList(documents);
// Add facets if present
final var facets = getFacets(queryResponse);
@@ -464,8 +422,9 @@ public class SearchService {
* Many results: add a
`filterQueries` constraint to narrow, or pass
`facetFields` on a relevant
`string`/`strings` field to surface the distribution
and pick a sharper filter.
- - Inspect `documents` for the actual
content. The response includes `maxScore` when
- the query is not `*:*`; use it as a
relative confidence signal across queries.
+ - Inspect `documents` for the actual
content. Each document carries a relevance
+ `score`, and the response's `maxScore` is
the top score among them; both are most
+ meaningful as a relative confidence signal
for non-`*:*` queries.
5. Summarize.
- Answer the user's question grounded in the
documents found, citing concrete field
diff --git
a/src/test/java/org/apache/solr/mcp/server/search/SearchServiceIntegrationTest.java
b/src/test/java/org/apache/solr/mcp/server/search/SearchServiceIntegrationTest.java
index c7f7d7c0..55c1aabf 100644
---
a/src/test/java/org/apache/solr/mcp/server/search/SearchServiceIntegrationTest.java
+++
b/src/test/java/org/apache/solr/mcp/server/search/SearchServiceIntegrationTest.java
@@ -297,6 +297,25 @@ class SearchServiceIntegrationTest {
assertFalse(e.getMessage().contains("Hint:"), () -> "expected
no hint, got: " + e.getMessage());
}
+ /**
+ * The search tool always requests the relevance score (fl=score,*), so
a
+ * non-{@code *:*} query — which actually produces a meaningful ranking
— must
+ * come back with a non-null {@code maxScore} and a {@code score} on
every
+ * document. Both were silently dropped before this fix: Solr only
populates
+ * them when {@code score} is in the field list, which this service
never set.
+ */
+ @Test
+ void testSearchWithQueryPopulatesMaxScoreAndDocumentScore() throws
SolrServerException, IOException {
+ SearchResponse result = searchService.search(COLLECTION_NAME,
"genre_s:fantasy", null, null, null, null, null);
+ assertNotNull(result);
+ assertNotNull(result.maxScore(), "maxScore must be populated
for a non-*:* query");
+ assertTrue(result.maxScore() > 0, "maxScore should be a
positive relevance score");
+ assertFalse(result.documents().isEmpty());
+ for (Map<String, Object> book : result.documents()) {
+ assertNotNull(book.get("score"), "each document should
carry its relevance score");
+ }
+ }
+
@Test
void testSearchWithQuery() throws SolrServerException, IOException {
SearchResponse result = searchService.search(COLLECTION_NAME,
"name:\"Game of Thrones\"", null, null, null,
diff --git
a/src/test/java/org/apache/solr/mcp/server/search/SearchServiceTest.java
b/src/test/java/org/apache/solr/mcp/server/search/SearchServiceTest.java
index ed80b17b..28f10856 100644
--- a/src/test/java/org/apache/solr/mcp/server/search/SearchServiceTest.java
+++ b/src/test/java/org/apache/solr/mcp/server/search/SearchServiceTest.java
@@ -130,6 +130,30 @@ class SearchServiceTest {
assertNotNull(result);
}
+ /**
+ * The score must be requested on every search so Solr populates {@code
score}
+ * on each document and {@code maxScore} on the response — otherwise
both stay
+ * {@code null}/absent even though {@link SearchResponse}'s javadoc and
the
+ * search-collection prompt promise them.
+ */
+ @Test
+ void search_ShouldRequestScoreInFieldList() throws Exception {
+ SolrClient mockClient = mock(SolrClient.class);
+ QueryResponse mockResponse = mock(QueryResponse.class);
+ SolrDocumentList mockDocuments = createMockDocumentList();
+ when(mockResponse.getResults()).thenReturn(mockDocuments);
+ when(mockResponse.getFacetFields()).thenReturn(null);
+ when(mockClient.query(eq("test_collection"),
any(SolrQuery.class))).thenAnswer(invocation -> {
+ SolrQuery q = invocation.getArgument(1);
+
assertTrue(List.of(q.getFields().split(",")).contains("score"),
+ () -> "expected fl to request score,
got: " + q.getFields());
+ return mockResponse;
+ });
+ SearchService localService = new SearchService(mockClient);
+ SearchResponse result = localService.search("test_collection",
"name:foo", null, null, null, null, null);
+ assertNotNull(result);
+ }
+
@Test
void search_WithCustomQuery_ShouldUseProvidedQuery() throws Exception {
SolrClient mockClient = mock(SolrClient.class);
@@ -334,6 +358,49 @@ class SearchServiceTest {
assertFalse(result.facets().isEmpty());
}
+ /**
+ * Documents come back as the raw {@link SolrDocument} (which implements
+ * {@code Map<String, Object>}, backed by a {@link
java.util.LinkedHashMap}), so
+ * field order must survive unchanged rather than being scrambled by a
+ * {@link java.util.HashMap} copy.
+ */
+ @Test
+ void searchResponse_ShouldPreserveDocumentFieldOrder() throws Exception
{
+ SolrClient mockClient = mock(SolrClient.class);
+ QueryResponse mockResponse = mock(QueryResponse.class);
+ SolrDocumentList mockDocuments =
createMockDocumentListWithData();
+ when(mockResponse.getResults()).thenReturn(mockDocuments);
+ when(mockResponse.getFacetFields()).thenReturn(null);
+ when(mockClient.query(eq("test_collection"),
any(SolrQuery.class))).thenReturn(mockResponse);
+ SearchService localService = new SearchService(mockClient);
+ SearchResponse result = localService.search("test_collection",
null, null, null, null, null, null);
+ var firstDoc = result.documents().getFirst();
+ assertEquals(List.of("id", "name", "author_ss", "price",
"genre_s"), List.copyOf(firstDoc.keySet()),
+ "document field order must match Solr's
original order");
+ }
+
+ /**
+ * Solr already sorts facet buckets by count ({@code facet.sort=count},
the
+ * default this service sets); a {@link java.util.HashMap} would
discard that
+ * order before the LLM ever sees it.
+ */
+ @Test
+ void searchResponse_ShouldPreserveFacetOrder() throws Exception {
+ SolrClient mockClient = mock(SolrClient.class);
+ QueryResponse mockResponse = mock(QueryResponse.class);
+ SolrDocumentList mockDocuments = createMockDocumentList();
+ when(mockResponse.getResults()).thenReturn(mockDocuments);
+
when(mockResponse.getFacetFields()).thenReturn(createMockFacetFields());
+ when(mockClient.query(eq("test_collection"),
any(SolrQuery.class))).thenReturn(mockResponse);
+ SearchService localService = new SearchService(mockClient);
+ SearchResponse result = localService.search("test_collection",
null, null, List.of("genre_s", "author_ss"),
+ null, null, null);
+ assertEquals(List.of("genre_s", "author_ss"),
List.copyOf(result.facets().keySet()),
+ "facet field order must match Solr's response
order");
+ assertEquals(List.of("technology", "fiction"),
List.copyOf(result.facets().get("genre_s").keySet()),
+ "facet bucket order (count-sorted by Solr) must
be preserved");
+ }
+
private SolrDocumentList createMockDocumentList() {
SolrDocumentList documents = new SolrDocumentList();
documents.setNumFound(0);
