serhiy-bzhezytskyy commented on code in PR #4812: URL: https://github.com/apache/solr/pull/4812#discussion_r3976295935
########## solr/core/src/java/org/apache/solr/spelling/SpellCheckToken.java: ########## @@ -0,0 +1,107 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.spelling; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import org.apache.lucene.analysis.TokenStream; +import org.apache.lucene.analysis.tokenattributes.CharTermAttribute; +import org.apache.lucene.analysis.tokenattributes.FlagsAttribute; +import org.apache.lucene.analysis.tokenattributes.OffsetAttribute; +import org.apache.lucene.analysis.tokenattributes.PayloadAttribute; +import org.apache.lucene.analysis.tokenattributes.PositionIncrementAttribute; +import org.apache.lucene.analysis.tokenattributes.TypeAttribute; +import org.apache.lucene.util.BytesRef; + +/** + * One term occurrence carried through the spellchecker API, and the key type of {@link + * SpellingResult}. Unlike the old {@code Token} it replaces, this is a plain, immutable record -- + * not a Lucene {@code AttributeImpl} subclass. + * + * <p>A value type is needed here, rather than reading terms off a stream, because {@link + * SolrSpellChecker#mergeSuggestions} keys suggestions by (text, offset) pairs deserialized from a + * remote shard's response, where there is no {@link org.apache.lucene.analysis.TokenStream} to read + * from at all. + */ +public record SpellCheckToken( + String text, + int startOffset, + int endOffset, + String type, + int positionIncrement, + int flags, + BytesRef payload) { + + public SpellCheckToken(String text, int startOffset, int endOffset) { + this(text, startOffset, endOffset, "word", 1, 0, null); + } + + @Override + public String toString() { + return text; + } + + /** + * Reads {@code stream} to its end into a list, and closes it. This is the single point where a + * {@link TokenStream} becomes values: every consumer of the spellcheck API reads every token, so + * the stream's lifecycle need not reach any of them. + */ + public static List<SpellCheckToken> drain(TokenStream stream) throws IOException { + List<SpellCheckToken> tokens = new ArrayList<>(); + try (stream) { + AttributeReader attrs = new AttributeReader(stream); + stream.reset(); + while (stream.incrementToken()) { + tokens.add(attrs.current()); + } + stream.end(); + } + return tokens; + } + + /** Registers the six attributes {@link #drain} reads, once for the whole stream. */ + private static final class AttributeReader { + private final CharTermAttribute termAtt; + private final OffsetAttribute offsetAtt; + private final TypeAttribute typeAtt; + private final PositionIncrementAttribute posIncAtt; + private final FlagsAttribute flagsAtt; + private final PayloadAttribute payloadAtt; + + AttributeReader(TokenStream stream) { + termAtt = stream.addAttribute(CharTermAttribute.class); + offsetAtt = stream.addAttribute(OffsetAttribute.class); + typeAtt = stream.addAttribute(TypeAttribute.class); + posIncAtt = stream.addAttribute(PositionIncrementAttribute.class); + flagsAtt = stream.addAttribute(FlagsAttribute.class); + payloadAtt = stream.addAttribute(PayloadAttribute.class); + } + + /** Builds a {@link SpellCheckToken} from the stream's current position. */ + SpellCheckToken current() { + return new SpellCheckToken( + termAtt.toString(), + offsetAtt.startOffset(), + offsetAtt.endOffset(), + typeAtt.type(), + posIncAtt.getPositionIncrement(), + flagsAtt.getFlags(), + payloadAtt.getPayload()); Review Comment: Done, in the canonical constructor, so every construction is covered rather than only the attribute read -- and with the same null guard Lucene's own `PayloadAttributeImpl` uses in `copyTo` and `clone`. I kept `BytesRef` over a bare `byte[]`: this record is a map key in `SpellingResult` and `ConjunctionSolrSpellChecker`, and an array component would make the generated `equals`/`hashCode` identity-based. ########## solr/core/src/test/org/apache/solr/spelling/DirectSolrSpellCheckerTest.java: ########## @@ -51,6 +52,29 @@ public static void beforeClass() throws Exception { queryConverter.init(new NamedList<>()); } + /** A stream that emits exactly one token whose term text is empty. */ Review Comment: Applied at all three. Measured that it drains to exactly the token the assertion keys on: size 1, `text=""`, offsets 0..0, `type=word`, `posInc=1`, `payload=null` -- equal to `new SpellCheckToken("", 0, 0)`. 54 lines net across the three. ########## solr/core/src/test/org/apache/solr/spelling/DirectSolrSpellCheckerTest.java: ########## @@ -51,6 +52,29 @@ public static void beforeClass() throws Exception { queryConverter.init(new NamedList<>()); } + /** A stream that emits exactly one token whose term text is empty. */ Review Comment: Used `""`. Both work here: `tokenStream(null, "")` drains to the same single token, since `KeywordAnalyzer` never reads the field name. ########## solr/core/src/test/org/apache/solr/spelling/FileBasedSpellCheckerTest.java: ########## @@ -59,6 +60,29 @@ public static void afterClass() { queryConverter = null; } + /** A stream that emits exactly one token whose term text is empty. */ + private static TokenStream singleEmptyTermTokenStream() { Review Comment: Done -- all three copies were byte-identical. ########## solr/core/src/test/org/apache/solr/spelling/IndexBasedSpellCheckerTest.java: ########## @@ -197,16 +199,39 @@ public void testSpelling() throws Exception { (int) entry.getValue()); // Check empty token due to spellcheck.q = "" - spellOpts.tokens = List.of(new Token("", 0, 0)); + spellOpts.tokens = SpellCheckToken.drain(singleEmptyTermTokenStream()); result = checker.getSuggestions(spellOpts); assertNotNull(result); - suggestions = result.get(spellOpts.tokens.iterator().next()); + suggestions = result.get(new SpellCheckToken("", 0, 0)); assertNotNull(suggestions); assertTrue("suggestions should be empty", suggestions.isEmpty()); return null; }); } + /** A stream that emits exactly one token whose term text is empty. */ + private static TokenStream singleEmptyTermTokenStream() { Review Comment: Done. ########## solr/core/src/test/org/apache/solr/spelling/TestSuggestSpellingConverter.java: ########## @@ -66,12 +67,20 @@ protected TokenStreamComponents createComponents(String fieldName) { } public void assertConvertsTo(String text, String expected[]) { - Collection<Token> tokens = converter.convert(text); - assertEquals(tokens.size(), expected.length); - int i = 0; - for (Token token : tokens) { - assertEquals(token.toString(), expected[i]); - i++; + try { + TokenStream stream = converter.convert(text); + stream.reset(); + CharTermAttribute termAtt = stream.addAttribute(CharTermAttribute.class); + int i = 0; + while (stream.incrementToken()) { + assertEquals(termAtt.toString(), expected[i]); + i++; + } + stream.end(); + stream.close(); Review Comment: Done, try-with-resources. ########## solr/core/src/test/org/apache/solr/spelling/WordBreakSolrSpellCheckerTest.java: ########## @@ -73,83 +73,85 @@ public void testStandAlone() throws Exception { { // Prior to SOLR-8175, the required term would cause an AIOOBE. - Collection<Token> tokens = qc.convert("+pine apple good ness"); + List<SpellCheckToken> tokens = SpellCheckToken.drain(qc.convert("+pine apple good ness")); SpellingOptions spellOpts = new SpellingOptions(tokens, searcher.get().getIndexReader(), 10); SpellingResult result = checker.getSuggestions(spellOpts); searcher.decref(); assertTrue(result != null && result.getSuggestions() != null); assertEquals(5, result.getSuggestions().size()); } - Collection<Token> tokens = qc.convert("paintable pine apple good ness"); + List<SpellCheckToken> tokens = + SpellCheckToken.drain(qc.convert("paintable pine apple good ness")); SpellingOptions spellOpts = new SpellingOptions(tokens, searcher.get().getIndexReader(), 10); SpellingResult result = checker.getSuggestions(spellOpts); searcher.decref(); assertTrue(result != null && result.getSuggestions() != null); assertEquals(9, result.getSuggestions().size()); - for (Map.Entry<Token, LinkedHashMap<String, Integer>> s : result.getSuggestions().entrySet()) { - Token orig = s.getKey(); + for (Map.Entry<SpellCheckToken, LinkedHashMap<String, Integer>> s : + result.getSuggestions().entrySet()) { + SpellCheckToken orig = s.getKey(); String[] corr = s.getValue().keySet().toArray(new String[0]); if (orig.toString().equals("paintable")) { assertEquals(0, orig.startOffset()); assertEquals(9, orig.endOffset()); - assertEquals(9, orig.length()); + assertEquals(9, orig.text().length()); assertEquals(3, corr.length); assertEquals("paint able", corr[0]); // 1 op ; max doc freq=5 assertEquals("pain table", corr[1]); // 1 op ; max doc freq=2 assertEquals("pa in table", corr[2]); // 2 ops } else if (orig.toString().equals("pine apple")) { assertEquals(10, orig.startOffset()); assertEquals(20, orig.endOffset()); - assertEquals(10, orig.length()); + assertEquals(10, orig.text().length()); assertEquals(1, corr.length); assertEquals("pineapple", corr[0]); } else if (orig.toString().equals("paintable pine")) { assertEquals(0, orig.startOffset()); assertEquals(14, orig.endOffset()); - assertEquals(14, orig.length()); + assertEquals(14, orig.text().length()); assertEquals(1, corr.length); assertEquals("paintablepine", corr[0]); } else if (orig.toString().equals("good ness")) { assertEquals(21, orig.startOffset()); assertEquals(30, orig.endOffset()); - assertEquals(9, orig.length()); + assertEquals(9, orig.text().length()); assertEquals(1, corr.length); assertEquals("goodness", corr[0]); } else if (orig.toString().equals("pine apple good ness")) { assertEquals(10, orig.startOffset()); assertEquals(30, orig.endOffset()); - assertEquals(20, orig.length()); + assertEquals(20, orig.text().length()); assertEquals(1, corr.length); assertEquals("pineapplegoodness", corr[0]); } else if (orig.toString().equals("pine")) { assertEquals(10, orig.startOffset()); assertEquals(14, orig.endOffset()); - assertEquals(4, orig.length()); + assertEquals(4, orig.text().length()); Review Comment: Restored it on the record -- this PR had turned `orig.length()` into `orig.text().length()` at these 10 sites, and `length()` puts them back. The one other `.text().length()` in the tree is on a Lucene `Term`, so I left that one. -- 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]
