uschindler commented on code in PR #15704:
URL: https://github.com/apache/lucene/pull/15704#discussion_r2845737453
##########
lucene/analysis/common/src/java/org/apache/lucene/analysis/synonym/word2vec/Word2VecSynonymFilter.java:
##########
@@ -91,9 +92,9 @@ public boolean incrementToken() throws IOException {
BytesRefBuilder bytesRefBuilder = new BytesRefBuilder();
bytesRefBuilder.copyChars(termAtt.buffer(), 0, termAtt.length());
BytesRef term = bytesRefBuilder.get();
- List<TermAndBoost> synonyms =
+ Collection<TermAndBoost> synonyms =
this.synonymProvider.getSynonyms(term, maxSynonymsPerTerm,
minAcceptedSimilarity);
- if (synonyms.size() > 0) {
+ if (!synonyms.isEmpty()) {
Review Comment:
in Lucene world, most people prefer `synonyms.isEmpty() == false` (I am not
one of them , I like short code more, but wanted to say this).
##########
lucene/core/src/java/org/apache/lucene/analysis/AutomatonToTokenStream.java:
##########
@@ -61,10 +62,10 @@ public static TokenStream toTokenStream(Automaton
automaton) {
throw new IllegalArgumentException("Start node has incoming edges,
creating cycle");
}
- LinkedList<RemapNode> noIncomingEdges = new LinkedList<>();
+ Deque<RemapNode> noIncomingEdges = new ArrayDeque<>();
IntIntHashMap idToPos = new IntIntHashMap();
noIncomingEdges.addLast(new RemapNode(0, 0));
- while (noIncomingEdges.isEmpty() == false) {
+ while (!noIncomingEdges.isEmpty()) {
Review Comment:
see above
##########
lucene/analysis/common/src/test/org/apache/lucene/analysis/core/TestFlattenGraphFilter.java:
##########
@@ -815,167 +814,4 @@ public boolean recursivelyValidate(
}
return accept;
}
-
Review Comment:
thanks for removing this :-)
##########
lucene/analysis/common/src/java/org/apache/lucene/analysis/synonym/word2vec/Word2VecSynonymProvider.java:
##########
@@ -66,14 +67,14 @@ public Word2VecSynonymProvider(Word2VecModel model) throws
IOException {
this.hnswGraph = builder.build(word2VecModel.size());
}
- public List<TermAndBoost> getSynonyms(
+ public Collection<TermAndBoost> getSynonyms(
Review Comment:
Not sure if this is a problematic backwards-compatible change, but if we
only apply that to main branch, no problem at all. The synonyms are not
ordered, aren't they? If the order is important, then `Collection` is not best
choice. Maybe then expose as `Deque` instead of list.
I have no idea how many people implement custom synonym providers. In any
case it is marked `@lucene.experimental`.
##########
lucene/highlighter/src/java/org/apache/lucene/search/vectorhighlight/FieldPhraseList.java:
##########
Review Comment:
should possible be marked `@lucene.experimental`
--
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]
