OK. So I think I fixed the whitespace problem.
Thanks for explaining the problem with interfaces -- that makes sense
now. I assume that EventListener and RequestHandler are interfaces
because they've been thought long and hard about and are not going to
change?
My first try at the patch was just to include the public methods, which
are the ones you list:
.initialize(Config)
.isHighlightEnabled(SolrParams)
.doHighlighting(...)
.getHighlightFields(...)
I discovered that the unit tests call the formatters and fragmenters
directly so in the interface version I made public get methods for
these. Now that we're using an abstract class I am able to just include
these as is - so no changes to HighlighterTest this time. But speaking
of unit tests... Anecdotally I know that the SolrCore changes allow the
highlighter to be configured (my custom highlighter). I guess I should
write a unit test that ensures this works. I'll do that now.
After doing some thinking I decided to leave the default implementation
of isHighlightingEnabled(SolrParams), and getHighlightFields(...) in the
abstract class because both methods deal with reading parameters. I
can't think of a use case of a highlighter that wouldn't use this or at
worst ignore/override this method. Is this a reasonable decision?
I wasn't sure what to do with the logger, so I left it in the
AbstractSolrHighlighter.
Thoughts?
Tricia
p.s. I've attached the updated patch since JIRA appears to be down.
Mike Klaas wrote:
Hi Tricia,
I don't suggest removing whitespace by hand--the best way to avoid the
situation is to make sure that your editor does not automatically edit
the whitespace of a file (some good-intentioned editors do this).
The maintenance concern with interfaces is that if we want to add a
method to the highlighter contract, doing so will break all custom
implementors of the interface. If it is an abstract class that must
be subclassed, we can add a default implementation to the new method
without breaking everyone's code.
As for the contract, I was going to review the proposed patch to see
what the interface consists of, but I think that all that is required is:
.initialize(Config)
.isHighlightEnabled(SolrParams)
.doHighlighting(...)
.getHighlightFields(...)
(which is probably what you had in your patch already---JIRA seems
down at the moment so I can't check).
cheers,
-Mike
On 5-Mar-08, at 2:49 PM, Tricia Williams wrote:
Thanks to Grant and Mike for the feedback! It is much appreciated.
Is there a quick and easy way to check for unnecessary whitespace
changes? It isn't that hard for me to go through the patch by hand
to find and remove them, but if there is an easier way I'm happy to
hear it.
I had taken the suggestion that Eli gave somewhat literally and made
SolrHighlighter an interface like RequestHandler. In the SolrCore
there are three existing objects that are configured:
SolrEventListener, SolrRequestHandler, and UpdateHandler. The first
two are interfaces and the third is an abstract class. While I'm
sure the maintenance concern is legitimate, I'm not sure what the
maintenance concern is - could someone elaborate?
I'll take your advice and make an AbstractSolrHighlighter that
SolrHighlighter (and my custom highlighter) extends. I noticed that
some of the other configurable objects implement SolrInfoMBean. Is
this something that the SolrHighlighter/AbstractSolrHighlighter
should also do?
Thanks,
Tricia
Mike Klaas (JIRA) wrote:
[
https://issues.apache.org/jira/browse/SOLR-386?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12574337#action_12574337
]
Mike Klaas commented on SOLR-386:
---------------------------------
Hi Tricia,
I'm not sure that I would ever use SolrHighlighter overriding (if I
had the need, I probably would just make a separate search
component). However, several people want this functionality and it
has relatively low implementation/maintenance cost.
There are a few things that need to be done to get the patch
committed. First, the unnecessary whitespace changes in SolrCore
shouldn't be in the diff (it makes it really hard to see the
changes, and might make it hard to apply/revert). Also, I'm
skeptical of using interfaces for this type of thing, for
maintenance reasons. Perhaps we could move to one of the approaches
that Grant suggested?
Thanks again for the contribution, and sorry it took so long
Add confuguration to specify SolrHighlighter implementation
-----------------------------------------------------------
Key: SOLR-386
URL: https://issues.apache.org/jira/browse/SOLR-386
Project: Solr
Issue Type: Improvement
Components: highlighter
Affects Versions: 1.3
Reporter: Eli Levine
Assignee: Mike Klaas
Attachments: SOLR-386-SolrHighlighter.patch,
SOLR-386-SolrHighlighter.patch, SOLR-386-SolrHighlighter.patch,
SOLR-386-SolrHighlighter.patch
It would be great if SolrCore allowed the highlighter class to be
configurable. A good way would be to add a +class+ attribute to
the <highlighting> element in solrconfig.xml, similar to how the
RequestHandler instance is initialized in SorCore.
Index: CHANGES.txt
===================================================================
--- CHANGES.txt (revision 634072)
+++ CHANGES.txt (working copy)
@@ -541,6 +541,11 @@
35. SOLR-249: Deprecated SolrException( int, ... ) constructors in favor
of constructors that takes an ErrorCode enum. This will ensure that
all SolrExceptions use a valid HTTP status code. (ryan)
+
+xx. SOLR-386: Abstracted SolrHighlighter to AbstractSolrHighlighter and
+ extended in SolrHighlighter. Adjusted SolrCore and solrconfig.xml so
+ that highlighter is configurable via a class attribute. Allows users
+ to use their own highlighter implementation. (Tricia Williams)
Changes in runtime behavior
1. Highlighting using DisMax will only pick up terms from the main
Index: src/test/test-files/solr/conf/solrconfig.xml
===================================================================
--- src/test/test-files/solr/conf/solrconfig.xml (revision 634072)
+++ src/test/test-files/solr/conf/solrconfig.xml (working copy)
@@ -318,7 +318,7 @@
</requestHandler>
- <highlighting>
+ <highlighting class="org.apache.solr.highlight.SolrHighlighter">
<!-- Configure the standard fragmenter -->
<fragmenter name="gap" class="org.apache.solr.highlight.GapFragmenter" default="true">
<lst name="defaults">
Index: src/java/org/apache/solr/highlight/AbstractSolrHighlighter.java
===================================================================
--- src/java/org/apache/solr/highlight/AbstractSolrHighlighter.java (revision 0)
+++ src/java/org/apache/solr/highlight/AbstractSolrHighlighter.java (revision 0)
@@ -0,0 +1,95 @@
+package org.apache.solr.highlight;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.logging.Logger;
+
+import org.apache.lucene.search.Query;
+import org.apache.lucene.search.highlight.Formatter;
+import org.apache.lucene.search.highlight.Fragmenter;
+import org.apache.lucene.search.highlight.Highlighter;
+import org.apache.lucene.search.highlight.QueryScorer;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.params.HighlightParams;
+import org.apache.solr.common.params.SolrParams;
+import org.apache.solr.common.util.NamedList;
+import org.apache.solr.core.Config;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.search.DocList;
+import org.apache.solr.util.SolrPluginUtils;
+
+public abstract class AbstractSolrHighlighter
+{
+ public static Logger log = Logger.getLogger(AbstractSolrHighlighter.class.getName());
+
+ // Thread safe registry
+ protected final Map<String,SolrFormatter> formatters =
+ Collections.synchronizedMap( new HashMap<String, SolrFormatter>() );
+
+ // Thread safe registry
+ protected final Map<String,SolrFragmenter> fragmenters =
+ Collections.synchronizedMap( new HashMap<String, SolrFragmenter>() );
+
+ public abstract void initalize( final Config config );
+
+
+ /**
+ * Check whether Highlighting is enabled for this request.
+ * @param params The params controlling Highlighting
+ * @return <code>true</code> if highlighting enabled, <code>false</code> if not.
+ */
+ public boolean isHighlightingEnabled(SolrParams params) {
+ return params.getBool(HighlightParams.HIGHLIGHT, false);
+ }
+
+ /**
+ * Return a String array of the fields to be highlighted.
+ * Falls back to the programatic defaults, or the default search field if the list of fields
+ * is not specified in either the handler configuration or the request.
+ * @param query The current Query
+ * @param request The current SolrQueryRequest
+ * @param defaultFields Programmatic default highlight fields, used if nothing is specified in the handler config or the request.
+ */
+ public String[] getHighlightFields(Query query, SolrQueryRequest request, String[] defaultFields) {
+ String fields[] = request.getParams().getParams(HighlightParams.FIELDS);
+
+ // if no fields specified in the request, or the handler, fall back to programmatic default, or default search field.
+ if(emptyArray(fields)) {
+ // use default search field if highlight fieldlist not specified.
+ if (emptyArray(defaultFields)) {
+ String defaultSearchField = request.getSchema().getSolrQueryParser(null).getField();
+ fields = null == defaultSearchField ? new String[]{} : new String[]{defaultSearchField};
+ }
+ else {
+ fields = defaultFields;
+ }
+ }
+ else if (fields.length == 1) {
+ // if there's a single request/handler value, it may be a space/comma separated list
+ fields = SolrPluginUtils.split(fields[0]);
+ }
+
+ return fields;
+ }
+
+ protected boolean emptyArray(String[] arr) {
+ return (arr == null || arr.length == 0 || arr[0] == null || arr[0].trim().length() == 0);
+ }
+
+ /**
+ * Generates a list of Highlighted query fragments for each item in a list
+ * of documents, or returns null if highlighting is disabled.
+ *
+ * @param docs query results
+ * @param query the query
+ * @param req the current request
+ * @param defaultFields default list of fields to summarize
+ *
+ * @return NamedList containing a NamedList for each document, which in
+ * turns contains sets (field, summary) pairs.
+ */
+ @SuppressWarnings("unchecked")
+ public abstract NamedList<Object> doHighlighting(DocList docs, Query query, SolrQueryRequest req, String[] defaultFields) throws IOException;
+}
Index: src/java/org/apache/solr/highlight/SolrHighlighter.java
===================================================================
--- src/java/org/apache/solr/highlight/SolrHighlighter.java (revision 634072)
+++ src/java/org/apache/solr/highlight/SolrHighlighter.java (working copy)
@@ -62,18 +62,9 @@
*
* @since solr 1.3
*/
-public class SolrHighlighter
+public class SolrHighlighter extends AbstractSolrHighlighter
{
- public static Logger log = Logger.getLogger(SolrHighlighter.class.getName());
- // Thread safe registry
- protected final Map<String,SolrFormatter> formatters =
- Collections.synchronizedMap( new HashMap<String, SolrFormatter>() );
-
- // Thread safe registry
- protected final Map<String,SolrFragmenter> fragmenters =
- Collections.synchronizedMap( new HashMap<String, SolrFragmenter>() );
-
public void initalize( final Config config )
{
formatters.clear();
@@ -102,15 +93,6 @@
/**
- * Check whether Highlighting is enabled for this request.
- * @param params The params controlling Highlighting
- * @return <code>true</code> if highlighting enabled, <code>false</code> if not.
- */
- public boolean isHighlightingEnabled(SolrParams params) {
- return params.getBool(HighlightParams.HIGHLIGHT, false);
- }
-
- /**
* Return a Highlighter appropriate for this field.
* @param query The current Query
* @param fieldName The name of the field
@@ -145,40 +127,6 @@
}
/**
- * Return a String array of the fields to be highlighted.
- * Falls back to the programatic defaults, or the default search field if the list of fields
- * is not specified in either the handler configuration or the request.
- * @param query The current Query
- * @param request The current SolrQueryRequest
- * @param defaultFields Programmatic default highlight fields, used if nothing is specified in the handler config or the request.
- */
- public String[] getHighlightFields(Query query, SolrQueryRequest request, String[] defaultFields) {
- String fields[] = request.getParams().getParams(HighlightParams.FIELDS);
-
- // if no fields specified in the request, or the handler, fall back to programmatic default, or default search field.
- if(emptyArray(fields)) {
- // use default search field if highlight fieldlist not specified.
- if (emptyArray(defaultFields)) {
- String defaultSearchField = request.getSchema().getDefaultSearchFieldName();
- fields = null == defaultSearchField ? new String[]{} : new String[]{defaultSearchField};
- }
- else {
- fields = defaultFields;
- }
- }
- else if (fields.length == 1) {
- // if there's a single request/handler value, it may be a space/comma separated list
- fields = SolrPluginUtils.split(fields[0]);
- }
-
- return fields;
- }
-
- protected boolean emptyArray(String[] arr) {
- return (arr == null || arr.length == 0 || arr[0] == null || arr[0].trim().length() == 0);
- }
-
- /**
* Return the max number of snippets for this field. If this has not
* been configured for this field, fall back to the configured default
* or the solr default.
Index: src/java/org/apache/solr/core/SolrCore.java
===================================================================
--- src/java/org/apache/solr/core/SolrCore.java (revision 634072)
+++ src/java/org/apache/solr/core/SolrCore.java (working copy)
@@ -305,8 +305,11 @@
private UpdateHandler createUpdateHandler(String className) {
return createInstance(className, UpdateHandler.class, "Update Handler");
}
-
+ private SolrHighlighter createHighlighter(String className) {
+ return createInstance(className, SolrHighlighter.class, "Highlighter");
+ }
+
/**
* @return the last core initialized. If you are using multiple cores,
* this is not a function to use.
@@ -379,8 +382,9 @@
reqHandlers = new RequestHandlers(this);
reqHandlers.initHandlersFromConfig( solrConfig );
- // TODO? could select the highlighter implementation
- highlighter = new SolrHighlighter();
+ highlighter = createHighlighter(
+ solrConfig.get("highlighting/@class", SolrHighlighter.class.getName())
+ );
highlighter.initalize( solrConfig );
try {
