LUCENE-4916: Spatial Within RPT and shape simplification bug

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

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

@@ -52,7 +52,7 @@ public abstract class AbstractVisitingPrefixTreeFilter extends AbstractPrefixTre
   public AbstractVisitingPrefixTreeFilter(Shape queryShape, String fieldName, SpatialPrefixTree grid,
                                           int detailLevel, int prefixGridScanLevel) {
     super(queryShape, fieldName, grid, detailLevel);
-    this.prefixGridScanLevel = Math.max(1, Math.min(prefixGridScanLevel, grid.getMaxLevels() - 1));
+    this.prefixGridScanLevel = Math.max(0, Math.min(prefixGridScanLevel, grid.getMaxLevels() - 1));
     assert detailLevel <= grid.getMaxLevels();
   }
 

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

@@ -32,6 +32,7 @@ import org.apache.lucene.util.Bits;
 import org.apache.lucene.util.FixedBitSet;
 
 import java.io.IOException;
+import java.util.Collection;
 import java.util.Iterator;
 
 /**
@@ -160,18 +161,41 @@ public class WithinPrefixTreeFilter extends AbstractVisitingPrefixTreeFilter {
 
       @Override
       protected void visitLeaf(Cell cell) throws IOException {
-        SpatialRelation relation = visitRelation;
+        //visitRelation is declared as a field, populated by visit() so we don't recompute it
+        assert detailLevel != cell.getLevel();
         assert visitRelation == cell.getShape().relate(queryShape);
-        if (relation.intersects()) {
+        if (allCellsIntersectQuery(cell, visitRelation))
           collectDocs(inside);
-        } else {
+        else
           collectDocs(outside);
       }
+
+      /** Returns true if the provided cell, and all its sub-cells down to
+       * detailLevel all intersect the queryShape.
+       */
+      private boolean allCellsIntersectQuery(Cell cell, SpatialRelation relate/*cell to query*/) {
+        if (relate == null)
+          relate = cell.getShape().relate(queryShape);
+        if (cell.getLevel() == detailLevel)
+          return relate.intersects();
+        if (relate == SpatialRelation.WITHIN)
+          return true;
+        if (relate == SpatialRelation.DISJOINT)
+          return false;
+        // Note: Generating all these cells just to determine intersection is not ideal.
+        // It was easy to implement but could be optimized. For example if the docs
+        // in question are already marked in the 'outside' bitset then it can be avoided.
+        Collection<Cell> subCells = cell.getSubCells(null);
+        for (Cell subCell : subCells) {
+          if (!allCellsIntersectQuery(subCell, null))//recursion
+            return false;
+        }
+        return true;
       }
 
       @Override
       protected void visitScanned(Cell cell) throws IOException {
-        if (queryShape.relate(cell.getShape()).intersects()) {
+        if (allCellsIntersectQuery(cell, null)) {
           collectDocs(inside);
         } else {
           collectDocs(outside);

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

@@ -55,12 +55,14 @@ public class SpatialOpRecursivePrefixTreeTest extends StrategyTestCase {
     deleteAll();
   }
 
-  public void mySetup() throws IOException {
+  public void mySetup(int maxLevels) 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));
+    if (maxLevels == -1)
+      maxLevels = randomIntBetween(1, 8);
+    this.grid = new QuadPrefixTree(ctx, maxLevels);
     this.strategy = new RecursivePrefixTreeStrategy(grid, getClass().getSimpleName());
     //((PrefixTreeStrategy) strategy).setDistErrPct(0);//fully precise to grid
 
@@ -70,30 +72,27 @@ public class SpatialOpRecursivePrefixTreeTest extends StrategyTestCase {
   @Test
   @Repeat(iterations = 10)
   public void testIntersects() throws IOException {
-    mySetup();
+    mySetup(-1);
     doTest(SpatialOperation.Intersects);
   }
 
   @Test
   @Repeat(iterations = 10)
   public void testWithin() throws IOException {
-    mySetup();
+    mySetup(-1);
     doTest(SpatialOperation.IsWithin);
   }
 
   @Test
   @Repeat(iterations = 10)
   public void testContains() throws IOException {
-    mySetup();
+    mySetup(-1);
     doTest(SpatialOperation.Contains);
   }
 
   @Test
   public void testWithinDisjointParts() throws IOException {
-    this.ctx = new SpatialContext(false, null, new RectangleImpl(0, 256, -128, 128, null));
+    mySetup(7);
-    //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)));
@@ -105,6 +104,29 @@ public class SpatialOpRecursivePrefixTreeTest extends StrategyTestCase {
     assertTrue(searchResults.numFound == 0);
   }
 
+  @Test /** LUCENE-4916 */
+  public void testWithinLeafApproxRule() throws IOException {
+    mySetup(2);//4x4 grid
+    //indexed shape will simplify to entire right half (2 top cells)
+    adoc("0", ctx.makeRectangle(192, 204, -128, 128));
+    commit();
+
+    ((RecursivePrefixTreeStrategy) strategy).setPrefixGridScanLevel(randomInt(2));
+
+    //query does NOT contain it; both indexed cells are leaves to the query, and
+    // when expanded to the full grid cells, the top one's top row is disjoint
+    // from the query and thus not a match.
+    assertTrue(executeQuery(strategy.makeQuery(
+        new SpatialArgs(SpatialOperation.IsWithin, ctx.makeRectangle(38, 192, -72, 56))
+    ), 1).numFound==0);//no-match
+
+    //this time the rect is a little bigger and is considered a match. It's a
+    // an acceptable false-positive because of the grid approximation.
+    assertTrue(executeQuery(strategy.makeQuery(
+        new SpatialArgs(SpatialOperation.IsWithin, ctx.makeRectangle(38, 192, -72, 80))
+    ), 1).numFound==1);//match
+  }
+
   private void doTest(final SpatialOperation operation) throws IOException {
     Map<String, Shape> indexedShapes = new LinkedHashMap<String, Shape>();
     final int numIndexedShapes = randomIntBetween(1, 6);