jtibshirani commented on a change in pull request #160: URL: https://github.com/apache/lucene/pull/160#discussion_r644158197
########## File path: lucene/core/src/test/org/apache/lucene/search/TestSynonymQuery.java ########## @@ -466,4 +467,26 @@ public void testRandomTopDocs() throws IOException { reader.close(); dir.close(); } + + public void testRewrite() throws IOException { + // zero length SynonymQuery is rewritten + SynonymQuery q = new SynonymQuery.Builder("f").build(); Review comment: A small comment: in most other rewrite tests (like `TestBoostQuery#testRewrite`) we check the higher-level call `IndexSearcher#rewrite`. It'd be nice to do this to be consistent and to exercise the full rewrite logic. ########## File path: lucene/sandbox/src/java/org/apache/lucene/sandbox/search/CombinedFieldQuery.java ########## @@ -237,14 +236,6 @@ public Query rewrite(IndexReader reader) throws IOException { if (fieldTerms.length == 1) { Review comment: I like the simplification below of removing the rewrite to synonym query (which is not perfectly accurate). I think we also need to fix or remove this check `if (fieldTerms.length == 1) { ... }`, since it's only accurate when the field weight is 1.0f. -- 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 --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org