javanna commented on a change in pull request #581: LUCENE-3041: QueryVisitor URL: https://github.com/apache/lucene-solr/pull/581#discussion_r260298924
########## File path: lucene/core/src/java/org/apache/lucene/search/PhraseQuery.java ########## @@ -284,6 +284,16 @@ public Query rewrite(IndexReader reader) throws IOException { } } + @Override + public void visit(QueryVisitor visitor) { + for (Term term : terms) { + QueryVisitor v = visitor.getSubVisitor(BooleanClause.Occur.MUST, this); Review comment: I agree with Simon. I find it counter intuitive to get the sub-visitor for the same query more than once. if we do need that short-circuiting maybe it can be made more explicit in the API. Probably because I am not yet very familiar with the change here, but I have a hard time following where things happen here: we pull the sub-visitor from the same parent visitor multiple times, yet what gets returned may be null at some point. Does that mean that the parent visitor holds state that changes as we consume terms through its sub-visitors? ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on 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: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org