-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LUCENE-8903: Add LatLonShape point query #762
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much does it help? :)
@@ -94,9 +94,18 @@ private LatLonShape() { | |||
return new Field[] {new LatLonTriangle(fieldName, lat, lon, lat, lon, lat, lon)}; | |||
} | |||
|
|||
/** create a query to find all indexed shapes that comply the {@link QueryRelation} with the provided point | |||
**/ | |||
public static Query newPointQuery(String field, QueryRelation queryRelation, double lat, double lon) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need a relation or can we just assume INTERSECTS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, I would keep it like that for two reasons:
-
Keep it consistent with all the other queries in LatLonShape
-
You can build queries like give me all my shapes that do not contain this point. For Within it becomes a term query matching all indexed point which encoded value are equal to the encoded value of the query.
@@ -147,6 +147,11 @@ protected Query newRectQuery(String field, QueryRelation queryRelation, double m | |||
return LatLonShape.newBoxQuery(field, queryRelation, minLat, maxLat, minLon, maxLon); | |||
} | |||
|
|||
/** factory method to create a new bounding box query */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/bounding box/point/
} else { | ||
System.out.println(b.toString()); | ||
fail = true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftovers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems all this verify functions contain this piece of logic. I guess they were introduced for debugging so I don't want to change it in this PR.
In the dataset I am working with I get speed ups of around 10%:
|
Going a bit more in detail, I wrote the following test to prove a difference between LatLonPoint and LatLonShape:
The test basically index a shape and make a point query and at the same time it index the same point a make a polygon query with the same polygon. It will eventually fail because in LatLonShape, both polygon and point work on the encoding space. On the other hand for LatLonPoint, the query polygon does not work on the encoding space. |
I think this is duplicate of LUCENE-8670 which I opened and posted a patch back at the end of January. If I remember right, the only reason we were holding off on that patch was because querying by |
I think that can be solved by #770 (LUCENE-8746). This is actually what is proposed, to create an R-tree structure when creating a multi-shape. |
# Conflicts: # lucene/sandbox/src/java/org/apache/lucene/document/LatLonShape.java # lucene/sandbox/src/test/org/apache/lucene/document/BaseLatLonShapeTestCase.java
# Conflicts: # lucene/sandbox/src/java/org/apache/lucene/document/LatLonShape.java # lucene/sandbox/src/test/org/apache/lucene/document/BaseLatLonShapeTestCase.java
# Conflicts: # lucene/sandbox/src/java/org/apache/lucene/document/LatLonShape.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
//This should be moved when LatLonShape is moved from sandbox! | ||
/** | ||
* Compute whether the given x, y point is in a triangle; uses the winding order method */ | ||
private static boolean pointInTriangle (double x, double y, double ax, double ay, double bx, double by, double cx, double cy) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate of?
public static boolean pointInTriangle (double x, double y, double ax, double ay, double bx, double by, double cx, double cy) { |
Adds a query to LatLonShape that filters by a provided point.