-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Issue 67: Update Elasticsearch and improve geoshape indexing #79
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
Changes from all commits
62e02b0
513d1f6
33f7d8f
12dcf21
230382c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,7 +83,7 @@ public JanusGraphPredicate negate() { | |
| }, | ||
|
|
||
| /** | ||
| * Whether one geographic region is completely contains within another | ||
| * Whether one geographic region is completely within another | ||
| */ | ||
| WITHIN { | ||
| @Override | ||
|
|
@@ -104,6 +104,34 @@ public boolean hasNegation() { | |
| return false; | ||
| } | ||
|
|
||
| @Override | ||
| public JanusGraphPredicate negate() { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
| }, | ||
|
|
||
| /** | ||
| * Whether one geographic region completely contains another | ||
| */ | ||
| CONTAINS { | ||
| @Override | ||
| public boolean test(Object value, Object condition) { | ||
| Preconditions.checkArgument(condition instanceof Geoshape); | ||
| if (value == null) return false; | ||
| Preconditions.checkArgument(value instanceof Geoshape); | ||
| return ((Geoshape) value).contains((Geoshape) condition); | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return "contains"; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean hasNegation() { | ||
| return false; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comment on why this is not supported. I think it is because when shapes intersect at their edges, part of the shapes are on the outside.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A JanusGraphPredicate can only have a negation if it can be defined directly as another JanusGraphPredicate. This isn't possible for Geo.Contains (e.g. negation is not Geo.WITHIN for example). Note this is a separate question from whether the negation of the Contains spatial predicate exists mathematically, though I'm not sure it does because of subtleties in the definition. https://en.wikipedia.org/wiki/DE-9IM#Spatial_predicates |
||
| } | ||
|
|
||
| @Override | ||
| public JanusGraphPredicate negate() { | ||
| throw new UnsupportedOperationException(); | ||
|
|
@@ -137,4 +165,7 @@ public static <V> P<V> geoDisjoint(final V value) { | |
| public static <V> P<V> geoWithin(final V value) { | ||
| return new P(Geo.WITHIN, value); | ||
| } | ||
| public static <V> P<V> geoContains(final V value) { | ||
| return new P(Geo.CONTAINS, value); | ||
| } | ||
| } | ||
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.
This is a LGPL library. Are there alternatives? It looks like the code is only used in test cases. Maybe we can make it an optional, off-by-default, inclusion via a Maven build flag or profile. TinkerPop had to do something similar for Neo4j.
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.
@pluradj Good catch.
@mbrukman We should have some license automation checks too.
@sjudeng Please let us know if there are any alternatives.
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.
@mbrukman I created #128 to track automating dependency license check as part of CI
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.
JTS is currently a non-optional dependency in janusgraph-solr and is distributed with JanusGraph (and previously Titan) releases.
janusgraph-solr/pom.xml
What could be done is to mark it as optional in the poms so it's available for tests and users with requirements for relevant features (Solr, Geoshapes, etc.) can manually add it to their lib directory.
Given it's already a dependency can this be a separate issue or is it a blocker here?
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.
Looks like the longer term solution might be a new Eclipse Foundation project aiming to replace JTS.
https://github.com/locationtech/jts
https://www.locationtech.org/projects/technology.jts
https://www.locationtech.org/proposals/jts-topology-suite
It's being developed by LocationTech, who also develop Spatial4j, through-which the JTS dependency comes in to Solr/JanusGraph. I'd guess the roadmap is for a stable new-JTS release and then a Spatial4j update/release migrating to same, at which point JanusGraph could update as well (likely involving a Solr version upgrade).
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.
ha, you're right @sjudeng - had no idea that dependency was around for so long.
Good find on that new project. any indications that they are close to a release? It seems like they've been at it for a while.
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.
I don't see anything on a release timeline. There's recent mailing list activity but there was a gap for a few months before that. Also I don't think a full migration in JanusGraph would be possible until Spatial4j and Solr were updated to use JTS 2. The good news is the core Spatial4j developer, who I think also is a core contributor on the Solr spatial connectors, has been active on the JTS 2 mailing list so he is at least aware of the effort.