LUCENE-8620: LatLonShape contains #546

iverase · 2019-01-22T13:58:06Z

LatLonShape's implementation for spatial relationship CONTAINS.

jpountz

I had a (very) quick look, I'll try to do a deeper review later.

jpountz · 2019-01-22T14:04:33Z

lucene/core/src/java/org/apache/lucene/geo/EdgeTree.java

@@ -101,6 +101,60 @@ public Relation relateTriangle(double ax, double ay, double bx, double by, doubl
    return Relation.CELL_OUTSIDE_QUERY;
  }

+  /** Used by {@link withinTriangle} to check the relationship between a triangle and the query shape */
+  public enum WithinRelation {
+    /** If the shape is a candidate for within. Tipically this is return if the query shape is fully inside


s/Tipically/Typically/

jpountz · 2019-01-22T14:15:10Z

lucene/core/src/java/org/apache/lucene/geo/EdgeTree.java

+     * the triangle or if the query shape intersects only edges that do not belong to the original shape. */
+    CANDIDATE,
+    /** Return this if if the query shape intersects an edge that does belong to the original shape. */
+    INTERSECTS,


is it what we call CROSSES elsewhere?

jpountz · 2019-01-22T14:21:14Z

lucene/core/src/java/org/apache/lucene/geo/EdgeTree.java

@@ -150,9 +204,88 @@ private Relation internalComponentRelateTriangle(double ax, double ay, double bx
    if (tree.crossesTriangle(ax, ay, bx, by, cx, cy)) {
      return Relation.CELL_CROSSES_QUERY;
    }
+    if (pointInTriangle(tree.lon1, tree.lat1, ax, ay, bx, by, cx, cy) == true) {
+      return Relation.CELL_CROSSES_QUERY;
+    }


This looks like an unrelated bug, let's have a dedicated issue for it?

I opened https://issues.apache.org/jira/browse/LUCENE-8654

jpountz · 2019-01-22T14:26:34Z

lucene/sandbox/src/java/org/apache/lucene/document/LatLonShape.java

@@ -118,19 +118,21 @@ public static Query newPolygonQuery(String field, QueryRelation queryRelation, P
   * these triangles are encoded and inserted as separate indexed POINT fields
   */
  private static class LatLonTriangle extends Field {
-
+    //Constructor for points and lines


jpountz · 2019-01-22T14:44:37Z

lucene/sandbox/src/java/org/apache/lucene/document/LatLonShapeBoundingBoxQuery.java

+    int cY = scratchTriangle.cY;
+    int cX = scratchTriangle.cX;
+
+    return rectangle2D.withinTriangle(aX, aY, scratchTriangle.ab, bX, bY, scratchTriangle.bc, cX, cY, scratchTriangle.ca);


extracting variables doesn't seem helpful here?

jpountz · 2019-01-22T14:50:01Z

lucene/sandbox/src/java/org/apache/lucene/geo/Rectangle2D.java

@@ -202,7 +286,7 @@ private boolean queryIntersects(int ax, int ay, int bx, int by, int cx, int cy)
    return false;
  }

-  /** returns true if the edge (defined by (ax, ay) (bx, by)) intersects the query */
+  /** returns true if the edge (defined by (ax, ay) (bx, by)) crosses the query */


docs don't match the method name

jpountz · 2019-01-22T14:50:34Z

lucene/sandbox/src/java/org/apache/lucene/geo/Rectangle2D.java

@@ -224,7 +313,7 @@ private static boolean bboxContainsTriangle(int ax, int ay, int bx, int by, int
        && bboxContainsPoint(cx, cy, minX, maxX, minY, maxY);
  }

-  /** returns true if the edge (defined by (ax, ay) (bx, by)) intersects the query */
+  /** returns true if the edge (defined by (ax, ay) (bx, by)) crosses the query */


docs don't match the method name?

jpountz · 2019-01-22T14:51:54Z

lucene/sandbox/src/test/org/apache/lucene/document/TestLatLonLineShapeQueries.java

            return false;
          }
+        } else if(queryRelation == QueryRelation.CONTAINS) {


let's add one space between if and (?

jpountz · 2019-01-22T14:55:36Z

lucene/sandbox/src/test/org/apache/lucene/document/TestLatLonLineShapeQueries.java

            return false;
          }
+        } else if(queryRelation == QueryRelation.CONTAINS) {
+          EdgeTree.WithinRelation relation = rectangle2D.withinTriangle(decoded.aX, decoded.aY, decoded.ab, decoded.bX, decoded.bY, decoded.bc, decoded.cX, decoded.cY, decoded.ca);
+          if  (relation == EdgeTree.WithinRelation.INTERSECTS) {


only one space between if and (? :)

jpountz · 2019-01-22T15:13:11Z

lucene/sandbox/src/test/org/apache/lucene/document/TestLatLonMultiPolygonShapeQueries.java

+      return true;
+    }
+    Polygon[] currentPolygons = new Polygon[totalPolygons];
+    for (int i =0; i < totalPolygons; i++) {


add a space between = and 0?

jpountz · 2019-01-23T08:53:32Z

lucene/core/src/java/org/apache/lucene/geo/EdgeTree.java

+    /** If the shape is a candidate for within. Typically this is return if the query shape is fully inside
+     * the triangle or if the query shape intersects only edges that do not belong to the original shape. */
+    CANDIDATE,
+    /** Return this if if the query shape intersects an edge that does belong to the original shape. */


update javadocs to say "crosses" instead of intersects?

jpountz · 2019-01-23T08:55:24Z

lucene/core/src/java/org/apache/lucene/geo/EdgeTree.java

+    double maxLat = StrictMath.max(StrictMath.max(ay, by), cy);
+    double maxLon = StrictMath.max(StrictMath.max(ax, bx), cx);
+
+    //disjoint then we return null


jpountz · 2019-01-23T09:02:22Z

lucene/core/src/java/org/apache/lucene/geo/EdgeTree.java

+   * @param cx longitude of point c of the triangle
+   * @param cy latitude of point c of the triangle
+   * @param ca if edge ca belongs to the original shape
+   * @return


let's either have it in the javadocs with a description or remove it?

jpountz · 2019-01-23T09:15:30Z

lucene/core/src/java/org/apache/lucene/geo/EdgeTree.java

+
+    if (minLat <= maxY && minLon <= maxX) {
+      WithinRelation relation = internalWithinTriangle(ax, ay, ab, bx, by, bc, cx, cy, ca);
+      if (relation != null) {


internalWithinTriangle never returns null?

jpountz · 2019-01-23T10:07:03Z

lucene/sandbox/src/java/org/apache/lucene/geo/Rectangle2D.java

+
+    return bboxWithinTriangle(ax, ay, ab, bx, by, bc, cx, cy, ca, minX, maxX, minY, maxY);
+  }
+  public EdgeTree.WithinRelation bboxWithinTriangle(int ax, int ay, boolean ab, int bx, int by, boolean bc, int cx, int cy, boolean ca, int minLon, int maxLon, int minLat, int maxLat) {


this can be made private?

jpountz · 2019-01-23T10:21:03Z

lucene/sandbox/src/java/org/apache/lucene/geo/Rectangle2D.java

+    if ((tMinX <= minLon && tMaxX >= maxLon && tMinY <= minLat && tMaxY >= maxLat) == false) {
+      return EdgeTree.WithinRelation.DISJOINT;
+    }
+    //We check the min and max points, if true it is inside


we only check the min point?

jpountz · 2019-01-23T10:21:37Z

lucene/sandbox/src/java/org/apache/lucene/geo/Rectangle2D.java

+    int tMaxY = StrictMath.max(StrictMath.max(ay, by), cy);
+    if ((tMinX <= minLon && tMaxX >= maxLon && tMinY <= minLat && tMaxY >= maxLat) == false) {
+      return EdgeTree.WithinRelation.DISJOINT;
+    }


we don't seem to need this as this is only called from withinTriangle which checks this for us?

jpountz · 2019-01-23T10:31:03Z

lucene/sandbox/src/java/org/apache/lucene/geo/Tessellator.java

+    double maxX = StrictMath.max(ax, StrictMath.max(bx, cx));
+    double maxY = StrictMath.max(ay, StrictMath.max(by, cy));
+    //check the bounding box because if the triangle is degenerated, e.g points and lines, we need to filter out
+    //coplanar points that are not part of the triangle.


jpountz · 2019-01-23T10:41:17Z

lucene/sandbox/src/java/org/apache/lucene/document/LatLonShapeQuery.java

@@ -43,7 +44,7 @@
 * Base LatLonShape Query class providing common query logic for
 * {@link LatLonShapeBoundingBoxQuery} and {@link LatLonShapePolygonQuery}
 *
- * Note: this class implements the majority of the INTERSECTS, WITHIN, DISJOINT relation logic
+ * Note: this class implements the majority of the CROSSES, WITHIN, DISJOINT relation logic


bad search/replace? also add CONTAINS to the list?

jpountz · 2019-01-23T10:47:27Z

lucene/sandbox/src/java/org/apache/lucene/document/LatLonShapeQuery.java

+        IntersectVisitor visitor;
+        IntersectVisitor disjointVisitor;
+        if (queryRelation == QueryRelation.CONTAINS) {
+          visitor = getDenseIntersectVisitor(within, disjoint, queryRelation);


the within/disjoint naming of the bitsets feels a bit weird now (my fault, sorry!). Maybe candidates/rejected or something like that instead?

iverase · 2019-01-23T15:32:44Z

I have decided to refractor the triangles method in EdgeTree. I think now they are much clear as the spatial logic has been moved to the subclasses.

# Conflicts: # lucene/core/src/java/org/apache/lucene/geo/EdgeTree.java # lucene/core/src/java/org/apache/lucene/geo/Polygon2D.java # lucene/sandbox/src/java/org/apache/lucene/geo/Line2D.java

…r a line

… over it

iverase · 2019-03-13T15:12:20Z

This PR is a bit polluted so I have opened a new one (#608) and I close this.

iverase added 2 commits January 22, 2019 14:55

LUCENE-8620: LatLonShape contains

9ae37a8

Remove some left overs from testing

fc52a6c

jpountz reviewed Jan 22, 2019

View reviewed changes

iverase added 5 commits January 23, 2019 08:59

Address first review comments

3685162

Merge branch 'master' into contains

f2397ec

resolve comflict

65a6d02

Revert some renaming on comments

fcb6977

Addressong more comments from review

644e643

jpountz reviewed Jan 23, 2019

View reviewed changes

iverase added 4 commits January 23, 2019 15:51

Refractor edge tree class so spatial logic is in subclasses

1422701

addressing more comments

7febc18

small edits

5d1dad9

remove unused methos

0476769

iverase added 16 commits January 23, 2019 16:59

fix precommit errors

a4295ad

Merge remote-tracking branch 'origin/master' into contains

44b2540

Merge remote-tracking branch 'origin/master' into contains

1d2955c

add support for multi-shape queries

bd36a5a

add test for dateline, currently failing

5cd5cef

Merge remote-tracking branch 'origin/master' into contains

9985d94

# Conflicts: # lucene/core/src/java/org/apache/lucene/geo/EdgeTree.java # lucene/core/src/java/org/apache/lucene/geo/Polygon2D.java # lucene/sandbox/src/java/org/apache/lucene/geo/Line2D.java

Merge remote-tracking branch 'origin/master' into contains

6c1f6b0

# Conflicts: # lucene/core/src/java/org/apache/lucene/geo/EdgeTree.java # lucene/core/src/java/org/apache/lucene/geo/Polygon2D.java # lucene/sandbox/src/java/org/apache/lucene/geo/Line2D.java

Merge remote-tracking branch 'origin/master' into contains

b3e4cbf

Return when line crosses

08cc09d

Shortcut for Polygon within a triangle, and the triangle is a point o…

70dc4ec

…r a line

Check properly bounding boxes relationship

182c184

Remove check for triangle points

2157b2f

Return WITHIN in relateLine when it happens

4ae7041

Return WITHIN in relateLine when it happens

bac3c91

check that line is fully over the other line

fae297d

When checking if kine is within the comparision needs equal as well

2dcd760

iverase added 27 commits February 27, 2019 16:00

special case when lines are paralell

1897e59

LINE2D specific tests

aebd33e

Merge remote-tracking branch 'upstream/master' into contains

d454e7c

Control crossing the dateline by ignoring lines on triangles that lie…

19c2fad

… over it

Control crossing the dateline by ignoring lines on triangles that lie…

a56fee9

… over it

Add more tests over the dateline

c14f39f

Some formatting

b43ab32

Check if when splitting a polygon we are doing it from the same point

24b577f

Improve handling of edge cases

6f03f83

Improve handling of edge cases

40698aa

Improve handling of edge cases

bdc4c75

Improve handling of edge cases

1313b79

Throw error when detecting a problem in tessellation

144ae52

Merge remote-tracking branch 'origin/master' into contains

ba81501

Revert to String method to original

82216eb

Clean up tessellator

5388968

Clean up tessellator tests

5e98aea

Simplify EdgeTree logic

38851ae

Re-oder methods in EdgeTree

b21912c

more clean-up in EdgeTree

688385a

more clean-up in EdgeTree

4f52036

remove unused imports

7d0e217

simplify crosses logic

99036f7

Make sure we do not return empty tessellation

818ff30

Improve spatial relationship methods

781cfa6

Decode triangles in base class

8279bdb

Update javadocs

5045674

iverase closed this Mar 13, 2019

iverase deleted the contains branch February 7, 2020 20:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LUCENE-8620: LatLonShape contains #546

LUCENE-8620: LatLonShape contains #546

iverase commented Jan 22, 2019

jpountz left a comment

jpountz Jan 22, 2019

jpountz Jan 22, 2019

jpountz Jan 22, 2019

iverase Jan 22, 2019

jpountz Jan 22, 2019

jpountz Jan 22, 2019

jpountz Jan 22, 2019

jpountz Jan 22, 2019

jpountz Jan 22, 2019

jpountz Jan 22, 2019

jpountz Jan 22, 2019

jpountz Jan 23, 2019

jpountz Jan 23, 2019

jpountz Jan 23, 2019

jpountz Jan 23, 2019

jpountz Jan 23, 2019

jpountz Jan 23, 2019

jpountz Jan 23, 2019

jpountz Jan 23, 2019

jpountz Jan 23, 2019

jpountz Jan 23, 2019

iverase commented Jan 23, 2019

iverase commented Mar 13, 2019

LUCENE-8620: LatLonShape contains #546

LUCENE-8620: LatLonShape contains #546

Conversation

iverase commented Jan 22, 2019

jpountz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iverase commented Jan 23, 2019

iverase commented Mar 13, 2019