lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Smiley (JIRA)" <>
Subject [jira] [Commented] (LUCENE-4167) Remove the use of SpatialOperation
Date Thu, 28 Jun 2012 16:01:43 GMT


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:
   * 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) {

   * 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);

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:
  public Query makeQuery(SpatialArgs args, T fieldInfo) {
    Filter filter = makeFilter(args, fieldInfo);
    ValueSource vs = makeValueSource(args, fieldInfo);
    return new FilteredQuery(new FunctionQuery(vs), filter);

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.
> Remove the use of SpatialOperation
> ----------------------------------
>                 Key: LUCENE-4167
>                 URL:
>             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:!default.jspa
For more information on JIRA, see:


To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message