dsmiley commented on code in PR #2489:
URL: https://github.com/apache/solr/pull/2489#discussion_r1643463278
##########
solr/core/src/java/org/apache/solr/request/json/RequestUtil.java:
##########
@@ -241,13 +243,16 @@ public static void processParams(
if (queriesJsonObj instanceof Map && queriesJsonObj != null) {
@SuppressWarnings("unchecked")
final Map<String, Object> queriesAsMap = (Map<String, Object>)
queriesJsonObj;
+ ArrayList<String> queryKeys = new ArrayList<>();
for (Map.Entry<String, Object> queryJsonProperty :
queriesAsMap.entrySet()) {
out = queryJsonProperty.getKey();
+ queryKeys.add(out);
arr = true;
isQuery = true;
convertJsonPropertyToLocalParams(
newMap, jsonQueryConverter, queryJsonProperty, out, isQuery,
arr);
}
+ newMap.put(COMBINER_KEYS, queryKeys.toArray(new String[0])); //
nocommit a hack
Review Comment:
Making RequestUtils add a param specific to a niche feature is a hack. If
we really need this here (?), I think we should use a generic name like
"json.queries".
##########
solr/core/src/java/org/apache/solr/search/combining/CombineQParserPlugin.java:
##########
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.search.combining;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+import org.apache.lucene.search.BooleanClause.Occur;
+import org.apache.lucene.search.MatchNoDocsQuery;
+import org.apache.lucene.search.Query;
+import org.apache.lucene.search.QueryVisitor;
+import org.apache.solr.common.params.CombinerParams;
+import org.apache.solr.common.params.SolrParams;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.SimpleOrderedMap;
+import org.apache.solr.handler.component.QueryComponent;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.ExtendedQueryBase;
+import org.apache.solr.search.QParser;
+import org.apache.solr.search.QParserPlugin;
+import org.apache.solr.search.QueryCommand;
+import org.apache.solr.search.QueryParsing;
+import org.apache.solr.search.QueryResult;
+import org.apache.solr.search.SolrIndexSearcher;
+import org.apache.solr.search.SyntaxError;
+
+public class CombineQParserPlugin extends QParserPlugin {
Review Comment:
This source file captures the essential aspect of my proposal. Needs
another iteration of refinement at least (e.g. javadocs, and param naming /
defaulting). Maybe more.
##########
solr/core/src/test/org/apache/solr/handler/component/DebugComponentTest.java:
##########
@@ -52,7 +64,7 @@ public void testBasicInterface() {
"//str[@name='querystring']='*:*'",
"//str[@name='parsedquery']='MatchAllDocsQuery(*:*)'",
"//str[@name='parsedquery_toString']='*:*'",
- "count(//lst[@name='explain']/*)=3",
+ "count(//lst[@name='explain']/*)=6",
Review Comment:
I suppose I'd need to see a before & after to truly "see" the change.
##########
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java:
##########
@@ -1689,10 +1689,11 @@ private void doProcessUngroupedSearch(ResponseBuilder
rb, QueryCommand cmd, Quer
SolrIndexSearcher searcher = req.getSearcher();
- try {
+ if (cmd.getQuery() instanceof SelfExecutingQuery) {
+ // TODO QueryResult ought not to be created and passed in methods of
QueryComponent
+ result = ((SelfExecutingQuery) cmd.getQuery()).search(searcher, cmd);
+ } else {
Review Comment:
Here's where the minimal essential bit to alter QueryComponent by adding a
new abstraction (SelfExecutingQuery) that might be useful for other features;
not just "combining".
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -706,9 +707,12 @@ public <K, V> boolean regenerateItem(
}
}
- public QueryResult search(QueryResult qr, QueryCommand cmd) throws
IOException {
- getDocListC(qr, cmd);
- return qr;
+ public void search(QueryResult qr, QueryCommand cmd) throws IOException {
+ try {
+ getDocListC(qr, cmd);
+ } catch (FuzzyTermsEnum.FuzzyTermsException e) { // unsure where best to
catch this; shrug
Review Comment:
I moved the FuzzyTermsException catch block from QueryComponent to here. I
wanted it in a deeper place that would be caught by multiple inward code paths
instead of just QC. It still feels it ought to be deeper but I didn't look for
a better spot. CombineQuery calls into this.
##########
solr/core/src/java/org/apache/solr/search/combining/CombineQParserPlugin.java:
##########
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.search.combining;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+import org.apache.lucene.search.BooleanClause.Occur;
+import org.apache.lucene.search.MatchNoDocsQuery;
+import org.apache.lucene.search.Query;
+import org.apache.lucene.search.QueryVisitor;
+import org.apache.solr.common.params.CombinerParams;
+import org.apache.solr.common.params.SolrParams;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.SimpleOrderedMap;
+import org.apache.solr.handler.component.QueryComponent;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.ExtendedQueryBase;
+import org.apache.solr.search.QParser;
+import org.apache.solr.search.QParserPlugin;
+import org.apache.solr.search.QueryCommand;
+import org.apache.solr.search.QueryParsing;
+import org.apache.solr.search.QueryResult;
+import org.apache.solr.search.SolrIndexSearcher;
+import org.apache.solr.search.SyntaxError;
+
+public class CombineQParserPlugin extends QParserPlugin {
+ public static final String NAME = "combine";
+
+ @Override
+ public QParser createParser(
+ String qstr, SolrParams localParams, SolrParams params, SolrQueryRequest
req) {
+ return new CombineQParser(qstr, localParams, params, req);
+ }
+
+ static class CombineQParser extends QParser {
+
+ private List<String> unparsedQueries;
+ private QueriesCombiner queriesCombiningStrategy;
+ private List<QParser> queryParsers;
+ private List<Query> queries;
+
+ public CombineQParser(
+ String qstr, SolrParams localParams, SolrParams params,
SolrQueryRequest req) {
+ super(qstr, localParams, params, req);
+ }
+
+ @Override
+ public Query parse() throws SyntaxError {
+
+ var queriesToCombineKeys = localParams.getParams("keys");
+ if (queriesToCombineKeys == null) {
+ queriesToCombineKeys = params.getParams(CombinerParams.COMBINER_KEYS);
+ }
+
+ unparsedQueries = new ArrayList<>(queriesToCombineKeys.length);
+ queryParsers = new ArrayList<>(queriesToCombineKeys.length);
+ queries = new ArrayList<>(queriesToCombineKeys.length);
+
+ // nocommit blend localParams and params without needless suffix in local
+ var blendParams = SolrParams.wrapDefaults(localParams, params);
+ queriesCombiningStrategy =
QueriesCombiner.getImplementation(blendParams);
+
+ String defType = blendParams.get(QueryParsing.DEFTYPE, DEFAULT_QTYPE);
+
+ for (String queryKey : queriesToCombineKeys) {
+ final var unparsedQuery = blendParams.get(queryKey);
+ final var parser = QParser.getParser(unparsedQuery, defType, req);
+ var query = parser.getQuery();
+ if (query == null) { // sad this can happen
+ query = new MatchNoDocsQuery();
+ }
+ unparsedQueries.add(unparsedQuery);
+ queryParsers.add(parser);
+ queries.add(query);
+ }
+
+ return new CombineQuery(queriesCombiningStrategy, queries);
+ }
+
+ @Override
+ public void addDebugInfo(NamedList<Object> dbg) {
+ super.addDebugInfo(dbg);
+
+ NamedList<NamedList<Object>> queriesDebug = new SimpleOrderedMap<>();
+ String[] queryKeys =
req.getParams().getParams(CombinerParams.COMBINER_KEYS);
+ for (int i = 0; i < queries.size(); i++) {
+ NamedList<Object> singleQueryDebug = new SimpleOrderedMap<>();
+ singleQueryDebug.add("querystring", unparsedQueries.get(i));
+ singleQueryDebug.add("queryparser",
queryParsers.get(i).getClass().getSimpleName());
+ singleQueryDebug.add("parsedquery",
QueryParsing.toString(queries.get(i), req.getSchema()));
+ singleQueryDebug.add("parsedquery_toString",
queries.get(i).toString());
+ queriesDebug.add(queryKeys[i], singleQueryDebug);
+ }
+ dbg.add("queriesToCombine", queriesDebug);
+ }
+ }
+
+ static final class CombineQuery extends ExtendedQueryBase
+ implements QueryComponent.SelfExecutingQuery {
+
+ private final QueriesCombiner combiner;
+ private final List<Query> queries;
+
+ public CombineQuery(QueriesCombiner combiner, List<Query> queries) {
+ this.combiner = combiner;
+ this.queries = queries;
+ }
+
+ @Override
+ public String toString(String field) {
+ return super.toString(field) + "{!" + NAME + "}"; // TODO others
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) return true;
+ if (!(o instanceof CombineQuery)) return false;
+ CombineQuery that = (CombineQuery) o;
+ return Objects.equals(combiner, that.combiner) &&
Objects.equals(queries, that.queries);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(combiner, queries);
+ }
+
+ @Override
+ public void visit(QueryVisitor visitor) {
+ for (Query query : queries) {
+ query.visit(visitor.getSubVisitor(Occur.MUST, this));
+ }
+ }
+
+ @Override
+ public QueryResult search(SolrIndexSearcher searcher, QueryCommand cmd)
throws IOException {
+ QueryResult[] results = new QueryResult[queries.size()];
+ // TODO do in multiple threads?
+ for (int i = 0; i < queries.size(); i++) {
+ cmd.setQuery(queries.get(i));
+ QueryResult qr = new QueryResult();
+ searcher.search(qr, cmd);
+ results[i] = qr;
+ }
+
+ // nocommit but how is the docSet (e.g. for faceting) or maybe other
things supported?
Review Comment:
This seems like a fundamental limitation in the whole feature honestly.
What Hossman said about re-rank query is interesting... not sure if it
addresses this.
##########
solr/core/src/java/org/apache/solr/search/combining/CombineQParserPlugin.java:
##########
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.search.combining;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+import org.apache.lucene.search.BooleanClause.Occur;
+import org.apache.lucene.search.MatchNoDocsQuery;
+import org.apache.lucene.search.Query;
+import org.apache.lucene.search.QueryVisitor;
+import org.apache.solr.common.params.CombinerParams;
+import org.apache.solr.common.params.SolrParams;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.SimpleOrderedMap;
+import org.apache.solr.handler.component.QueryComponent;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.ExtendedQueryBase;
+import org.apache.solr.search.QParser;
+import org.apache.solr.search.QParserPlugin;
+import org.apache.solr.search.QueryCommand;
+import org.apache.solr.search.QueryParsing;
+import org.apache.solr.search.QueryResult;
+import org.apache.solr.search.SolrIndexSearcher;
+import org.apache.solr.search.SyntaxError;
+
+public class CombineQParserPlugin extends QParserPlugin {
+ public static final String NAME = "combine";
+
+ @Override
+ public QParser createParser(
+ String qstr, SolrParams localParams, SolrParams params, SolrQueryRequest
req) {
+ return new CombineQParser(qstr, localParams, params, req);
+ }
+
+ static class CombineQParser extends QParser {
+
+ private List<String> unparsedQueries;
+ private QueriesCombiner queriesCombiningStrategy;
+ private List<QParser> queryParsers;
+ private List<Query> queries;
+
+ public CombineQParser(
+ String qstr, SolrParams localParams, SolrParams params,
SolrQueryRequest req) {
+ super(qstr, localParams, params, req);
+ }
+
+ @Override
+ public Query parse() throws SyntaxError {
+
+ var queriesToCombineKeys = localParams.getParams("keys");
+ if (queriesToCombineKeys == null) {
+ queriesToCombineKeys = params.getParams(CombinerParams.COMBINER_KEYS);
+ }
+
+ unparsedQueries = new ArrayList<>(queriesToCombineKeys.length);
+ queryParsers = new ArrayList<>(queriesToCombineKeys.length);
+ queries = new ArrayList<>(queriesToCombineKeys.length);
+
+ // nocommit blend localParams and params without needless suffix in local
+ var blendParams = SolrParams.wrapDefaults(localParams, params);
+ queriesCombiningStrategy =
QueriesCombiner.getImplementation(blendParams);
+
+ String defType = blendParams.get(QueryParsing.DEFTYPE, DEFAULT_QTYPE);
+
+ for (String queryKey : queriesToCombineKeys) {
+ final var unparsedQuery = blendParams.get(queryKey);
+ final var parser = QParser.getParser(unparsedQuery, defType, req);
+ var query = parser.getQuery();
+ if (query == null) { // sad this can happen
+ query = new MatchNoDocsQuery();
+ }
+ unparsedQueries.add(unparsedQuery);
+ queryParsers.add(parser);
+ queries.add(query);
+ }
+
+ return new CombineQuery(queriesCombiningStrategy, queries);
+ }
+
+ @Override
+ public void addDebugInfo(NamedList<Object> dbg) {
+ super.addDebugInfo(dbg);
+
+ NamedList<NamedList<Object>> queriesDebug = new SimpleOrderedMap<>();
+ String[] queryKeys =
req.getParams().getParams(CombinerParams.COMBINER_KEYS);
+ for (int i = 0; i < queries.size(); i++) {
+ NamedList<Object> singleQueryDebug = new SimpleOrderedMap<>();
+ singleQueryDebug.add("querystring", unparsedQueries.get(i));
+ singleQueryDebug.add("queryparser",
queryParsers.get(i).getClass().getSimpleName());
+ singleQueryDebug.add("parsedquery",
QueryParsing.toString(queries.get(i), req.getSchema()));
+ singleQueryDebug.add("parsedquery_toString",
queries.get(i).toString());
+ queriesDebug.add(queryKeys[i], singleQueryDebug);
+ }
+ dbg.add("queriesToCombine", queriesDebug);
+ }
+ }
+
+ static final class CombineQuery extends ExtendedQueryBase
Review Comment:
A "SelfExecutingQuery" isn't normal; Lucene isn't going to work with this
thing since it doesn't implement getWeight nor can it. But users don't need to
know/care.
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
