dsmiley commented on code in PR #4812:
URL: https://github.com/apache/solr/pull/4812#discussion_r3970913075
##########
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:
I think this method's implementation can be simply `return new
KeywordAnalyzer().tokenStream(null, "");`
##########
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:
call deepCopyOf() on that BytesRef. Usually the BytesRef is re-used.
Instead of even having a BytesRef intermediary, we might just simply have a
copy of the byte[] itself; sized right -- I leave that to you.
##########
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:
close() should be in the finally
##########
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:
_again_
##########
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:
it'd be reasonable to add a length() helper, IMO.
##########
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:
and again
--
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]
