------- Additional Comments From [EMAIL PROTECTED] 2005-04-27 17:15 ------- Wolf's revisions to my changes to Query.combine() look fine. The single-query optimization is good -- my oversight to have not included it originally. I don't believe either of the other two changes is necessary, but they are correct: 1. Using a flag instead of the labelled loop is a matter of style as Wolf says, and it's a little less efficent (the biggest effect could be remedied by one more if (splittable) to avoid unnecessarily copying the clauses of a BooleanQuery where coord is not disabled).
Yep, the additional if... should be added.
2. Changing BooleanQuery equality to be independent of clause order is semantically correct, although again it is a little less efficient. It's only purpose is to stop a false-negative in the new tests.
Here I don't agree. The previous implementation was incorrect, and the new tests did discover that bug. I also considered to correct this by ensuring a defined order of clauses, or by replacing the vector with a set. That would have been a bit more performant, but would have needed much more effort and may have caused unwanted side effects.
In general, IMHO query processing performance is nearly always dominated by index accesses, and in the few cases where query preparation takes a significant share, the whole processing will be fast enough anyway. So I don't see a need to squeeze out the last few processing cycles from query preparation.
Many additional optimizations could be added. It seems redundant to have optimizations here and in the rewrite mechanism. Since we are down to just Query.combine(), only called from one place, I think a better fix is to change MultiSearcher to pass the reader as well. Then Query.combine() could construct the straightforward BooleanQuery and rewrite it. All the optimizations would then go into a single place, the rewrite methods. Wolf, what do you think of that approach?
Yes, there is a problem of code duplication. But I don't yet understand your proposal. Which reader could the MultiSearcher pass? We only have Searchables inside of MultiSearcher which don't (and probably shouldn't) expose their readers.
Another way to approach the problem would be to split the rewriting process into two phases: in the first phase the query is rewritten into a combination of term queries, and in the second phase this combination is optimized. The second phase doesn't need the reader anymore. Then the MultiSearcher could delegate the first phase to its Searchables (as before), combine the resulting queries by just joining them, and then call the optimization method on the combined query. If there are no objections I could try if that works.
--Wolf
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
