dsmiley commented on code in PR #4276:
URL: https://github.com/apache/solr/pull/4276#discussion_r3066846114
##########
solr/solr-ref-guide/modules/query-guide/examples/JsonRequestApiTest.java:
##########
@@ -83,6 +83,53 @@ public void testSimpleJsonQuery() throws Exception {
assertResponseFoundNumDocs(queryResponse, expectedResults);
}
+ /**
+ * Test json query behaviour in case of multiple query to be executed using
Combined Query.
+ *
+ * @throws Exception the exception
+ */
+ @Test
+ public void testSimpleJsonQueryWithQueriesParams() throws Exception {
+ SolrClient solrClient = cluster.getSolrClient();
+ final int expectedResults = 2;
+ final Map<String, Object> queriesMap = new HashMap<>();
+ queriesMap.put("query1", Map.of("lucene", Map.of("query", "apache", "df",
"manu")));
+ queriesMap.put("query2", Map.of("edismax", Map.of("query", "solr", "df",
"name")));
Review Comment:
you could inline and use `Map.of` and end up with something structurally
easy to read and less imperative.
##########
solr/server/solr/configsets/sample_techproducts_configs/conf/solrconfig.xml:
##########
Review Comment:
Mmmmm, not sure what I think about adding another handler to one of only 2
configSets we ship with Solr. But I suppose it's fine as this particular one,
techproducts, is to show off features. So sure.
CC @epugh for 2nd opinion.
##########
solr/solr-ref-guide/modules/query-guide/examples/JsonRequestApiTest.java:
##########
@@ -83,6 +83,53 @@ public void testSimpleJsonQuery() throws Exception {
assertResponseFoundNumDocs(queryResponse, expectedResults);
}
+ /**
+ * Test json query behaviour in case of multiple query to be executed using
Combined Query.
+ *
+ * @throws Exception the exception
+ */
+ @Test
+ public void testSimpleJsonQueryWithQueriesParams() throws Exception {
+ SolrClient solrClient = cluster.getSolrClient();
+ final int expectedResults = 2;
+ final Map<String, Object> queriesMap = new HashMap<>();
+ queriesMap.put("query1", Map.of("lucene", Map.of("query", "apache", "df",
"manu")));
+ queriesMap.put("query2", Map.of("edismax", Map.of("query", "solr", "df",
"name")));
+ ModifiableSolrParams queryParams = new ModifiableSolrParams();
Review Comment:
`SolrParams.of()` if we're forced to provide one (which is sad if true for
*Json* Queryrequest
##########
solr/solr-ref-guide/modules/query-guide/examples/JsonRequestApiTest.java:
##########
@@ -83,6 +83,53 @@ public void testSimpleJsonQuery() throws Exception {
assertResponseFoundNumDocs(queryResponse, expectedResults);
}
+ /**
+ * Test json query behaviour in case of multiple query to be executed using
Combined Query.
+ *
+ * @throws Exception the exception
+ */
+ @Test
+ public void testSimpleJsonQueryWithQueriesParams() throws Exception {
+ SolrClient solrClient = cluster.getSolrClient();
+ final int expectedResults = 2;
+ final Map<String, Object> queriesMap = new HashMap<>();
+ queriesMap.put("query1", Map.of("lucene", Map.of("query", "apache", "df",
"manu")));
+ queriesMap.put("query2", Map.of("edismax", Map.of("query", "solr", "df",
"name")));
+ ModifiableSolrParams queryParams = new ModifiableSolrParams();
+ queryParams.set("qt", "/search");
Review Comment:
I'd rather we generally avoid this and instead use QueryRequest.setPath
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/json/JsonQueryRequest.java:
##########
@@ -101,6 +101,30 @@ public JsonQueryRequest setQuery(Map<String, Object>
queryJson) {
return this;
}
+ /**
+ * Specify the queries parameter sent as a part of JSON request.
+ *
+ * <p>This method would be helpful in setting the queries parameter
specially for Combined Query
+ * Component {@code CombinedQueryComponent} use case.
+ *
+ * <p><b>Example:</b> You wish to send the JSON request:
+ *
+ * <pre>{@code {'limit': 5, 'queries': {'query1': {'lucene':
+ * {'df':'genre_s', 'query': 'scifi'}}}, 'query2': {'knn': {'f': 'vector',
'query': [0.1, 0.43]}}}}
+ * </pre>
+ *
+ * @param queriesJson a Map of values representing the query subtree of the
JSON request you wish
+ * to send.
+ * @throws IllegalArgumentException if {@code queriesJson} is null.
+ */
+ public JsonQueryRequest setQueries(Map<String, Object> queriesJson) {
+ if (queriesJson == null) {
+ throw new IllegalArgumentException("'queriesJson' parameter must be
non-null");
+ }
Review Comment:
I suggest we simply do nothing. The fact that we return JsonQueryRequest
(this) means this is a fluent API, which is easy to call with chaining. If we
force the caller that may or may not have the parameter to add an if condition
-- it's annoying.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
