epugh commented on PR #4812: URL: https://github.com/apache/solr/pull/4812#issuecomment-5600948727
I wanted to see what Claude would do so I asked it to `/simplify` and `/code-review` and this is the summary of what i found, plus a `what_claude_found.patch` file: 1. Resource leak fix (/code-review, 3 sites, +1 I found matching the same pattern): AbstractLuceneSpellChecker, DirectSolrSpellChecker, Suggester.getSuggestions, WordBreakSolrSpellChecker.drainToArray, and SpellCheckComponent.addOriginalTermsToResponse all opened a TokenStream via tokenStreamSupplier.get() with stream.close() only reachable if the loop ran to completion — any mid-loop exception leaked it. Wrapped each in try/finally. 2. Duplication fix (reuse + simplification + altitude, all three independently flagged this): the same ~10-line "register 6 attributes, build a SpellCheckToken per token" block was copy-pasted across 4 sites. Extracted a new SpellCheckToken.AttributeReader (public nested class) that registers the attributes once and exposes .current(); all 4 sites now use it. 3. Efficiency fix (efficiency agent): QueryWordsTokenStream.incrementToken() was calling addAttribute() on every single token instead of once per word — now cached as fields when the inner per-word stream is created. [what_claude_found.patch](https://github.com/user-attachments/files/32004935/what_claude_found.patch) And then, two more items that were flagged as much bigger effort: 1. The Supplier<TokenStream> design itself. Every reviewer converged here from a different angle: it forces the query to be re-tokenized 2–4x per request (peek check, real suggestion pass, addOriginalTermsToResponse, and once per sub-checker in ConjunctionSolrSpellChecker) where the old Collection<Token> was computed once and shared. Since every real consumer drains the stream to completion anyway (no early exit), a List<SpellCheckToken> computed once — which SpellCheckToken is already perfectly shaped for — would eliminate the re-analysis cost, the leak-prone stream lifecycle, and most of the duplication, and would let QueryWordsTokenStream (120 new lines) be deleted entirely. This is a real redesign of the PR's core mechanism though, not a local fix — I'd rather you or the PR author weigh in before I rewrite that much of someone else's contribution. 2. Swallowed IOException behavior change (/code-review): the old SpellingQueryConverter.convert() silently caught and skipped a per-word IOException; the new lazy stream has no such catch, so one bad word now aborts the whole request. That's a behavior call for the author to make intentionally, not something to silently patch either direction. Now I am starting to understand why the deprecation has lingered since Solr 7! @serhiy-bzhezytskyy can you evaluate the specific fixes in the patch file? They seem like they need a decision made, I don't know if they are right or just more complexity. After all, the tests all pass! Then, those two items need some thinking.. Is there a way to embrace the `List<SpellCheckToken>` compute once idea? -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
