I really dislike the cancerous chaining (all setters returning
this): it's poor API design because it creates unnecessary
ambiguity on how to consume it. It amounts to a denial-of-service
attack on the devs who consume our APIs.... We should strive to have
less, not more, ambiguity in all of our APIs.
But, since others seem to love it, as a compromise, can you write all
consumption of the cancerous chaining as if the methods did not chain?
Ie minimize the cancer: contain it to the API definition, alone.
The litmus test is then simple: if I were to change all methods to
return void instead, everything should compile / tests should pass.
Otherwise, the patch looks great!
One can actually use GroupingSearch in a shard'd env, on each shard,
right? It's just that then you merge them like normal on the front
end (ie, TopGroups.merge). Is that the only reason for the "... in a
non distributed environment" javadoc warning?
I think the jdocs for each ctor should explain what kind of grouping
impl will be used (ie, the ctor taking Filter groupEndDocs uses
single-pass block collector, and requires you indexed doc blocks).
Maybe the ctor should take docValuesType / diskResidentValues, instead
of setters to change it? Ie, so that you are stating up front what
source to group by (DocValues, FC (Term), function, block).
Maybe you should pass the groupSort, groupsOffset, groupsLimit to the
search method (instead of setters)?