LUCENE-4644: Spatial IsWithin predicate for RecursivePrefixTreeStrategy

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1461581 13f79535-47bb-0310-9956-ffa450edef68

lucene/CHANGES.txt

@@ -139,6 +139,11 @@ New Features
 * LUCENE-4839: Added SorterTemplate.timSort, a O(n log n) stable sort algorithm
   that performs well on partially sorted data. (Adrien Grand)
 
+* LUCENE-4644: Added support for the "IsWithin" spatial predicate for
+  RecursivePrefixTreeStrategy. It's for matching non-point indexed shapes; if
+  you only have points (1/doc) then "Intersects" is equivalent and faster.
+  See the javadocs.  (David Smiley)
+
 Optimizations
 
 * LUCENE-4839: SorterTemplate.merge can now be overridden in order to replace

lucene/spatial/src/java/org/apache/lucene/spatial/prefix/AbstractPrefixTreeFilter.java

@@ -27,14 +27,14 @@ import org.apache.lucene.search.DocIdSetIterator;
 import org.apache.lucene.search.Filter;
 import org.apache.lucene.spatial.prefix.tree.SpatialPrefixTree;
 import org.apache.lucene.util.Bits;
-import org.apache.lucene.util.OpenBitSet;
+import org.apache.lucene.util.FixedBitSet;
 
 import java.io.IOException;
 
 /**
  * Base class for Lucene Filters on SpatialPrefixTree fields.
  *
- * @lucene.internal
+ * @lucene.experimental
  */
 public abstract class AbstractPrefixTreeFilter extends Filter {
 
@@ -93,13 +93,13 @@ public abstract class AbstractPrefixTreeFilter extends Filter {
         this.termsEnum = terms.iterator(null);
     }
 
-    protected void collectDocs(OpenBitSet bitSet) throws IOException {
+    protected void collectDocs(FixedBitSet bitSet) throws IOException {
       //WARN: keep this specialization in sync
       assert termsEnum != null;
       docsEnum = termsEnum.docs(acceptDocs, docsEnum, DocsEnum.FLAG_NONE);
       int docid;
       while ((docid = docsEnum.nextDoc()) != DocIdSetIterator.NO_MORE_DOCS) {
-        bitSet.fastSet(docid);
+        bitSet.set(docid);
       }
     }
 

lucene/spatial/src/java/org/apache/lucene/spatial/prefix/AbstractVisitingPrefixTreeFilter.java

@@ -90,7 +90,7 @@ public abstract class AbstractVisitingPrefixTreeFilter extends AbstractPrefixTre
    * method then it's short-circuited until it finds one, at which point
    * {@link #visit(org.apache.lucene.spatial.prefix.tree.Node)} is called. At
    * some depths, of the tree, the algorithm switches to a scanning mode that
-   * finds calls {@link #visitScanned(org.apache.lucene.spatial.prefix.tree.Node, com.spatial4j.core.shape.Shape)}
+   * finds calls {@link #visitScanned(org.apache.lucene.spatial.prefix.tree.Node)}
    * for each leaf cell found.
    *
    * @lucene.internal
@@ -207,7 +207,6 @@ public abstract class AbstractVisitingPrefixTreeFilter extends AbstractPrefixTre
         throw new IllegalStateException("Spatial logic error");
 
       //Check for adjacent leaf (happens for indexed non-point shapes)
-      assert !cell.isLeaf();
       if (hasIndexedLeaves && cell.getLevel() != 0) {
         //If the next indexed term just adds a leaf marker ('+') to cell,
         // then add all of those docs
@@ -257,8 +256,7 @@ public abstract class AbstractVisitingPrefixTreeFilter extends AbstractPrefixTre
      * Scans ({@code termsEnum.next()}) terms until a term is found that does
      * not start with curVNode's cell. If it finds a leaf cell or a cell at
      * level {@code scanDetailLevel} then it calls {@link
-     * #visitScanned(org.apache.lucene.spatial.prefix.tree.Node,
-     * com.spatial4j.core.shape.Shape)}.
+     * #visitScanned(org.apache.lucene.spatial.prefix.tree.Node)}.
      */
     protected void scan(int scanDetailLevel) throws IOException {
       for (;
@@ -270,15 +268,7 @@ public abstract class AbstractVisitingPrefixTreeFilter extends AbstractPrefixTre
         if (termLevel > scanDetailLevel)
           continue;
         if (termLevel == scanDetailLevel || scanCell.isLeaf()) {
-          Shape cShape;
-          //if this cell represents a point, use the cell center vs the box
-          // (points never have isLeaf())
-          if (termLevel == grid.getMaxLevels() && !scanCell.isLeaf())
-            cShape = scanCell.getCenter();
-          else
-            cShape = scanCell.getShape();
-
-          visitScanned(scanCell, cShape);
+          visitScanned(scanCell);
         }
       }//term loop
     }
@@ -337,10 +327,8 @@ public abstract class AbstractVisitingPrefixTreeFilter extends AbstractPrefixTre
     /**
      * The cell is either indexed as a leaf or is the last level of detail. It
      * might not even intersect the query shape, so be sure to check for that.
-     * Use {@code cellShape} instead of {@code cell.getCellShape} for the cell's
-     * shape.
      */
-    protected abstract void visitScanned(Node cell, Shape cellShape) throws IOException;
+    protected abstract void visitScanned(Node cell) throws IOException;
 
 
     protected void preSiblings(VNode vNode) throws IOException {

lucene/spatial/src/java/org/apache/lucene/spatial/prefix/IntersectsPrefixTreeFilter.java

@@ -24,7 +24,7 @@ import org.apache.lucene.search.DocIdSet;
 import org.apache.lucene.spatial.prefix.tree.Node;
 import org.apache.lucene.spatial.prefix.tree.SpatialPrefixTree;
 import org.apache.lucene.util.Bits;
-import org.apache.lucene.util.OpenBitSet;
+import org.apache.lucene.util.FixedBitSet;
 
 import java.io.IOException;
 
@@ -53,11 +53,11 @@ public class IntersectsPrefixTreeFilter extends AbstractVisitingPrefixTreeFilter
   @Override
   public DocIdSet getDocIdSet(AtomicReaderContext context, Bits acceptDocs) throws IOException {
     return new VisitorTemplate(context, acceptDocs, hasIndexedLeaves) {
-      private OpenBitSet results;
+      private FixedBitSet results;
 
       @Override
       protected void start() {
-        results = new OpenBitSet(maxDoc);
+        results = new FixedBitSet(maxDoc);
       }
 
       @Override
@@ -80,8 +80,16 @@ public class IntersectsPrefixTreeFilter extends AbstractVisitingPrefixTreeFilter
       }
 
       @Override
-      protected void visitScanned(Node cell, Shape cellShape) throws IOException {
-        if (queryShape.relate(cellShape).intersects())
+      protected void visitScanned(Node cell) throws IOException {
+        Shape cShape;
+        //if this cell represents a point, use the cell center vs the box
+        // TODO this behavior is debatable; might want to be configurable
+        // (points never have isLeaf())
+        if (cell.getLevel() == grid.getMaxLevels() && !cell.isLeaf())
+          cShape = cell.getCenter();
+        else
+          cShape = cell.getShape();
+        if (queryShape.relate(cShape).intersects())
           collectDocs(results);
       }
 

lucene/spatial/src/java/org/apache/lucene/spatial/prefix/RecursivePrefixTreeStrategy.java

@@ -62,17 +62,22 @@ public class RecursivePrefixTreeStrategy extends PrefixTreeStrategy {
 
   @Override
   public Filter makeFilter(SpatialArgs args) {
-    final SpatialOperation op = args.getOperation();
-    if (op != SpatialOperation.Intersects)
-      throw new UnsupportedSpatialOperation(op);
 
     Shape shape = args.getShape();
-
     int detailLevel = grid.getLevelForDistance(args.resolveDistErr(ctx, distErrPct));
+    final boolean hasIndexedLeaves = true;
 
-    return new IntersectsPrefixTreeFilter(
-        shape, getFieldName(), grid, detailLevel, prefixGridScanLevel,
-        true);//hasIndexedLeaves
+    final SpatialOperation op = args.getOperation();
+    if (op == SpatialOperation.Intersects) {
+      return new IntersectsPrefixTreeFilter(
+          shape, getFieldName(), grid, detailLevel, prefixGridScanLevel,
+          hasIndexedLeaves);
+    } else if (op == SpatialOperation.IsWithin) {
+      return new WithinPrefixTreeFilter(
+          shape, getFieldName(), grid, detailLevel, prefixGridScanLevel,
+          -1);//-1 flag is slower but ensures correct results
+    }
+    throw new UnsupportedSpatialOperation(op);
   }
 }
 

lucene/spatial/src/java/org/apache/lucene/spatial/prefix/WithinPrefixTreeFilter.java

@@ -0,0 +1,184 @@
+package org.apache.lucene.spatial.prefix;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import com.spatial4j.core.context.SpatialContext;
+import com.spatial4j.core.distance.DistanceUtils;
+import com.spatial4j.core.shape.Circle;
+import com.spatial4j.core.shape.Point;
+import com.spatial4j.core.shape.Rectangle;
+import com.spatial4j.core.shape.Shape;
+import com.spatial4j.core.shape.SpatialRelation;
+import org.apache.lucene.index.AtomicReaderContext;
+import org.apache.lucene.search.DocIdSet;
+import org.apache.lucene.spatial.prefix.tree.Node;
+import org.apache.lucene.spatial.prefix.tree.SpatialPrefixTree;
+import org.apache.lucene.util.Bits;
+import org.apache.lucene.util.FixedBitSet;
+
+import java.io.IOException;
+import java.util.Iterator;
+
+/**
+ * Finds docs where its indexed shape is {@link org.apache.lucene.spatial.query.SpatialOperation#IsWithin
+ * WITHIN} the query shape.  It works by looking at cells outside of the query
+ * shape to ensure documents there are excluded. By default, it will
+ * examine all cells, and it's fairly slow.  If you know that the indexed shapes
+ * are never comprised of multiple disjoint parts (which also means it is not multi-valued),
+ * then you can pass {@code SpatialPrefixTree.getDistanceForLevel(maxLevels)} as
+ * the {@code queryBuffer} constructor parameter to minimally look this distance
+ * beyond the query shape's edge.  Even if the indexed shapes are sometimes
+ * comprised of multiple disjoint parts, you might want to use this option with
+ * a large buffer as a faster approximation with minimal false-positives.
+ *
+ * @lucene.experimental
+ */
+//TODO LUCENE-4869: implement faster algorithm based on filtering out false-positives of a
+//  minimal query buffer by looking in a DocValues cache holding a representative
+//  point of each disjoint component of a document's shape(s).
+public class WithinPrefixTreeFilter extends AbstractVisitingPrefixTreeFilter {
+
+  private final Shape bufferedQueryShape;//if null then the whole world
+
+  /**
+   * See {@link AbstractVisitingPrefixTreeFilter#AbstractVisitingPrefixTreeFilter(com.spatial4j.core.shape.Shape, String, org.apache.lucene.spatial.prefix.tree.SpatialPrefixTree, int, int)}.
+   * {@code queryBuffer} is the (minimum) distance beyond the query shape edge
+   * where non-matching documents are looked for so they can be excluded. If
+   * -1 is used then the whole world is examined (a good default for correctness).
+   */
+  public WithinPrefixTreeFilter(Shape queryShape, String fieldName, SpatialPrefixTree grid,
+                                int detailLevel, int prefixGridScanLevel, double queryBuffer) {
+    super(queryShape, fieldName, grid, detailLevel, prefixGridScanLevel);
+    if (queryBuffer == -1)
+      this.bufferedQueryShape = null;
+    else
+      this.bufferedQueryShape = bufferShape(queryShape, queryBuffer);
+  }
+
+  /** Returns a new shape that is larger than shape by at distErr.
+   */
+  //TODO move this generic code elsewhere?  Spatial4j?
+  protected Shape bufferShape(Shape shape, double distErr) {
+    if (distErr <= 0)
+      throw new IllegalArgumentException("distErr must be > 0");
+    SpatialContext ctx = grid.getSpatialContext();
+    if (shape instanceof Point) {
+      return ctx.makeCircle((Point)shape, distErr);
+    } else if (shape instanceof Circle) {
+      Circle circle = (Circle) shape;
+      double newDist = circle.getRadius() + distErr;
+      if (ctx.isGeo() && newDist > 180)
+        newDist = 180;
+      return ctx.makeCircle(circle.getCenter(), newDist);
+    } else {
+      Rectangle bbox = shape.getBoundingBox();
+      double newMinX = bbox.getMinX() - distErr;
+      double newMaxX = bbox.getMaxX() + distErr;
+      double newMinY = bbox.getMinY() - distErr;
+      double newMaxY = bbox.getMaxY() + distErr;
+      if (ctx.isGeo()) {
+        if (newMinY < -90)
+          newMinY = -90;
+        if (newMaxY > 90)
+          newMaxY = 90;
+        if (newMinY == -90 || newMaxY == 90 || bbox.getWidth() + 2*distErr > 360) {
+          newMinX = -180;
+          newMaxX = 180;
+        } else {
+          newMinX = DistanceUtils.normLonDEG(newMinX);
+          newMaxX = DistanceUtils.normLonDEG(newMaxX);
+        }
+      } else {
+        //restrict to world bounds
+        newMinX = Math.max(newMinX, ctx.getWorldBounds().getMinX());
+        newMaxX = Math.min(newMaxX, ctx.getWorldBounds().getMaxX());
+        newMinY = Math.max(newMinY, ctx.getWorldBounds().getMinY());
+        newMaxY = Math.min(newMaxY, ctx.getWorldBounds().getMaxY());
+      }
+      return ctx.makeRectangle(newMinX, newMaxX, newMinY, newMaxY);
+    }
+  }
+
+
+  @Override
+  public DocIdSet getDocIdSet(AtomicReaderContext context, Bits acceptDocs) throws IOException {
+    return new VisitorTemplate(context, acceptDocs, true) {
+      private FixedBitSet inside;
+      private FixedBitSet outside;
+      private SpatialRelation visitRelation;
+
+      @Override
+      protected void start() {
+        inside = new FixedBitSet(maxDoc);
+        outside = new FixedBitSet(maxDoc);
+      }
+
+      @Override
+      protected DocIdSet finish() {
+        inside.andNot(outside);
+        return inside;
+      }
+
+      @Override
+      protected Iterator<Node> findSubCellsToVisit(Node cell) {
+        //use buffered query shape instead of orig.  Works with null too.
+        return cell.getSubCells(bufferedQueryShape).iterator();
+      }
+
+      @Override
+      protected boolean visit(Node cell) throws IOException {
+        //cell.relate is based on the bufferedQueryShape; we need to examine what
+        // the relation is against the queryShape
+        visitRelation = cell.getShape().relate(queryShape);
+        if (visitRelation == SpatialRelation.WITHIN) {
+          collectDocs(inside);
+          return false;
+        } else if (visitRelation == SpatialRelation.DISJOINT) {
+          collectDocs(outside);
+          return false;
+        } else if (cell.getLevel() == detailLevel) {
+          collectDocs(inside);
+          return false;
+        }
+        return true;
+      }
+
+      @Override
+      protected void visitLeaf(Node cell) throws IOException {
+        SpatialRelation relation = visitRelation;
+        assert visitRelation == cell.getShape().relate(queryShape);
+        if (relation.intersects()) {
+          collectDocs(inside);
+        } else {
+          collectDocs(outside);
+        }
+      }
+
+      @Override
+      protected void visitScanned(Node cell) throws IOException {
+        if (queryShape.relate(cell.getShape()).intersects()) {
+          collectDocs(inside);
+        } else {
+          collectDocs(outside);
+        }
+      }
+
+    }.getDocIdSet();
+  }
+
+}

lucene/spatial/src/java/org/apache/lucene/spatial/prefix/tree/SpatialPrefixTree.java

@@ -19,6 +19,7 @@ package org.apache.lucene.spatial.prefix.tree;
 
 import com.spatial4j.core.context.SpatialContext;
 import com.spatial4j.core.shape.Point;
+import com.spatial4j.core.shape.Rectangle;
 import com.spatial4j.core.shape.Shape;
 
 import java.nio.charset.Charset;
@@ -77,7 +78,26 @@ public abstract class SpatialPrefixTree {
    */
   public abstract int getLevelForDistance(double dist);
 
-  //TODO double getDistanceForLevel(int level)
+  /**
+   * Given a node having the specified level, returns the distance from opposite
+   * corners. Since this might very depending on where the node is, this method
+   * may over-estimate.
+   *
+   * @param level [1 to maxLevels]
+   * @return > 0
+   */
+  public double getDistanceForLevel(int level) {
+    if (level < 1 || level > getMaxLevels())
+      throw new IllegalArgumentException("Level must be in 1 to maxLevels range");
+    //TODO cache for each level
+    Node node = getNode(ctx.getWorldBounds().getCenter(), level);
+    Rectangle bbox = node.getShape().getBoundingBox();
+    double width = bbox.getWidth();
+    double height = bbox.getHeight();
+    //Use standard cartesian hypotenuse. For geospatial, this answer is larger
+    // than the correct one but it's okay to over-estimate.
+    return Math.sqrt(width * width + height * height);
+  }
 
   private transient Node worldNode;//cached
 
@@ -111,6 +131,9 @@ public abstract class SpatialPrefixTree {
     return target;
   }
 
+  /**
+   * Returns the cell containing point {@code p} at the specified {@code level}.
+   */
   protected Node getNode(Point p, int level) {
     return getNodes(p, level, false).get(0);
   }

lucene/spatial/src/java/org/apache/lucene/spatial/query/SpatialOperation.java

@@ -17,6 +17,7 @@ package org.apache.lucene.spatial.query;
  * limitations under the License.
  */
 
+import com.spatial4j.core.shape.Rectangle;
 import com.spatial4j.core.shape.Shape;
 import com.spatial4j.core.shape.SpatialRelation;
 
@@ -55,13 +56,14 @@ public abstract class SpatialOperation implements Serializable {
   public static final SpatialOperation BBoxWithin     = new SpatialOperation("BBoxWithin", true, false, false) {
     @Override
     public boolean evaluate(Shape indexedShape, Shape queryShape) {
-      return indexedShape.getBoundingBox().relate(queryShape) == SpatialRelation.WITHIN;
+      Rectangle bbox = indexedShape.getBoundingBox();
+      return bbox.relate(queryShape) == SpatialRelation.WITHIN || bbox.equals(queryShape);
     }
   };
   public static final SpatialOperation Contains       = new SpatialOperation("Contains", true, true, false) {
     @Override
     public boolean evaluate(Shape indexedShape, Shape queryShape) {
-      return indexedShape.hasArea() && indexedShape.relate(queryShape) == SpatialRelation.CONTAINS;
+      return indexedShape.hasArea() && indexedShape.relate(queryShape) == SpatialRelation.CONTAINS || indexedShape.equals(queryShape);
     }
   };
   public static final SpatialOperation Intersects     = new SpatialOperation("Intersects", true, false, false) {
@@ -85,7 +87,7 @@ public abstract class SpatialOperation implements Serializable {
   public static final SpatialOperation IsWithin       = new SpatialOperation("IsWithin", true, false, true) {
     @Override
     public boolean evaluate(Shape indexedShape, Shape queryShape) {
-      return queryShape.hasArea() && indexedShape.relate(queryShape) == SpatialRelation.WITHIN;
+      return queryShape.hasArea() && (indexedShape.relate(queryShape) == SpatialRelation.WITHIN || indexedShape.equals(queryShape));
     }
   };
   public static final SpatialOperation Overlaps       = new SpatialOperation("Overlaps", true, false, true) {

lucene/spatial/src/test/org/apache/lucene/spatial/SpatialTestCase.java

@@ -45,7 +45,7 @@ import static com.carrotsearch.randomizedtesting.RandomizedTest.randomIntBetween
 public abstract class SpatialTestCase extends LuceneTestCase {
 
   private DirectoryReader indexReader;
-  private RandomIndexWriter indexWriter;
+  protected RandomIndexWriter indexWriter;
   private Directory directory;
   protected IndexSearcher indexSearcher;
 

lucene/spatial/src/test/org/apache/lucene/spatial/StrategyTestCase.java

@@ -26,10 +26,12 @@ import org.apache.lucene.document.Document;
 import org.apache.lucene.document.Field;
 import org.apache.lucene.document.StoredField;
 import org.apache.lucene.document.StringField;
+import org.apache.lucene.index.Term;
 import org.apache.lucene.queries.function.FunctionQuery;
 import org.apache.lucene.queries.function.ValueSource;
 import org.apache.lucene.search.CheckHits;
 import org.apache.lucene.search.ScoreDoc;
+import org.apache.lucene.search.TermQuery;
 import org.apache.lucene.search.TopDocs;
 import org.apache.lucene.spatial.query.SpatialArgs;
 import org.apache.lucene.spatial.query.SpatialArgsParser;
@@ -203,6 +205,10 @@ public abstract class StrategyTestCase extends SpatialTestCase {
     return doc;
   }
 
+  protected void deleteDoc(String id) throws IOException {
+    indexWriter.deleteDocuments(new TermQuery(new Term("id", id)));
+  }
+
   /** scores[] are in docId order */
   protected void checkValueSource(ValueSource vs, float scores[], float delta) throws IOException {
     FunctionQuery q = new FunctionQuery(vs);

lucene/spatial/src/test/org/apache/lucene/spatial/prefix/SpatialOpRecursivePrefixTreeTest.java

@@ -19,8 +19,10 @@ package org.apache.lucene.spatial.prefix;
 
 import com.carrotsearch.randomizedtesting.annotations.Repeat;
 import com.spatial4j.core.context.SpatialContext;
+import com.spatial4j.core.shape.Point;
 import com.spatial4j.core.shape.Rectangle;
 import com.spatial4j.core.shape.Shape;
+import com.spatial4j.core.shape.SpatialRelation;
 import com.spatial4j.core.shape.impl.RectangleImpl;
 import org.apache.lucene.search.Query;
 import org.apache.lucene.spatial.StrategyTestCase;
@@ -29,9 +31,11 @@ import org.apache.lucene.spatial.prefix.tree.QuadPrefixTree;
 import org.apache.lucene.spatial.prefix.tree.SpatialPrefixTree;
 import org.apache.lucene.spatial.query.SpatialArgs;
 import org.apache.lucene.spatial.query.SpatialOperation;
+import org.junit.Before;
 import org.junit.Test;
 
 import java.io.IOException;
+import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -45,67 +49,114 @@ public class SpatialOpRecursivePrefixTreeTest extends StrategyTestCase {
 
   private SpatialPrefixTree grid;
 
-  @Test
-  @Repeat(iterations = 20)
-  public void testIntersects() throws IOException {
-    //non-geospatial makes this test a little easier
+  @Before
+  public void setUp() throws Exception {
+    super.setUp();
+    deleteAll();
+  }
+
+  public void mySetup() throws IOException {
+    //non-geospatial makes this test a little easier (in gridSnap), and using boundary values 2^X raises
+    // the prospect of edge conditions we want to test, plus makes for simpler numbers (no decimals).
     this.ctx = new SpatialContext(false, null, new RectangleImpl(0, 256, -128, 128, null));
     //A fairly shallow grid, and default 2.5% distErrPct
     this.grid = new QuadPrefixTree(ctx, randomIntBetween(1, 8));
     this.strategy = new RecursivePrefixTreeStrategy(grid, getClass().getSimpleName());
     //((PrefixTreeStrategy) strategy).setDistErrPct(0);//fully precise to grid
 
-    deleteAll();
+    System.out.println("Strategy: "+strategy.toString());
+  }
 
+  @Test
+  @Repeat(iterations = 10)
+  public void testIntersects() throws IOException {
+    mySetup();
+    doTest(SpatialOperation.Intersects);
+  }
+
+  @Test
+  @Repeat(iterations = 10)
+  public void testWithin() throws IOException {
+    mySetup();
+    doTest(SpatialOperation.IsWithin);
+  }
+
+  @Test
+  public void testWithinDisjointParts() throws IOException {
+    this.ctx = new SpatialContext(false, null, new RectangleImpl(0, 256, -128, 128, null));
+    //A fairly shallow grid, and default 2.5% distErrPct
+    this.grid = new QuadPrefixTree(ctx, 7);
+    this.strategy = new RecursivePrefixTreeStrategy(grid, getClass().getSimpleName());
+
+    //one shape comprised of two parts, quite separated apart
+    adoc("0", new ShapePair(ctx.makeRectangle(0, 10, -120, -100), ctx.makeRectangle(220, 240, 110, 125)));
+    commit();
+    //query surrounds only the second part of the indexed shape
+    Query query = strategy.makeQuery(new SpatialArgs(SpatialOperation.IsWithin, ctx.makeRectangle(210, 245, 105, 128)));
+    SearchResults searchResults = executeQuery(query, 1);
+    //we shouldn't find it because it's not completely within
+    assertTrue(searchResults.numFound==0);
+  }
+
+  private void doTest(final SpatialOperation operation) throws IOException {
     Map<String, Shape> indexedShapes = new LinkedHashMap<String, Shape>();
-    Map<String, Rectangle> indexedGriddedShapes = new LinkedHashMap<String, Rectangle>();
     final int numIndexedShapes = randomIntBetween(1, 6);
-    for (int i = 1; i <= numIndexedShapes; i++) {
-      String id = "" + i;
-      Shape indexShape = randomRectangle();
-      Rectangle gridShape = gridSnapp(indexShape);
-      indexedShapes.put(id, indexShape);
-      indexedGriddedShapes.put(id, gridShape);
-      adoc(id, indexShape);
+    for (int i = 0; i < numIndexedShapes; i++) {
+      String id = ""+i;
+      Shape indexedShape;
+      if (random().nextInt(4) == 0) {
+        indexedShape = new ShapePair( gridSnapp(randomRectangle()), gridSnapp(randomRectangle()) );
+      } else {
+        indexedShape = gridSnapp(randomRectangle());
+      }
+      indexedShapes.put(id, indexedShape);
+      adoc(id, indexedShape);
+      if (random().nextInt(10) == 0)
+        commit();
+    }
+
+    //delete some
+    Iterator<String> idIter = indexedShapes.keySet().iterator();
+    while (idIter.hasNext()) {
+      String id = idIter.next();
+      if (random().nextInt(10) == 0) {
+        deleteDoc(id);
+        idIter.remove();
+      }
     }
 
     commit();
 
-    final int numQueryShapes = atLeast(10);
+    final int numQueryShapes = atLeast(20);
     for (int i = 0; i < numQueryShapes; i++) {
       int scanLevel = randomInt(grid.getMaxLevels());
       ((RecursivePrefixTreeStrategy) strategy).setPrefixGridScanLevel(scanLevel);
-      Rectangle queryShape = randomRectangle();
-      Rectangle queryGridShape = gridSnapp(queryShape);
+      Shape queryShape = gridSnapp(randomRectangle());
 
       //Generate truth via brute force
-      final SpatialOperation operation = SpatialOperation.Intersects;
       Set<String> expectedIds = new TreeSet<String>();
-      Set<String> optionalIds = new TreeSet<String>();
-      for (String id : indexedShapes.keySet()) {
-        Shape indexShape = indexedShapes.get(id);
-        Rectangle indexGridShape = indexedGriddedShapes.get(id);
-        if (operation.evaluate(indexShape, queryShape))
-          expectedIds.add(id);
-        else if (operation.evaluate(indexGridShape, queryGridShape))
-          optionalIds.add(id);
+      for (Map.Entry<String, Shape> entry : indexedShapes.entrySet()) {
+        if (operation.evaluate(entry.getValue(), queryShape))
+          expectedIds.add(entry.getKey());
       }
 
       //Search and verify results
       Query query = strategy.makeQuery(new SpatialArgs(operation, queryShape));
       SearchResults got = executeQuery(query, 100);
       Set<String> remainingExpectedIds = new TreeSet<String>(expectedIds);
-      String msg = queryShape.toString()+" Expect: "+expectedIds+" Opt: "+optionalIds;
+      String msg = queryShape.toString()+" Expect: "+expectedIds;
       for (SearchResult result : got.results) {
         String id = result.getId();
         Object removed = remainingExpectedIds.remove(id);
         if (removed == null) {
-          assertTrue("Shouldn't match " + id + " in "+msg, optionalIds.contains(id));
+          fail("Shouldn't match " + id + " ("+ indexedShapes.get(id) +") in " + msg);
         }
       }
-      assertTrue("Didn't match " + remainingExpectedIds + " in " + msg, remainingExpectedIds.isEmpty());
+      if (!remainingExpectedIds.isEmpty()) {
+        Shape firstFailedMatch = indexedShapes.get(remainingExpectedIds.iterator().next());
+        fail("Didn't match " + firstFailedMatch + " in " + msg +" (of "+remainingExpectedIds.size()+")");
+      }
     }
-
   }
 
   protected Rectangle gridSnapp(Shape snapMe) {
@@ -130,4 +181,48 @@ public class SpatialOpRecursivePrefixTreeTest extends StrategyTestCase {
     return ctx.makeRectangle(minX, maxX, minY, maxY);
   }
 
+  /** An aggregate of 2 shapes. Only implements what's necessary for the test here.
+   * TODO replace with Spatial4j trunk ShapeCollection. */
+  private class ShapePair implements Shape {
+
+    Shape shape1, shape2;
+
+    public ShapePair(Shape shape1, Shape shape2) {
+      this.shape1 = shape1;
+      this.shape2 = shape2;
+    }
+
+    @Override
+    public SpatialRelation relate(Shape other) {
+      //easy to observe is correct; not an optimal code path but this is a test
+      if (shape1.relate(other) == SpatialRelation.CONTAINS || shape2.relate(other) == SpatialRelation.CONTAINS)
+        return SpatialRelation.CONTAINS;
+      if (shape1.relate(other) == SpatialRelation.WITHIN && shape2.relate(other) == SpatialRelation.WITHIN)
+        return SpatialRelation.WITHIN;
+      if (shape1.relate(other).intersects() || shape2.relate(other).intersects())
+        return SpatialRelation.INTERSECTS;
+      return SpatialRelation.DISJOINT;
+    }
+
+    @Override
+    public Rectangle getBoundingBox() {
+      return ctx.getWorldBounds();//good enough
+    }
+
+    @Override
+    public boolean hasArea() {
+      throw new UnsupportedOperationException("TODO unimplemented");//TODO
+    }
+
+    @Override
+    public double getArea(SpatialContext ctx) {
+      throw new UnsupportedOperationException("TODO unimplemented");//TODO
+    }
+
+    @Override
+    public Point getCenter() {
+      throw new UnsupportedOperationException("TODO unimplemented");//TODO
+    }
+  }
+
 }