dsmiley commented on a change in pull request #412:
URL: https://github.com/apache/lucene/pull/412#discussion_r739664697
##########
File path:
lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/UnifiedHighlighter.java
##########
@@ -165,6 +275,27 @@ public UnifiedHighlighter(IndexSearcher indexSearcher,
Analyzer indexAnalyzer) {
"indexAnalyzer is required" + " (even if in some circumstances it
isn't used)");
}
+ /**
+ * Constructs the highlighter with the given the {@link Builder}.
+ *
+ * @param builder - a {@link Builder} object.
+ */
+ public UnifiedHighlighter(Builder<?> builder) {
+ this.searcher = builder.searcher;
+ this.indexAnalyzer = Objects.requireNonNull(builder.indexAnalyzer);
+ this.flags = builder.flags;
Review comment:
I propose a builder.getFlags() so you can compute the appropriate flags
there, and someone could even override the builder there if they choose.
##########
File path:
lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/UnifiedHighlighter.java
##########
@@ -1163,10 +1299,18 @@ public CacheHelper getReaderCacheHelper() {
/** Flags for controlling highlighting behavior. */
public enum HighlightFlag {
- /** @see UnifiedHighlighter#setHighlightPhrasesStrictly(boolean) */
+ /**
+ * Here position sensitive queries (e.g. phrases and {@link SpanQuery}ies)
are highlighted
+ * strictly based on query matches (slower) versus any/all occurrences of
the underlying terms.
+ * By default it's enabled, but there's no overhead if such queries aren't
used.
+ */
PHRASES,
- /** @see UnifiedHighlighter#setHandleMultiTermQuery(boolean) */
+ /**
Review comment:
I think these could be javadoc references to the setters on the builder?
##########
File path:
lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/UnifiedHighlighter.java
##########
@@ -207,18 +342,22 @@ public void setFieldMatcher(Predicate<String> predicate) {
}
/**
- * Returns whether {@link MultiTermQuery} derivatives will be highlighted.
By default it's
- * enabled. MTQ highlighting can be expensive, particularly when using
offsets in postings.
+ * Returns the predicate to use for extracting the query part that must be
highlighted. By default
+ * only queries that target the current field are kept. (AKA
requireFieldMatch)
*/
+ protected Predicate<String> getFieldMatcher(String field) {
Review comment:
assuming we move the default selection logic to the builder, this can be
implemented like a getter; no?
##########
File path:
lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/UnifiedHighlighter.java
##########
@@ -165,6 +275,27 @@ public UnifiedHighlighter(IndexSearcher indexSearcher,
Analyzer indexAnalyzer) {
"indexAnalyzer is required" + " (even if in some circumstances it
isn't used)");
}
+ /**
+ * Constructs the highlighter with the given the {@link Builder}.
+ *
+ * @param builder - a {@link Builder} object.
+ */
+ public UnifiedHighlighter(Builder<?> builder) {
+ this.searcher = builder.searcher;
+ this.indexAnalyzer = Objects.requireNonNull(builder.indexAnalyzer);
+ this.flags = builder.flags;
+ this.defaultHandleMtq = builder.handleMultiTermQuery;
+ this.defaultHighlightPhrasesStrictly = builder.highlightPhrasesStrictly;
+ this.defaultPassageRelevancyOverSpeed = builder.passageRelevancyOverSpeed;
Review comment:
no; I don't think we want all these booleans on UnifiedHighlighter when
we have the flags. The booleans can stay on the builder and inform how the
flags there is computed.
##########
File path:
lucene/highlighter/src/java/org/apache/lucene/search/uhighlight/UnifiedHighlighter.java
##########
@@ -182,6 +313,10 @@ public void setMaxLength(int maxLength) {
this.maxLength = maxLength;
}
+ public void setFlags(Set<HighlightFlag> flags) {
Review comment:
Uh; no, UH is now mutable again. I'm dubious we even want this on the
builder since it's more of an advanced case to customize these flags. Someone
can always subclass the getter to customize.
--
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
