You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "dsmiley (via GitHub)" <gi...@apache.org> on 2024/06/18 12:16:29 UTC
[PR] Refactor: QueryResult: don't provide to SolrIndexSearcher [solr]
dsmiley opened a new pull request, #2524:
URL: https://github.com/apache/solr/pull/2524
IMO it's wrong for SolrIndexSearcher.search(QueryCommand) to also take a QueryResult param and not return it. It's an "out" parameter, but there is no need for this! This PR deprecates that method and replaces it with one that does the right thing. In one spot, I see QueryResult early segment termination being set up in QueryComponent but that should be done in SolrIndexSearcher.
I also deprecated many low-value SolrIndexSearcher methods involving QueryCommand; a QueryCommand is a superior API to methods taking lots of parameters. Some weren't even called. Some were in a place or two. I inlined some of those albeit not all. Some were called by SolrPluginUtils methods that themselves felt like questionable utility methods so I deprecated them too; they are used by DebugComponent and MLTHandler but IMO if they need to exist, DebugComponent would be the best home as it's the principal place for debut output to be.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Refactor: QueryResult: don't provide to SolrIndexSearcher [solr]
Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on code in PR #2524:
URL: https://github.com/apache/solr/pull/2524#discussion_r1644863482
##########
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java:
##########
@@ -421,22 +416,22 @@ public void process(ResponseBuilder rb) throws IOException {
cmd.setSegmentTerminateEarly(false);
try {
if (params.getBool(GroupParams.GROUP_DISTRIBUTED_FIRST, false)) {
- doProcessGroupedDistributedSearchFirstPhase(rb, cmd, result);
+ doProcessGroupedDistributedSearchFirstPhase(rb, cmd);
return;
} else if (params.getBool(GroupParams.GROUP_DISTRIBUTED_SECOND, false)) {
- doProcessGroupedDistributedSearchSecondPhase(rb, cmd, result);
+ doProcessGroupedDistributedSearchSecondPhase(rb, cmd);
return;
}
- doProcessGroupedSearch(rb, cmd, result);
+ doProcessGroupedSearch(rb, cmd);
Review Comment:
subjective/style: if/since we're changing this code area, how about adding a `else { }` wrap and then removing the two `return;` in the first and second phases?
```suggestion
else {
doProcessGroupedSearch(rb, cmd);
}
```
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Refactor: QueryResult: don't provide to SolrIndexSearcher [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley merged PR #2524:
URL: https://github.com/apache/solr/pull/2524
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Refactor: QueryResult: don't provide to SolrIndexSearcher [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2524:
URL: https://github.com/apache/solr/pull/2524#discussion_r1654698142
##########
solr/core/src/test/org/apache/solr/search/TestSearchPerf.java:
##########
@@ -173,7 +172,14 @@ private long doListGen(Query q, List<Query> filt) throws Exception {
long ret = 0;
for (int i = 0; i < ITERATIONS; i++) {
DocList l =
- searcher.getDocList(q, filt, (Sort) null, 0, 10, SolrIndexSearcher.NO_CHECK_QCACHE);
+ new QueryCommand()
+ .setQuery(q)
+ .setFilterList(filt)
+ .setOffset(0)
Review Comment:
The default is -1 and there are no safeguards around interpreting that so I can't remove this line. I think the default *should* be 0 but it isn't :-(
##########
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java:
##########
@@ -403,14 +403,9 @@ public void process(ResponseBuilder rb) throws IOException {
req.getContext().put(SolrIndexSearcher.STATS_SOURCE, statsCache.get(req));
- QueryResult result = new QueryResult();
-
cmd.setSegmentTerminateEarly(
params.getBool(
CommonParams.SEGMENT_TERMINATE_EARLY, CommonParams.SEGMENT_TERMINATE_EARLY_DEFAULT));
- if (cmd.getSegmentTerminateEarly()) {
- result.setSegmentTerminatedEarly(Boolean.FALSE);
- }
Review Comment:
Do you mean for grouping, which apparently doesn't support early termination?
##########
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java:
##########
@@ -421,22 +416,22 @@ public void process(ResponseBuilder rb) throws IOException {
cmd.setSegmentTerminateEarly(false);
try {
if (params.getBool(GroupParams.GROUP_DISTRIBUTED_FIRST, false)) {
- doProcessGroupedDistributedSearchFirstPhase(rb, cmd, result);
+ doProcessGroupedDistributedSearchFirstPhase(rb, cmd);
return;
} else if (params.getBool(GroupParams.GROUP_DISTRIBUTED_SECOND, false)) {
- doProcessGroupedDistributedSearchSecondPhase(rb, cmd, result);
+ doProcessGroupedDistributedSearchSecondPhase(rb, cmd);
return;
}
- doProcessGroupedSearch(rb, cmd, result);
+ doProcessGroupedSearch(rb, cmd);
Review Comment:
I agree, and I think we should do more than that. One "return" statement for the whole grouping section; no need to have just for GROUP_DISTRIBUTED_FIRST and SECOND.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Refactor: QueryResult: don't provide to SolrIndexSearcher [solr]
Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on code in PR #2524:
URL: https://github.com/apache/solr/pull/2524#discussion_r1654776265
##########
solr/core/src/test/org/apache/solr/search/TestSearchPerf.java:
##########
@@ -173,7 +172,14 @@ private long doListGen(Query q, List<Query> filt) throws Exception {
long ret = 0;
for (int i = 0; i < ITERATIONS; i++) {
DocList l =
- searcher.getDocList(q, filt, (Sort) null, 0, 10, SolrIndexSearcher.NO_CHECK_QCACHE);
+ new QueryCommand()
+ .setQuery(q)
+ .setFilterList(filt)
+ .setOffset(0)
Review Comment:
Hmm, I think the default is zero, no?
https://github.com/apache/solr/blob/releases/solr/9.6.1/solr/core/src/java/org/apache/solr/search/QueryCommand.java#L36
##########
solr/core/src/java/org/apache/solr/handler/MoreLikeThisHandler.java:
##########
@@ -161,13 +162,16 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
mltDocs = mlt.getMoreLikeThis(reader, start, rows, filters, flags);
} else if (q != null) {
// Matching options
Review Comment:
```suggestion
```
##########
solr/core/src/java/org/apache/solr/handler/MoreLikeThisHandler.java:
##########
@@ -161,13 +162,16 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
mltDocs = mlt.getMoreLikeThis(reader, start, rows, filters, flags);
} else if (q != null) {
// Matching options
- boolean includeMatch = params.getBool(MoreLikeThisParams.MATCH_INCLUDE, true);
- int matchOffset = params.getInt(MoreLikeThisParams.MATCH_OFFSET, 0);
- // Find the base match
DocList match =
- searcher.getDocList(
- query, null, null, matchOffset, 1, flags); // only get the first one...
- if (includeMatch) {
+ new QueryCommand()
+ .setQuery(query)
+ .setOffset(params.getInt(MoreLikeThisParams.MATCH_OFFSET, 0))
+ .setLen(1) // only get the first one...
+ .setFlags(flags)
+ .search(searcher)
+ .getDocList();
+ // Find the base match
Review Comment:
```suggestion
```
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Refactor: QueryResult: don't provide to SolrIndexSearcher [solr]
Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on code in PR #2524:
URL: https://github.com/apache/solr/pull/2524#discussion_r1644850887
##########
solr/core/src/test/org/apache/solr/search/TestSearchPerf.java:
##########
@@ -173,7 +172,14 @@ private long doListGen(Query q, List<Query> filt) throws Exception {
long ret = 0;
for (int i = 0; i < ITERATIONS; i++) {
DocList l =
- searcher.getDocList(q, filt, (Sort) null, 0, 10, SolrIndexSearcher.NO_CHECK_QCACHE);
+ new QueryCommand()
+ .setQuery(q)
+ .setFilterList(filt)
+ .setOffset(0)
Review Comment:
```suggestion
```
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Refactor: QueryResult: don't provide to SolrIndexSearcher [solr]
Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on code in PR #2524:
URL: https://github.com/apache/solr/pull/2524#discussion_r1644852623
##########
solr/core/src/java/org/apache/solr/handler/MoreLikeThisHandler.java:
##########
@@ -165,8 +166,12 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
int matchOffset = params.getInt(MoreLikeThisParams.MATCH_OFFSET, 0);
// Find the base match
DocList match =
- searcher.getDocList(
- query, null, null, matchOffset, 1, flags); // only get the first one...
+ new QueryCommand()
+ .setQuery(query)
+ .setOffset(matchOffset)
+ .setLen(1) // only get the first one...
+ .setFlags(flags)
+ .search(searcher).getDocList();
Review Comment:
precommit
```suggestion
.search(searcher)
.getDocList();
```
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Refactor: QueryResult: don't provide to SolrIndexSearcher [solr]
Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on code in PR #2524:
URL: https://github.com/apache/solr/pull/2524#discussion_r1644860246
##########
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java:
##########
@@ -403,14 +403,9 @@ public void process(ResponseBuilder rb) throws IOException {
req.getContext().put(SolrIndexSearcher.STATS_SOURCE, statsCache.get(req));
- QueryResult result = new QueryResult();
-
cmd.setSegmentTerminateEarly(
params.getBool(
CommonParams.SEGMENT_TERMINATE_EARLY, CommonParams.SEGMENT_TERMINATE_EARLY_DEFAULT));
- if (cmd.getSegmentTerminateEarly()) {
- result.setSegmentTerminatedEarly(Boolean.FALSE);
- }
Review Comment:
observation: deferring this bit also helps with the `// not supported, silently ignore any segmentTerminateEarly flag` bit at lines 420/421 415/416 below i.e. if the results have `Boolean.FALSE` in future then it would not be despite the silent ignoring.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Refactor: QueryResult: don't provide to SolrIndexSearcher [solr]
Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on code in PR #2524:
URL: https://github.com/apache/solr/pull/2524#discussion_r1729272808
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -716,6 +716,12 @@ public <K, V> boolean regenerateItem(
}
}
+ /** Primary entrypoint for searching, using a {@link QueryCommand}. */
+ public QueryResult search(QueryCommand cmd) throws IOException {
+ return search(new QueryResult(), cmd);
+ }
+
+ @Deprecated
Review Comment:
#2662 to remove use of this API variant
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Refactor: QueryResult: don't provide to SolrIndexSearcher [solr]
Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on PR #2524:
URL: https://github.com/apache/solr/pull/2524#issuecomment-2197018871
> (no CHANGES.txt; just deprecating low level stuff & refactoring stuff that shouldn't have an effect)
>
> WDYT @cpoerschke
Perhaps `QueryComponent` and `SolrIndexSearcher` might not be totally low level stuff here, i.e. folks with custom components (or just a copy/paste/adapt 'fork' of QueryComponent) might call the APIs being deprecated.
Therefore it could be nice to "Other Changes" mention about the deprecation before it goes away in Solr 10 or so. The `@deprecated` annotation serves the same purpose but only slightly so i.e. they might only get noticed when users try to upgrade whereas a mention in the CHANGES.txt might trigger an earlier realisation of "hey, we need to do some prep work or keep this in mind when upgrading our custom query component in future" etc.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Refactor: QueryResult: don't provide to SolrIndexSearcher [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2524:
URL: https://github.com/apache/solr/pull/2524#issuecomment-2197732565
oh the irony; a CHANGES.txt conflict; sigh
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org