[
https://issues.apache.org/jira/browse/LUCENE-4167?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13403181#comment-13403181
]
David Smiley commented on LUCENE-4167:
--------------------------------------
bq. We don't need SpatialArgs at the moment. It feels like a hack to allow us
to add whatever arguments we want without having to change method signature and
so we can have some defaults.
The command pattern isn't a hack, I find it rather appropriate in this case,
and I remember thinking it was a brilliant move by Ryan to have used it when I
first saw it in Strategy for the first time. The distance precision is kind of
a "hint" that is optional. On the other hand, the shape & operation are
fundamental. I agree putting the operation into the method name does indeed
make it clear which operations are supported, which is good for clarity and
compile-time safety. But the compile-time safety is only realized if used
directly by client code, not when the client is remote and passes a query
string that is parsed and evaluated against a Strategy. This is what happens
in Solr, in some of our testing, and I expect similarly for potential users
like ElasticSearch when that eventually happens.
Curious, how do you propose compile-time safety be added to see what shape(s) a
Strategy indexes? And what about multi-value compile-time checks?
bq. Now I think about it, what exactly are the purposes of makeQuery and
makeValueSource()? I don't think it's clear.
I'll copy-paste the javadocs for you here:
{code:java}
/**
* Make a query which has a score based on the distance from the data to the
query shape.
* The default implementation constructs a {@link FilteredQuery} based on
* {@link #makeFilter(com.spatial4j.core.query.SpatialArgs,
SpatialFieldInfo)} and
* {@link #makeValueSource(com.spatial4j.core.query.SpatialArgs,
SpatialFieldInfo)}.
*/
public Query makeQuery(SpatialArgs args, T fieldInfo) {
{code}
{code:java}
/**
* The value source yields a number that is proportional to the distance
between the query shape and indexed data.
* @param args
* @param fieldInfo
*/
public abstract ValueSource makeValueSource(SpatialArgs args, T fieldInfo);
{code}
You rightly point out that the javadocs for makeValueSource is a bit vague. I
thought it would be best to leave some wiggle room for different ways of
calculating the distance instead of being precise. Do you think we should
spell it out exactly and thus leave no room for alternatives? In terms of its
relationship with makeQuery(), I think it makes sense to effectively fix the
Strategy specification to essentially be what makeQuery()'s default impl does
now (now as of last night anyway). So yes, the filter & query's matching
documents should always be the same. Adding documentation to this affect would
be good; one has to infer that at the moment based on the existing docs.
Here's an idea: what if makeQuery() goes away? Why is it needed when it can be
constructed by a client quite simply like it is now:
{code:java}
public Query makeQuery(SpatialArgs args, T fieldInfo) {
Filter filter = makeFilter(args, fieldInfo);
ValueSource vs = makeValueSource(args, fieldInfo);
return new FilteredQuery(new FunctionQuery(vs), filter);
}
{code}
If it goes away, my acceptance level of putting the operation name into the
method would be higher since you'd only have makeXXXXFilter without a Query
variant.
{code}
> Remove the use of SpatialOperation
> ----------------------------------
>
> Key: LUCENE-4167
> URL: https://issues.apache.org/jira/browse/LUCENE-4167
> Project: Lucene - Java
> Issue Type: Bug
> Components: modules/spatial
> Reporter: Chris Male
> Attachments: LUCENE-4167.patch
>
>
> Looking at the code in TwoDoublesStrategy I noticed
> SpatialOperations.BBoxWithin vs isWithin which confused me. Looking over the
> other Strategys I see that really only isWithin and Intersects is supported.
> Only TwoDoublesStrategy supports IsDisjointTo. The remainder of
> SpatialOperations are not supported.
> I don't think we should use SpatialOperation as this stage since it is not
> clear what Operations are supported by what Strategys, many Operations are
> not supported, and the code for handling the Operations is usually the same.
> We can spin off the code for TwoDoublesStrategy's IsDisjointTo support into a
> different Strategy.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
