benwtrent commented on code in PR #14154:
URL: https://github.com/apache/lucene/pull/14154#discussion_r1925341296
##########
lucene/suggest/src/java/org/apache/lucene/search/suggest/document/CompletionAnalyzer.java:
##########
@@ -112,6 +116,25 @@ public CompletionAnalyzer(
ConcatenateGraphFilter.DEFAULT_MAX_GRAPH_EXPANSIONS);
}
+ /**
+ * Creates CompletionAnalyzer with the given analyzer, preserving token
separation, position
+ * increments, and using the {@link
+ * org.apache.lucene.analysis.AnalyzerWrapper.WrappingReuseStrategy} reuse
strategy
+ */
+ public CompletionAnalyzer(
+ Analyzer analyzer,
+ boolean preserveSep,
+ boolean preservePositionIncrements,
+ ReuseStrategy fallbackStrategy) {
+ super(new WrappingReuseStrategy(fallbackStrategy));
+ // häckidy-hick-hack, because we cannot call super() with a reference to
"this":
+ ((WrappingReuseStrategy) getReuseStrategy()).setUp(this, analyzer,
wrappedComponents);
+ this.analyzer = analyzer;
+ this.preserveSep = preserveSep;
+ this.preservePositionIncrements = preservePositionIncrements;
+ this.maxGraphExpansions =
ConcatenateGraphFilter.DEFAULT_MAX_GRAPH_EXPANSIONS;
+ }
Review Comment:
```
/**
* JAVA DOCS about when you should use this and what it does
*/
public static CompletionAnalyzer withWrappingReuseStrategy(
Analyzer analyzer,
boolean preserveSep,
boolean preservePositionIncrements,
ReuseStrategy fallbackStrategy) {
CompletionAnalyzer completionAnalyzer = new CompletionAnalyzer(analyzer,
preserveSep, preservePositionIncrements, fallbackStrategy);
((WrappingReuseStrategy)completionAnalyzer.getReuseStrategy()).setUp(completionAnalyzer,
analyzer, completionAnalyzer.wrappedComponents);
return completionAnalyzer;
}
```
What if we made the ctor private, removed the setup hack and did it instead
within a public static method?
##########
lucene/core/src/java/org/apache/lucene/analysis/AnalyzerWrapper.java:
##########
@@ -151,4 +152,57 @@ protected final Reader initReaderForNormalization(String
fieldName, Reader reade
protected final AttributeFactory attributeFactory(String fieldName) {
return getWrappedAnalyzer(fieldName).attributeFactory(fieldName);
}
+
+ /**
+ * A {@link org.apache.lucene.analysis.Analyzer.ReuseStrategy} that checks
the wrapped analyzer's
+ * strategy for reusability. If the wrapped analyzer's strategy returns
null, components need to
+ * be re-created. During components creation, this analyzer must store the
wrapped analyzer's
+ * components in {@code wrappedComponents} local thread variable.
+ */
+ public static final class WrappingReuseStrategy extends ReuseStrategy {
+ private AnalyzerWrapper wrapper;
+ private Analyzer wrappedAnalyzer;
+ private CloseableThreadLocal<TokenStreamComponents> wrappedComponents;
+ private final ReuseStrategy fallbackStrategy;
+
+ public WrappingReuseStrategy(ReuseStrategy fallbackStrategy) {
+ this.fallbackStrategy = fallbackStrategy;
+ }
+
+ public void setUp(
+ AnalyzerWrapper wrapper,
+ Analyzer wrappedAnalyzer,
+ CloseableThreadLocal<TokenStreamComponents> wrappedComponents) {
+ this.wrapper = wrapper;
+ this.wrappedAnalyzer = wrappedAnalyzer;
+ this.wrappedComponents = wrappedComponents;
+ }
+
+ @Override
+ public TokenStreamComponents getReusableComponents(Analyzer analyzer,
String fieldName) {
+ if (analyzer == wrapper) {
+ if
(wrappedAnalyzer.getReuseStrategy().getReusableComponents(wrappedAnalyzer,
fieldName)
+ == null) {
+ return null;
+ } else {
+ return (TokenStreamComponents) getStoredValue(analyzer);
+ }
+ } else {
+ return fallbackStrategy.getReusableComponents(analyzer, fieldName);
+ }
+ }
+
+ @Override
+ public void setReusableComponents(
+ Analyzer analyzer, String fieldName, TokenStreamComponents components)
{
+ if (analyzer == wrapper) {
+ setStoredValue(analyzer, components);
+ wrappedAnalyzer
+ .getReuseStrategy()
+ .setReusableComponents(wrappedAnalyzer, fieldName,
wrappedComponents.get());
+ } else {
+ fallbackStrategy.setReusableComponents(analyzer, fieldName,
components);
+ }
Review Comment:
Do we really need to know the instance, or can we do `Analyzer analyzer
instanceof WrapperAnalyzer wrapperAnalyzer` then get the wrapped analyzer via
`getWrappedAnalyzer(fieldName)`?
##########
lucene/core/src/java/org/apache/lucene/analysis/AnalyzerWrapper.java:
##########
@@ -151,4 +152,57 @@ protected final Reader initReaderForNormalization(String
fieldName, Reader reade
protected final AttributeFactory attributeFactory(String fieldName) {
return getWrappedAnalyzer(fieldName).attributeFactory(fieldName);
}
+
+ /**
+ * A {@link org.apache.lucene.analysis.Analyzer.ReuseStrategy} that checks
the wrapped analyzer's
+ * strategy for reusability. If the wrapped analyzer's strategy returns
null, components need to
+ * be re-created. During components creation, this analyzer must store the
wrapped analyzer's
+ * components in {@code wrappedComponents} local thread variable.
+ */
+ public static final class WrappingReuseStrategy extends ReuseStrategy {
+ private AnalyzerWrapper wrapper;
+ private Analyzer wrappedAnalyzer;
+ private CloseableThreadLocal<TokenStreamComponents> wrappedComponents;
+ private final ReuseStrategy fallbackStrategy;
+
+ public WrappingReuseStrategy(ReuseStrategy fallbackStrategy) {
+ this.fallbackStrategy = fallbackStrategy;
+ }
+
+ public void setUp(
+ AnalyzerWrapper wrapper,
+ Analyzer wrappedAnalyzer,
+ CloseableThreadLocal<TokenStreamComponents> wrappedComponents) {
+ this.wrapper = wrapper;
+ this.wrappedAnalyzer = wrappedAnalyzer;
+ this.wrappedComponents = wrappedComponents;
+ }
Review Comment:
This bugs me. We allow a reuse strategy to be mutated after construction. It
seems ready to cause a bug.
##########
lucene/core/src/java/org/apache/lucene/analysis/AnalyzerWrapper.java:
##########
@@ -151,4 +152,57 @@ protected final Reader initReaderForNormalization(String
fieldName, Reader reade
protected final AttributeFactory attributeFactory(String fieldName) {
return getWrappedAnalyzer(fieldName).attributeFactory(fieldName);
}
+
+ /**
+ * A {@link org.apache.lucene.analysis.Analyzer.ReuseStrategy} that checks
the wrapped analyzer's
+ * strategy for reusability. If the wrapped analyzer's strategy returns
null, components need to
+ * be re-created. During components creation, this analyzer must store the
wrapped analyzer's
+ * components in {@code wrappedComponents} local thread variable.
+ */
+ public static final class WrappingReuseStrategy extends ReuseStrategy {
Review Comment:
It seems to me that the better name is `UnWrapping` as if we find that the
analyzer is a wrapped analyzer, we unwrap it and use the underlying analyzer?
##########
lucene/core/src/java/org/apache/lucene/analysis/AnalyzerWrapper.java:
##########
@@ -151,4 +152,57 @@ protected final Reader initReaderForNormalization(String
fieldName, Reader reade
protected final AttributeFactory attributeFactory(String fieldName) {
return getWrappedAnalyzer(fieldName).attributeFactory(fieldName);
}
+
+ /**
+ * A {@link org.apache.lucene.analysis.Analyzer.ReuseStrategy} that checks
the wrapped analyzer's
+ * strategy for reusability. If the wrapped analyzer's strategy returns
null, components need to
+ * be re-created. During components creation, this analyzer must store the
wrapped analyzer's
+ * components in {@code wrappedComponents} local thread variable.
+ */
+ public static final class WrappingReuseStrategy extends ReuseStrategy {
+ private AnalyzerWrapper wrapper;
+ private Analyzer wrappedAnalyzer;
+ private CloseableThreadLocal<TokenStreamComponents> wrappedComponents;
+ private final ReuseStrategy fallbackStrategy;
+
+ public WrappingReuseStrategy(ReuseStrategy fallbackStrategy) {
+ this.fallbackStrategy = fallbackStrategy;
+ }
+
+ public void setUp(
+ AnalyzerWrapper wrapper,
+ Analyzer wrappedAnalyzer,
+ CloseableThreadLocal<TokenStreamComponents> wrappedComponents) {
+ this.wrapper = wrapper;
+ this.wrappedAnalyzer = wrappedAnalyzer;
+ this.wrappedComponents = wrappedComponents;
+ }
+
+ @Override
+ public TokenStreamComponents getReusableComponents(Analyzer analyzer,
String fieldName) {
+ if (analyzer == wrapper) {
Review Comment:
Do we really need to know the instance, or can we do `Analyzer analyzer
instanceof WrapperAnalyzer wrapperAnalyzer` then get the strategy from
`wrapperAnalyzer.getWrappedAnalyzer(fieldName).getReuseStategy().getReusableComponents...`?
##########
lucene/core/src/java/org/apache/lucene/analysis/AnalyzerWrapper.java:
##########
@@ -151,4 +152,57 @@ protected final Reader initReaderForNormalization(String
fieldName, Reader reade
protected final AttributeFactory attributeFactory(String fieldName) {
return getWrappedAnalyzer(fieldName).attributeFactory(fieldName);
}
+
+ /**
+ * A {@link org.apache.lucene.analysis.Analyzer.ReuseStrategy} that checks
the wrapped analyzer's
+ * strategy for reusability. If the wrapped analyzer's strategy returns
null, components need to
+ * be re-created. During components creation, this analyzer must store the
wrapped analyzer's
+ * components in {@code wrappedComponents} local thread variable.
+ */
+ public static final class WrappingReuseStrategy extends ReuseStrategy {
+ private AnalyzerWrapper wrapper;
+ private Analyzer wrappedAnalyzer;
+ private CloseableThreadLocal<TokenStreamComponents> wrappedComponents;
+ private final ReuseStrategy fallbackStrategy;
+
+ public WrappingReuseStrategy(ReuseStrategy fallbackStrategy) {
+ this.fallbackStrategy = fallbackStrategy;
+ }
+
+ public void setUp(
+ AnalyzerWrapper wrapper,
+ Analyzer wrappedAnalyzer,
+ CloseableThreadLocal<TokenStreamComponents> wrappedComponents) {
+ this.wrapper = wrapper;
+ this.wrappedAnalyzer = wrappedAnalyzer;
+ this.wrappedComponents = wrappedComponents;
+ }
Review Comment:
Ah, I see that the "wrappedComponents" is key here. its tricky, as the
analyzer provides access directly to `storedValue` and there is no way to
override :(.
That seems like a bad API already, but we don't want to change all these
long lived interfaces unnecessarily.
--
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: issues-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
