Wow, i'd *just* finished reading the last patch completely through, and now there's a new one ... fortunately i'm familiar with some of these changes (the day someone writes a really good "diff differ" i'll be a very happy man)
A couple of issues that jump out at me (in the new patch)... 1) you changed the SolrPluginUtils.splitList from ",| " to "," ... this eliminates the ability to seperate fields with spaces in query params, which i know is fairly common -- was there a particular reason for that? 2) if a field doesn't generate any snippets, it's still included in the output for that doc ... is this intentional? 3) using the example app with this patch, i tried both of these... http://localhost:8983/solr/select/?indent=on&q=video&fl=id,features&highlight=true&highlightFields=features http://localhost:8983/solr/select/?indent=on&q=features:video&fl=id,features&highlight=true&highlightFields=features ...on my port, i get no highlighting for any doc, even though in both cases there are docs in the result that have the simple term "video" in the features field ... am i doing something wrong, or is this a bug? 4) if you open a LUCENE-XXXX issue for the QueryTermExtractor issue with DisjunctionMaxQueries, and attach a patch with testcases, and no one objects, i'll commit it :) ... but in the mean time i'm curous what it is about QueryTermExtractor that doesn't work -- it looks like it's fallback behavior is to use "query.extractTerms" and that should do the same thing as your getTermsFromDisjunctionMaxQuery right? 5) perhaps a better way to pick the "default" fields if highlightFields isn't specified would be using the return fields being used (the "fl" param) rather then the default search field in StandardRequestHandler and the "qf" fields in DisMaxHandler ? 6) one thing to watch out for is schemas with no uniqueKey defined. currently that produces highlighting like this... <lst name="highlighting"> <lst name="id=null"> ... ...that would probably be better with no name. 7) there is at least one additional place to StandardRequestHandler could be refactored to use SolrPluginUtils -- that would be setReturnFields (which i discovered when i saw StandardRequestHandler still has the old splitList value) #5-7 above aren't issues, just comments, but if you can clarify #1-4 i'll happily commit this patch. -Hoss
