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: us...@infra.apache.org With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org