You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/03/26 04:38:47 UTC
[GitHub] [lucene-solr] dsmiley opened a new pull request #1381: SOLR-14364:
LTR SolrFeature fq improvements
dsmiley opened a new pull request #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381
https://issues.apache.org/jira/browse/SOLR-14364
Admittedly there is no test change. Again my primary motivation is really just for this class to not call `SolrIndexSearcher.getProcessedFilter` and otherwise behave as it did before.
CC @cpoerschke
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request
#1381: SOLR-14364: LTR SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381#discussion_r399415869
##########
File path: solr/core/src/java/org/apache/solr/search/grouping/CommandHandler.java
##########
@@ -228,12 +226,8 @@ private void searchWithTimeLimiter(Query query,
collector = MultiCollector.wrap(collector, hitCountCollector);
}
- if (filter.filter != null) {
- query = new BooleanQuery.Builder()
- .add(query, Occur.MUST)
- .add(filter.filter, Occur.FILTER)
- .build();
- }
+ query = QueryUtils.combineQueryAndFilter(query, filter.filter);
Review comment:
observation: if `query` were null (which presumably it wouldn't be but haven't looked) then `QueryUtils.combineQueryAndFilter` handles that differently now compared to existing logic
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on issue #1381: SOLR-14364: LTR
SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
dsmiley commented on issue #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381#issuecomment-608810732
Thanks for the code review Christine!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request
#1381: SOLR-14364: LTR SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381#discussion_r401059139
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/SolrFeature.java
##########
@@ -120,67 +123,66 @@ protected void validate() throws FeatureException {
": Q or FQ must be provided");
}
}
+
/**
* Weight for a SolrFeature
**/
public class SolrFeatureWeight extends FeatureWeight {
- final private Weight solrQueryWeight;
- final private Query query;
- final private List<Query> queryAndFilters;
+ private final Weight solrQueryWeight;
- public SolrFeatureWeight(IndexSearcher searcher,
- SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) throws IOException {
+ public SolrFeatureWeight(SolrIndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String, String[]> efi) throws IOException {
super(SolrFeature.this, searcher, request, originalQuery, efi);
try {
- String solrQuery = q;
- final List<String> fqs = fq;
-
- if ((solrQuery == null) || solrQuery.isEmpty()) {
- solrQuery = "*:*";
- }
-
- solrQuery = macroExpander.expand(solrQuery);
- if (solrQuery == null) {
- throw new FeatureException(this.getClass().getSimpleName()+" requires efi parameter that was not passed in request.");
- }
-
- final SolrQueryRequest req = makeRequest(request.getCore(), solrQuery,
- fqs, df);
+ final SolrQueryRequest req = makeRequest(request.getCore(), q, fq, df);
if (req == null) {
throw new IOException("ERROR: No parameters provided");
}
+ // Build the scoring query
+ Query scoreQuery;
+ String qStr = q;
Review comment:
observation: i like how use of `qStr` here hints towards String type which is clearer to read than the prior `solrQuery` being String type but namewise hinting towards Query type
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request
#1381: SOLR-14364: LTR SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381#discussion_r402340272
##########
File path: solr/core/src/java/org/apache/solr/search/grouping/CommandHandler.java
##########
@@ -228,12 +226,8 @@ private void searchWithTimeLimiter(Query query,
collector = MultiCollector.wrap(collector, hitCountCollector);
}
- if (filter.filter != null) {
- query = new BooleanQuery.Builder()
- .add(query, Occur.MUST)
- .add(filter.filter, Occur.FILTER)
- .build();
- }
+ query = QueryUtils.combineQueryAndFilter(query, filter.filter);
Review comment:
Okay, returning to this I think I now understand the `QueryUtils.combineQueryAndFilter` scenarios better:
* There are two inputs and zero, two or one of them could be null.
* scoreQuery non-null, filterQuery null:
* (both old and new code) just use `scoreQuery`.
* scoreQuery non-null, filterQuery non-null:
* `BooleanQuery.Builder` is used to combine the two (in both old and new code).
* scoreQuery null, filterQuery null:
* In the old code in CommandHandler/SolrIndexSearcher/Grouping/ExpandComponent a value of null would have been used for `scoreQuery`, searching on that isn't gonna end well, is it.
* In the new code `MatchAllDocsQuery` will be used to return everything.
* scoreQuery null, filterQuery non-null:
* In the old code `BooleanQuery.Builder` is used to combine the two, regardless of `scoreQuery` being null and so that search is not going to end well too.
* In the new code the `filterQuery` wrapped up into a `ConstantScoreQuery` is used to search. In terms of search meaning this is equivalent to the `scoreQuery` being a `MatchAllDocsQuery` i.e. with a score query matching everything only the filter query needs to be searched with.
So in the "scoreQuery null" scenario the new code
* no longer results in a failed search,
* but instead returns something,
* and what is returned is consistent w.r.t. any filter query,
* and/or consistent within itself i.e. a null (score or filter) query is taken to mean 'everything' i.e. `MatchAllDocsQuery`.
So yes, I agree, `MatchAllDocsQuery` is the logical choice.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request
#1381: SOLR-14364: LTR SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381#discussion_r401061433
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/SolrFeature.java
##########
@@ -120,67 +123,66 @@ protected void validate() throws FeatureException {
": Q or FQ must be provided");
}
}
+
/**
* Weight for a SolrFeature
**/
public class SolrFeatureWeight extends FeatureWeight {
- final private Weight solrQueryWeight;
- final private Query query;
- final private List<Query> queryAndFilters;
+ private final Weight solrQueryWeight;
- public SolrFeatureWeight(IndexSearcher searcher,
- SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) throws IOException {
+ public SolrFeatureWeight(SolrIndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String, String[]> efi) throws IOException {
super(SolrFeature.this, searcher, request, originalQuery, efi);
try {
- String solrQuery = q;
- final List<String> fqs = fq;
-
- if ((solrQuery == null) || solrQuery.isEmpty()) {
- solrQuery = "*:*";
- }
-
- solrQuery = macroExpander.expand(solrQuery);
- if (solrQuery == null) {
- throw new FeatureException(this.getClass().getSimpleName()+" requires efi parameter that was not passed in request.");
- }
-
- final SolrQueryRequest req = makeRequest(request.getCore(), solrQuery,
- fqs, df);
+ final SolrQueryRequest req = makeRequest(request.getCore(), q, fq, df);
if (req == null) {
throw new IOException("ERROR: No parameters provided");
}
+ // Build the scoring query
+ Query scoreQuery;
+ String qStr = q;
+ if (qStr == null || qStr.isEmpty()) {
+ scoreQuery = null; // ultimately behaves like MatchAllDocs
+ } else {
+ qStr = macroExpander.expand(qStr);
+ if (qStr == null) {
+ throw new FeatureException(this.getClass().getSimpleName() + " requires efi parameter that was not passed in request.");
+ }
+ scoreQuery = QParser.getParser(qStr, req).getQuery();
+ // note: QParser can return a null Query sometimes, such as if the query is a stopword or just symbols
+ if (scoreQuery == null) {
+ scoreQuery = new MatchNoDocsQuery(); // debatable; all or none?
+ }
+ }
+
// Build the filter queries
- queryAndFilters = new ArrayList<Query>(); // If there are no fqs we just want an empty list
- if (fqs != null) {
- for (String fq : fqs) {
- if ((fq != null) && (fq.trim().length() != 0)) {
Review comment:
observation: the `(fq.trim().length() != 0)` is being removed as part of the refactor but that seems fair since blank `fq` or `fq` with trailing blanks would seem unusual
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request
#1381: SOLR-14364: LTR SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381#discussion_r401063622
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/SolrFeature.java
##########
@@ -120,67 +123,66 @@ protected void validate() throws FeatureException {
": Q or FQ must be provided");
}
}
+
/**
* Weight for a SolrFeature
**/
public class SolrFeatureWeight extends FeatureWeight {
- final private Weight solrQueryWeight;
- final private Query query;
- final private List<Query> queryAndFilters;
+ private final Weight solrQueryWeight;
- public SolrFeatureWeight(IndexSearcher searcher,
- SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) throws IOException {
+ public SolrFeatureWeight(SolrIndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String, String[]> efi) throws IOException {
super(SolrFeature.this, searcher, request, originalQuery, efi);
try {
- String solrQuery = q;
- final List<String> fqs = fq;
-
- if ((solrQuery == null) || solrQuery.isEmpty()) {
- solrQuery = "*:*";
- }
-
- solrQuery = macroExpander.expand(solrQuery);
- if (solrQuery == null) {
- throw new FeatureException(this.getClass().getSimpleName()+" requires efi parameter that was not passed in request.");
- }
-
- final SolrQueryRequest req = makeRequest(request.getCore(), solrQuery,
- fqs, df);
+ final SolrQueryRequest req = makeRequest(request.getCore(), q, fq, df);
if (req == null) {
throw new IOException("ERROR: No parameters provided");
}
+ // Build the scoring query
+ Query scoreQuery;
+ String qStr = q;
+ if (qStr == null || qStr.isEmpty()) {
+ scoreQuery = null; // ultimately behaves like MatchAllDocs
+ } else {
+ qStr = macroExpander.expand(qStr);
+ if (qStr == null) {
+ throw new FeatureException(this.getClass().getSimpleName() + " requires efi parameter that was not passed in request.");
+ }
+ scoreQuery = QParser.getParser(qStr, req).getQuery();
+ // note: QParser can return a null Query sometimes, such as if the query is a stopword or just symbols
+ if (scoreQuery == null) {
+ scoreQuery = new MatchNoDocsQuery(); // debatable; all or none?
+ }
+ }
+
// Build the filter queries
- queryAndFilters = new ArrayList<Query>(); // If there are no fqs we just want an empty list
- if (fqs != null) {
- for (String fq : fqs) {
- if ((fq != null) && (fq.trim().length() != 0)) {
- fq = macroExpander.expand(fq);
- if (fq == null) {
- throw new FeatureException(this.getClass().getSimpleName()+" requires efi parameter that was not passed in request.");
+ Query filterDocSetQuery = null;
+ if (fq != null) {
+ List<Query> filterQueries = new ArrayList<>(); // If there are no fqs we just want an empty list
+ for (String fqStr : fq) {
+ if (fqStr != null) {
+ fqStr = macroExpander.expand(fqStr);
+ if (fqStr == null) {
+ throw new FeatureException(this.getClass().getSimpleName() + " requires efi parameter that was not passed in request.");
}
- final QParser fqp = QParser.getParser(fq, req);
- final Query filterQuery = fqp.getQuery();
+ final Query filterQuery = QParser.getParser(fqStr, req).getQuery();
if (filterQuery != null) {
- queryAndFilters.add(filterQuery);
+ filterQueries.add(filterQuery);
}
}
}
+
+ DocSet filtersDocSet = searcher.getDocSet(filterQueries); // execute
Review comment:
observation: i've sort of convinced myself that at this point `filterQueries` should be a non-empty list but if it were an empty list then i haven't yet dived fully enough into the `getDocSet` implementation to understand if that is doing what we want or if `filterDocSetQuery` remaining `null` might be clearer. perhaps you've thought about this already too?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request
#1381: SOLR-14364: LTR SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381#discussion_r402333435
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/SolrFeature.java
##########
@@ -120,67 +123,66 @@ protected void validate() throws FeatureException {
": Q or FQ must be provided");
}
}
+
/**
* Weight for a SolrFeature
**/
public class SolrFeatureWeight extends FeatureWeight {
- final private Weight solrQueryWeight;
- final private Query query;
- final private List<Query> queryAndFilters;
+ private final Weight solrQueryWeight;
- public SolrFeatureWeight(IndexSearcher searcher,
- SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) throws IOException {
+ public SolrFeatureWeight(SolrIndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String, String[]> efi) throws IOException {
super(SolrFeature.this, searcher, request, originalQuery, efi);
try {
- String solrQuery = q;
- final List<String> fqs = fq;
-
- if ((solrQuery == null) || solrQuery.isEmpty()) {
- solrQuery = "*:*";
- }
-
- solrQuery = macroExpander.expand(solrQuery);
- if (solrQuery == null) {
- throw new FeatureException(this.getClass().getSimpleName()+" requires efi parameter that was not passed in request.");
- }
-
- final SolrQueryRequest req = makeRequest(request.getCore(), solrQuery,
- fqs, df);
+ final SolrQueryRequest req = makeRequest(request.getCore(), q, fq, df);
if (req == null) {
throw new IOException("ERROR: No parameters provided");
}
+ // Build the scoring query
+ Query scoreQuery;
+ String qStr = q;
+ if (qStr == null || qStr.isEmpty()) {
+ scoreQuery = null; // ultimately behaves like MatchAllDocs
+ } else {
+ qStr = macroExpander.expand(qStr);
+ if (qStr == null) {
+ throw new FeatureException(this.getClass().getSimpleName() + " requires efi parameter that was not passed in request.");
+ }
+ scoreQuery = QParser.getParser(qStr, req).getQuery();
+ // note: QParser can return a null Query sometimes, such as if the query is a stopword or just symbols
+ if (scoreQuery == null) {
+ scoreQuery = new MatchNoDocsQuery(); // debatable; all or none?
Review comment:
> ... The semantics of what's here is no change from the prior logic. ...
Good to know, thanks for explaining!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1381:
SOLR-14364: LTR SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381#discussion_r401310167
##########
File path: solr/core/src/java/org/apache/solr/search/grouping/CommandHandler.java
##########
@@ -228,12 +226,8 @@ private void searchWithTimeLimiter(Query query,
collector = MultiCollector.wrap(collector, hitCountCollector);
}
- if (filter.filter != null) {
- query = new BooleanQuery.Builder()
- .add(query, Occur.MUST)
- .add(filter.filter, Occur.FILTER)
- .build();
- }
+ query = QueryUtils.combineQueryAndFilter(query, filter.filter);
Review comment:
I wouldn't define combineQueryAndFilter by who uses it to reverse engineer how it might work; it has its own javadocs. It behaves the same no matter who calls it. Do you think it's defaults are counterintuitive?
BTW I think it's a mistake that Solr's 'q' effectively defaults to semantics of MatchNoDocsQuery.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1381:
SOLR-14364: LTR SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381#discussion_r401323697
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/SolrFeature.java
##########
@@ -120,67 +123,66 @@ protected void validate() throws FeatureException {
": Q or FQ must be provided");
}
}
+
/**
* Weight for a SolrFeature
**/
public class SolrFeatureWeight extends FeatureWeight {
- final private Weight solrQueryWeight;
- final private Query query;
- final private List<Query> queryAndFilters;
+ private final Weight solrQueryWeight;
- public SolrFeatureWeight(IndexSearcher searcher,
- SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) throws IOException {
+ public SolrFeatureWeight(SolrIndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String, String[]> efi) throws IOException {
super(SolrFeature.this, searcher, request, originalQuery, efi);
try {
- String solrQuery = q;
- final List<String> fqs = fq;
-
- if ((solrQuery == null) || solrQuery.isEmpty()) {
- solrQuery = "*:*";
- }
-
- solrQuery = macroExpander.expand(solrQuery);
- if (solrQuery == null) {
- throw new FeatureException(this.getClass().getSimpleName()+" requires efi parameter that was not passed in request.");
- }
-
- final SolrQueryRequest req = makeRequest(request.getCore(), solrQuery,
- fqs, df);
+ final SolrQueryRequest req = makeRequest(request.getCore(), q, fq, df);
if (req == null) {
throw new IOException("ERROR: No parameters provided");
}
+ // Build the scoring query
+ Query scoreQuery;
+ String qStr = q;
+ if (qStr == null || qStr.isEmpty()) {
+ scoreQuery = null; // ultimately behaves like MatchAllDocs
+ } else {
+ qStr = macroExpander.expand(qStr);
+ if (qStr == null) {
+ throw new FeatureException(this.getClass().getSimpleName() + " requires efi parameter that was not passed in request.");
+ }
+ scoreQuery = QParser.getParser(qStr, req).getQuery();
+ // note: QParser can return a null Query sometimes, such as if the query is a stopword or just symbols
+ if (scoreQuery == null) {
+ scoreQuery = new MatchNoDocsQuery(); // debatable; all or none?
Review comment:
I think this is simply debatable in general. What does it mean for a query parser to return a null query, even when there was some input string? It's a bike-shed and reasonable arguments can be said either way. If I was in a more ambitious mood, I'd file a JIRA issue that would mandate that QParsers starting in 9.0 must return something other than null because the current situation is that every place that uses a QParser has to make some decision or possibly have subtle bugs if forgotten (which has happened over time).
The semantics of what's here is no change from the prior logic. It might not be obvious from the diff. In an earlier iteration of this PR I overlooked something here and the tests thankfully caught it. It's subtly confusing because there are multiple levels of defaulting -- the input string, the post-macro expansion, and the QParser result.
I feel strongly about how combineQueryAndFilter defaults / does its job though. It may be that some callers parse queries and produce different match-all vs match-none compared to combineQueryAndFilter if you were to give that a null parameter, but I'm not about to reconsider these places; I just wanted a useful utility method that I hope is intuitive to folks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request
#1381: SOLR-14364: LTR SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381#discussion_r401058472
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/SolrFeature.java
##########
@@ -120,67 +123,66 @@ protected void validate() throws FeatureException {
": Q or FQ must be provided");
}
}
+
/**
* Weight for a SolrFeature
**/
public class SolrFeatureWeight extends FeatureWeight {
- final private Weight solrQueryWeight;
- final private Query query;
- final private List<Query> queryAndFilters;
+ private final Weight solrQueryWeight;
- public SolrFeatureWeight(IndexSearcher searcher,
- SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) throws IOException {
+ public SolrFeatureWeight(SolrIndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String, String[]> efi) throws IOException {
super(SolrFeature.this, searcher, request, originalQuery, efi);
try {
- String solrQuery = q;
- final List<String> fqs = fq;
-
- if ((solrQuery == null) || solrQuery.isEmpty()) {
- solrQuery = "*:*";
- }
-
- solrQuery = macroExpander.expand(solrQuery);
- if (solrQuery == null) {
- throw new FeatureException(this.getClass().getSimpleName()+" requires efi parameter that was not passed in request.");
- }
-
- final SolrQueryRequest req = makeRequest(request.getCore(), solrQuery,
- fqs, df);
+ final SolrQueryRequest req = makeRequest(request.getCore(), q, fq, df);
if (req == null) {
throw new IOException("ERROR: No parameters provided");
}
+ // Build the scoring query
+ Query scoreQuery;
+ String qStr = q;
+ if (qStr == null || qStr.isEmpty()) {
+ scoreQuery = null; // ultimately behaves like MatchAllDocs
Review comment:
minor: s/MatchAllDocs/MatchAllDocsQuery
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1381:
SOLR-14364: LTR SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381#discussion_r399444698
##########
File path: solr/core/src/java/org/apache/solr/search/grouping/CommandHandler.java
##########
@@ -228,12 +226,8 @@ private void searchWithTimeLimiter(Query query,
collector = MultiCollector.wrap(collector, hitCountCollector);
}
- if (filter.filter != null) {
- query = new BooleanQuery.Builder()
- .add(query, Occur.MUST)
- .add(filter.filter, Occur.FILTER)
- .build();
- }
+ query = QueryUtils.combineQueryAndFilter(query, filter.filter);
Review comment:
If the parsed query is null (e.g. was stopwords or something), then the end effect is matching nothing. You can see this in the former logic by seeing solrQueryWeight getting set to null. In the new logic, this is done via setting query to MatchNoDocsQuery, and thus "query" variable is non-null.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1381:
SOLR-14364: LTR SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381#discussion_r401318959
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/SolrFeature.java
##########
@@ -120,67 +123,66 @@ protected void validate() throws FeatureException {
": Q or FQ must be provided");
}
}
+
/**
* Weight for a SolrFeature
**/
public class SolrFeatureWeight extends FeatureWeight {
- final private Weight solrQueryWeight;
- final private Query query;
- final private List<Query> queryAndFilters;
+ private final Weight solrQueryWeight;
- public SolrFeatureWeight(IndexSearcher searcher,
- SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) throws IOException {
+ public SolrFeatureWeight(SolrIndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String, String[]> efi) throws IOException {
super(SolrFeature.this, searcher, request, originalQuery, efi);
try {
- String solrQuery = q;
- final List<String> fqs = fq;
-
- if ((solrQuery == null) || solrQuery.isEmpty()) {
- solrQuery = "*:*";
- }
-
- solrQuery = macroExpander.expand(solrQuery);
- if (solrQuery == null) {
- throw new FeatureException(this.getClass().getSimpleName()+" requires efi parameter that was not passed in request.");
- }
-
- final SolrQueryRequest req = makeRequest(request.getCore(), solrQuery,
- fqs, df);
+ final SolrQueryRequest req = makeRequest(request.getCore(), q, fq, df);
if (req == null) {
throw new IOException("ERROR: No parameters provided");
}
+ // Build the scoring query
+ Query scoreQuery;
+ String qStr = q;
+ if (qStr == null || qStr.isEmpty()) {
+ scoreQuery = null; // ultimately behaves like MatchAllDocs
+ } else {
+ qStr = macroExpander.expand(qStr);
+ if (qStr == null) {
+ throw new FeatureException(this.getClass().getSimpleName() + " requires efi parameter that was not passed in request.");
+ }
+ scoreQuery = QParser.getParser(qStr, req).getQuery();
+ // note: QParser can return a null Query sometimes, such as if the query is a stopword or just symbols
+ if (scoreQuery == null) {
+ scoreQuery = new MatchNoDocsQuery(); // debatable; all or none?
+ }
+ }
+
// Build the filter queries
- queryAndFilters = new ArrayList<Query>(); // If there are no fqs we just want an empty list
- if (fqs != null) {
- for (String fq : fqs) {
- if ((fq != null) && (fq.trim().length() != 0)) {
- fq = macroExpander.expand(fq);
- if (fq == null) {
- throw new FeatureException(this.getClass().getSimpleName()+" requires efi parameter that was not passed in request.");
+ Query filterDocSetQuery = null;
+ if (fq != null) {
+ List<Query> filterQueries = new ArrayList<>(); // If there are no fqs we just want an empty list
+ for (String fqStr : fq) {
+ if (fqStr != null) {
+ fqStr = macroExpander.expand(fqStr);
+ if (fqStr == null) {
+ throw new FeatureException(this.getClass().getSimpleName() + " requires efi parameter that was not passed in request.");
}
- final QParser fqp = QParser.getParser(fq, req);
- final Query filterQuery = fqp.getQuery();
+ final Query filterQuery = QParser.getParser(fqStr, req).getQuery();
if (filterQuery != null) {
- queryAndFilters.add(filterQuery);
+ filterQueries.add(filterQuery);
}
}
}
+
+ DocSet filtersDocSet = searcher.getDocSet(filterQueries); // execute
Review comment:
Good eye. It's possible filterQueries might be empty if say someone added a blank fq. And I verified in a debugger get getDocSet does the right thing. It's taking a long time to do its job however (accumulating a complete docset, bit by bit). In another issue I'm targeting at 9.0, I optimized this case to a live-docs DocSet, among other things relating to TwoPhaseIterator. I should pull that out and make it its own improvement, along with a bit more documentation to getDocSet to indicate that's what the semantics are if an empty list is passed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request
#1381: SOLR-14364: LTR SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381#discussion_r401065312
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/SolrFeature.java
##########
@@ -280,52 +243,10 @@ public float score() throws IOException {
@Override
public float getMaxScore(int upTo) throws IOException {
- return Float.POSITIVE_INFINITY;
- }
- }
-
- /**
- * An iterator that allows to iterate only on the documents for which a feature has
- * a value.
- **/
- public class SolrFeatureScorerIterator extends DocIdSetIterator {
-
- final private DocIdSetIterator filterIterator;
- final private DocIdSetIterator scorerFilter;
-
- SolrFeatureScorerIterator(DocIdSetIterator filterIterator,
- DocIdSetIterator scorerFilter) {
- this.filterIterator = filterIterator;
- this.scorerFilter = scorerFilter;
- }
-
- @Override
- public int docID() {
- return filterIterator.docID();
- }
-
- @Override
- public int nextDoc() throws IOException {
- int docID = filterIterator.nextDoc();
- scorerFilter.advance(docID);
- return docID;
- }
-
- @Override
- public int advance(int target) throws IOException {
- // We use iterator to catch the scorer up since
- // that checks if the target id is in the query + all the filters
- int docID = filterIterator.advance(target);
- scorerFilter.advance(docID);
- return docID;
- }
-
- @Override
- public long cost() {
- return filterIterator.cost() + scorerFilter.cost();
+ return solrScorer.getMaxScore(upTo);
}
+ // TODO delegate more methods?
Review comment:
Good question. I'll take a look to see if we could have some reflective unit test coverage either way (similar to existing coverage elsewhere) to ensure delegation for any methods that might be added in future.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley closed pull request #1381: SOLR-14364: LTR
SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
dsmiley closed pull request #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request
#1381: SOLR-14364: LTR SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381#discussion_r401059644
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/SolrFeature.java
##########
@@ -120,67 +123,66 @@ protected void validate() throws FeatureException {
": Q or FQ must be provided");
}
}
+
/**
* Weight for a SolrFeature
**/
public class SolrFeatureWeight extends FeatureWeight {
- final private Weight solrQueryWeight;
- final private Query query;
- final private List<Query> queryAndFilters;
+ private final Weight solrQueryWeight;
- public SolrFeatureWeight(IndexSearcher searcher,
- SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) throws IOException {
+ public SolrFeatureWeight(SolrIndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String, String[]> efi) throws IOException {
super(SolrFeature.this, searcher, request, originalQuery, efi);
try {
- String solrQuery = q;
- final List<String> fqs = fq;
-
- if ((solrQuery == null) || solrQuery.isEmpty()) {
- solrQuery = "*:*";
- }
-
- solrQuery = macroExpander.expand(solrQuery);
- if (solrQuery == null) {
- throw new FeatureException(this.getClass().getSimpleName()+" requires efi parameter that was not passed in request.");
- }
-
- final SolrQueryRequest req = makeRequest(request.getCore(), solrQuery,
- fqs, df);
+ final SolrQueryRequest req = makeRequest(request.getCore(), q, fq, df);
if (req == null) {
throw new IOException("ERROR: No parameters provided");
}
+ // Build the scoring query
+ Query scoreQuery;
+ String qStr = q;
+ if (qStr == null || qStr.isEmpty()) {
+ scoreQuery = null; // ultimately behaves like MatchAllDocs
+ } else {
+ qStr = macroExpander.expand(qStr);
+ if (qStr == null) {
+ throw new FeatureException(this.getClass().getSimpleName() + " requires efi parameter that was not passed in request.");
+ }
+ scoreQuery = QParser.getParser(qStr, req).getQuery();
+ // note: QParser can return a null Query sometimes, such as if the query is a stopword or just symbols
+ if (scoreQuery == null) {
+ scoreQuery = new MatchNoDocsQuery(); // debatable; all or none?
Review comment:
Would 'all' be consistent with both prior q-is-missing code path using `*:*` and the new `QueryUtils.combineQueryAndFilter` choosing all?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on issue #1381: SOLR-14364: LTR
SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
dsmiley commented on issue #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381#issuecomment-608087581
Are you okay with me merging this now or is there anything left to resolve @cpoerschke ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request
#1381: SOLR-14364: LTR SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381#discussion_r401064123
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/SolrFeature.java
##########
@@ -209,49 +211,11 @@ private LocalSolrQueryRequest makeRequest(SolrCore core, String solrQuery,
@Override
public FeatureScorer scorer(LeafReaderContext context) throws IOException {
- Scorer solrScorer = null;
- if (solrQueryWeight != null) {
- solrScorer = solrQueryWeight.scorer(context);
- }
-
- final DocIdSetIterator idItr = getDocIdSetIteratorFromQueries(
- queryAndFilters, context);
- if (idItr != null) {
- return solrScorer == null ? new ValueFeatureScorer(this, 1f, idItr)
- : new SolrFeatureScorer(this, solrScorer,
- new SolrFeatureScorerIterator(idItr, solrScorer.iterator()));
- } else {
Review comment:
Very nice how all this vanishes!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request
#1381: SOLR-14364: LTR SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381#discussion_r401067383
##########
File path: solr/core/src/java/org/apache/solr/search/grouping/CommandHandler.java
##########
@@ -228,12 +226,8 @@ private void searchWithTimeLimiter(Query query,
collector = MultiCollector.wrap(collector, hitCountCollector);
}
- if (filter.filter != null) {
- query = new BooleanQuery.Builder()
- .add(query, Occur.MUST)
- .add(filter.filter, Occur.FILTER)
- .build();
- }
+ query = QueryUtils.combineQueryAndFilter(query, filter.filter);
Review comment:
Thanks for the pointer, i see the `MatchNoDocsQuery` now in SolrFeature, hadn't looked before.
Outside of `ltr/SolrFeature` though the `QueryUtils.combineQueryAndFilter` is using `MatchAllDocsQuery`, right?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] cpoerschke commented on issue #1381: SOLR-14364: LTR
SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on issue #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381#issuecomment-608387013
> Are you okay with me merging this now or is there anything left to resolve @cpoerschke ?
Sure, please go ahead. Thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1381:
SOLR-14364: LTR SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381#discussion_r401313568
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/SolrFeature.java
##########
@@ -120,67 +123,66 @@ protected void validate() throws FeatureException {
": Q or FQ must be provided");
}
}
+
/**
* Weight for a SolrFeature
**/
public class SolrFeatureWeight extends FeatureWeight {
- final private Weight solrQueryWeight;
- final private Query query;
- final private List<Query> queryAndFilters;
+ private final Weight solrQueryWeight;
- public SolrFeatureWeight(IndexSearcher searcher,
- SolrQueryRequest request, Query originalQuery, Map<String,String[]> efi) throws IOException {
+ public SolrFeatureWeight(SolrIndexSearcher searcher,
+ SolrQueryRequest request, Query originalQuery, Map<String, String[]> efi) throws IOException {
super(SolrFeature.this, searcher, request, originalQuery, efi);
try {
- String solrQuery = q;
- final List<String> fqs = fq;
-
- if ((solrQuery == null) || solrQuery.isEmpty()) {
- solrQuery = "*:*";
- }
-
- solrQuery = macroExpander.expand(solrQuery);
- if (solrQuery == null) {
- throw new FeatureException(this.getClass().getSimpleName()+" requires efi parameter that was not passed in request.");
- }
-
- final SolrQueryRequest req = makeRequest(request.getCore(), solrQuery,
- fqs, df);
+ final SolrQueryRequest req = makeRequest(request.getCore(), q, fq, df);
if (req == null) {
throw new IOException("ERROR: No parameters provided");
}
+ // Build the scoring query
+ Query scoreQuery;
+ String qStr = q;
+ if (qStr == null || qStr.isEmpty()) {
+ scoreQuery = null; // ultimately behaves like MatchAllDocs
+ } else {
+ qStr = macroExpander.expand(qStr);
+ if (qStr == null) {
+ throw new FeatureException(this.getClass().getSimpleName() + " requires efi parameter that was not passed in request.");
+ }
+ scoreQuery = QParser.getParser(qStr, req).getQuery();
+ // note: QParser can return a null Query sometimes, such as if the query is a stopword or just symbols
+ if (scoreQuery == null) {
+ scoreQuery = new MatchNoDocsQuery(); // debatable; all or none?
+ }
+ }
+
// Build the filter queries
- queryAndFilters = new ArrayList<Query>(); // If there are no fqs we just want an empty list
- if (fqs != null) {
- for (String fq : fqs) {
- if ((fq != null) && (fq.trim().length() != 0)) {
Review comment:
It seemed like a needless check since a blank fq would likely get parsed to null and we'd drop it then any way.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request
#1381: SOLR-14364: LTR SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381#discussion_r402945403
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/SolrFeature.java
##########
@@ -280,52 +243,10 @@ public float score() throws IOException {
@Override
public float getMaxScore(int upTo) throws IOException {
- return Float.POSITIVE_INFINITY;
- }
- }
-
- /**
- * An iterator that allows to iterate only on the documents for which a feature has
- * a value.
- **/
- public class SolrFeatureScorerIterator extends DocIdSetIterator {
-
- final private DocIdSetIterator filterIterator;
- final private DocIdSetIterator scorerFilter;
-
- SolrFeatureScorerIterator(DocIdSetIterator filterIterator,
- DocIdSetIterator scorerFilter) {
- this.filterIterator = filterIterator;
- this.scorerFilter = scorerFilter;
- }
-
- @Override
- public int docID() {
- return filterIterator.docID();
- }
-
- @Override
- public int nextDoc() throws IOException {
- int docID = filterIterator.nextDoc();
- scorerFilter.advance(docID);
- return docID;
- }
-
- @Override
- public int advance(int target) throws IOException {
- // We use iterator to catch the scorer up since
- // that checks if the target id is in the query + all the filters
- int docID = filterIterator.advance(target);
- scorerFilter.advance(docID);
- return docID;
- }
-
- @Override
- public long cost() {
- return filterIterator.cost() + scorerFilter.cost();
+ return solrScorer.getMaxScore(upTo);
}
+ // TODO delegate more methods?
Review comment:
In case it helps, I've branched off your branch here, merged latest master into it and added small [commit](https://github.com/cpoerschke/lucene-solr/commit/56af94b59e0377c4a1279ed5569ba4cc8cea0ee2) for `SolrFeature` here extending `FilterFeatureScorer`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request
#1381: SOLR-14364: LTR SolrFeature fq improvements
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #1381: SOLR-14364: LTR SolrFeature fq improvements
URL: https://github.com/apache/lucene-solr/pull/1381#discussion_r402335911
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/feature/SolrFeature.java
##########
@@ -280,52 +243,10 @@ public float score() throws IOException {
@Override
public float getMaxScore(int upTo) throws IOException {
- return Float.POSITIVE_INFINITY;
- }
- }
-
- /**
- * An iterator that allows to iterate only on the documents for which a feature has
- * a value.
- **/
- public class SolrFeatureScorerIterator extends DocIdSetIterator {
-
- final private DocIdSetIterator filterIterator;
- final private DocIdSetIterator scorerFilter;
-
- SolrFeatureScorerIterator(DocIdSetIterator filterIterator,
- DocIdSetIterator scorerFilter) {
- this.filterIterator = filterIterator;
- this.scorerFilter = scorerFilter;
- }
-
- @Override
- public int docID() {
- return filterIterator.docID();
- }
-
- @Override
- public int nextDoc() throws IOException {
- int docID = filterIterator.nextDoc();
- scorerFilter.advance(docID);
- return docID;
- }
-
- @Override
- public int advance(int target) throws IOException {
- // We use iterator to catch the scorer up since
- // that checks if the target id is in the query + all the filters
- int docID = filterIterator.advance(target);
- scorerFilter.advance(docID);
- return docID;
- }
-
- @Override
- public long cost() {
- return filterIterator.cost() + scorerFilter.cost();
+ return solrScorer.getMaxScore(upTo);
}
+ // TODO delegate more methods?
Review comment:
Looks like there could be scope for factoring out a `FilterFeatureScorer` class, I've opened https://issues.apache.org/jira/browse/SOLR-14378 re: that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org