Github user dennisgove commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/171#discussion_r110244491
--- Diff: solr/core/src/java/org/apache/solr/handler/StreamHandler.java ---
@@ -191,7 +237,20 @@ public void inform(SolrCore core) {
.withFunctionName("lteq", LessThanEqualToEvaluator.class)
.withFunctionName("not", NotEvaluator.class)
.withFunctionName("or", OrEvaluator.class)
-
+
+ // Date Time Evaluators
+ .withFunctionName(TemporalEvaluatorYear.FUNCTION_NAME,
TemporalEvaluatorYear.class)
--- End diff --
I'm having trouble making up my mind about this use of the FUNCTION_NAME
static property. For a while I've been considering moving the function name
into the implementing class (like you've done here) but I haven't been able to
convince myself that it is inherently better. I'd like to discuss it though, so
I'll list my reasons both for and against.
1. Function names were originally assigned in a single place, the
StreamHandler, to allow for easy overrides via the config file (eg. using
business specific logic for an innerJoin you could override the assignment of
`innerJoin` to your own class via the config file). Having the assignments in a
single place made this easier.
2. Having assignments in a single place makes it easy to see what the full
list of available function names is. Among other things, this has helped
prevent me from accidentally using an already used function name for a
different class.
3. Function names are now being assigned in at least 3 places
([StreamHandler](https://github.com/dennisgove/lucene-solr/blob/master/solr/core/src/java/org/apache/solr/handler/StreamHandler.java),
[GraphHandler](https://github.com/dennisgove/lucene-solr/blob/master/solr/core/src/java/org/apache/solr/handler/GraphHandler.java),
and
[SolrTable](https://github.com/dennisgove/lucene-solr/blob/master/solr/core/src/java/org/apache/solr/handler/sql/SolrTable.java))
so all the benefits of a single assignment is somewhat out the window.
4. By assigning the function names in the implementing class, we no longer
need to rely on the StreamFactory for knowing what the assigned function name
for a class is. This means we can use the function name in more logging and
error messages where a StreamFactory instance may not be readily available.
5. It will still be possible for people to override the default assigned
function names and classes, but the logic that currently does that will have to
be carefully changed to ensure full backward compatibility.
I'm in favor of making this change, but it's larger than something that
should probably be done in this PR.
@joel-bernstein, @covolution - your thoughts?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
