This feature allows to index points, circles and boxes in PREFIX_TREE mapping for ES. by davidclement90 · Pull Request #294 · JanusGraph/janusgraph

davidclement90 · 2017-05-31T13:01:51Z

I also fix some indentation issue.
#293

Signed-off-by: David Clement [email protected]

sjudeng

Nice work. Can you also update documentation (e.g. elasticsearch.txt and searchpredicates.txt)?

sjudeng · 2017-05-31T13:16:53Z

janusgraph-test/src/main/java/org/janusgraph/diskstorage/indexing/IndexProviderTest.java


            //Update some data
-            add(store, "doc4", getDocument("I'ts all a big Bob", -100, 11.2, Geoshape.point(48.0, 8.0), Geoshape.line(Arrays.asList(new double[][] {{7.5, 47.5}, {8.5, 48.5}})), Arrays.asList("10", "11", "12"), Sets.newHashSet("10", "11"), Instant.ofEpochSecond(4)), true);
+            add(store, "doc4", getDocument("I'ts all a big Bob", -100, 11.2, Geoshape.point(-48.0, 8.0), Geoshape.point(-48.0, 8.0), Arrays.asList("10", "11", "12"), Sets.newHashSet("10", "11"), Instant.ofEpochSecond(4)), true);


Is there a reason you changed the sign on the first point latitude?

Yes, I want to test the reindexing process for prefix_tree in "doc1" so I change the sign on latitude to be sure that the document has really changed.
I do not want to change all tests thoroughly so I change also the sign en "doc4".

Makes sense thanks

sjudeng · 2017-06-01T11:23:47Z

docs/searchpredicates.txt

 [[geoshape]]
 === Geoshape Data Type
-The Geoshape data type supports representing a point, circle, box, line, polygon, multi-point, multi-line and multi-polygon. Index backends currently support indexing points, lines and polygons. Indexing multi-point, multi-line and multi-polygon properties has not been tested.
+The Geoshape data type supports representing a point, circle, box, line, polygon, multi-point, multi-line and multi-polygon. Index backends currently support indexing points, circles, boxes, lines and polygons. Indexing multi-point, multi-line and multi-polygon properties has not been tested.


Can you add language to make clear that indexing circles and boxes is only supported under Elasticsearch?

Why only on Elasticsearch ?
Solr is also tested. And I think that the code allow it. https://github.com/JanusGraph/janusgraph/blob/master/janusgraph-solr/src/main/java/org/janusgraph/diskstorage/solr/SolrIndex.java#L361

Right, perfect! That code was added in #79 I just hadn't tested circle/box indexing. Thanks!

amcp

minor comments

amcp · 2017-06-03T10:53:36Z

janusgraph-es/src/main/java/org/janusgraph/diskstorage/es/ElasticSearchIndex.java

            Geoshape.Point p = geoshape.getPoint();
            return new double[]{p.getLongitude(), p.getLatitude()};
-        } else if (geoshape.getType() != Geoshape.Type.BOX && geoshape.getType() != Geoshape.Type.CIRCLE) {
+        } else if (geoshape.getType() == Geoshape.Type.BOX) {


why is it OK not to catch IOException for BOX but required for CIRCLE?

Yes it's OK.
IOException is throwed by geoshape.toMap()

For Box, the map is constructed without using geoshape.toMap().
For Circle, the map is constructed with geoshape.toMap().

amcp · 2017-06-03T10:55:01Z

janusgraph-es/src/main/java/org/janusgraph/diskstorage/es/ElasticSearchIndex.java

                case SET:
                case LIST:
-                    script.append("ctx._source[\"" + e.field + "\"].add(" + convertToJsType(e.value, scriptLang) + ");");
+                    script.append("ctx._source[\"" + e.field + "\"].add(" + convertToJsType(e.value, scriptLang, Mapping.getMapping(keyInformation)) + ");");


use a String.format to reduce the number of frame switches

@amcp Switching to String.format may be a performance hit. I was looking up whether it was preferred over StringBuilder and results seemed to say concatenation and StringBuilder end up being similar (though StringBuilder may be better style), but String.format is 5-20 time slower than both.

https://kylewbanks.com/blog/java-string-concatenation-vs-stringbuilder-vs-string-format-performance

http://jacksondunstan.com/articles/3015

https://stackoverflow.com/questions/513600/should-i-use-javas-string-format-if-performance-is-important/513705#513705

@sjudeng Wow. Thank you for this important learning. StringBuilder is the way to go from now on.

amcp · 2017-06-03T10:55:10Z

janusgraph-es/src/main/java/org/janusgraph/diskstorage/es/ElasticSearchIndex.java

+                    script.append("ctx._source[\"" + e.field + "\"].add(" + convertToJsType(e.value, scriptLang, Mapping.getMapping(keyInformation)) + ");");
                    if (hasDualStringMapping(keyInformation)) {
-                        script.append("ctx._source[\"" + getDualMappingName(e.field) + "\"].add(" + convertToJsType(e.value, scriptLang) + ");");
+                        script.append("ctx._source[\"" + getDualMappingName(e.field) + "\"].add(" + convertToJsType(e.value, scriptLang, Mapping.getMapping(keyInformation)) + ");");


String.format

not an issue!

Signed-off-by: David Clement <[email protected]>

…ixPrefixTree This feature allows to index points, circles and boxes in PREFIX_TREE mapping for ES.

janusgraph-bot added the cla: yes This PR is compliant with the CLA label May 31, 2017

davidclement90 force-pushed the elasticsearch-fixPrefixTree branch from 65fe639 to 6f76edd Compare May 31, 2017 13:06

sjudeng requested changes May 31, 2017

View reviewed changes

davidclement90 force-pushed the elasticsearch-fixPrefixTree branch from 6f76edd to 504f773 Compare June 1, 2017 07:27

sjudeng reviewed Jun 1, 2017

View reviewed changes

sjudeng approved these changes Jun 2, 2017

View reviewed changes

amcp reviewed Jun 3, 2017

View reviewed changes

Allows to index points, circles and boxes in PREFIX_TREE mapping in ES.

d1a7e58

Signed-off-by: David Clement <[email protected]>

davidclement90 force-pushed the elasticsearch-fixPrefixTree branch from 504f773 to d1a7e58 Compare June 6, 2017 22:38

amcp approved these changes Jun 7, 2017

View reviewed changes

amcp merged commit 392121e into JanusGraph:master Jun 7, 2017

davidclement90 deleted the elasticsearch-fixPrefixTree branch June 7, 2017 11:50

bwatson-rti-org pushed a commit to bwatson-rti-org/janusgraph that referenced this pull request Mar 9, 2019

Merge pull request JanusGraph#294 from davidclement90/elasticsearch-f…

746786f

…ixPrefixTree This feature allows to index points, circles and boxes in PREFIX_TREE mapping for ES.

micpod pushed a commit to micpod/janusgraph that referenced this pull request Nov 5, 2019

Merge pull request JanusGraph#294 from davidclement90/elasticsearch-f…

e64af63

…ixPrefixTree This feature allows to index points, circles and boxes in PREFIX_TREE mapping for ES.

Uh oh!

Conversation

davidclement90 commented May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjudeng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidclement90 Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjudeng Jun 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

davidclement90 commented May 31, 2017 •

edited

Loading

davidclement90 Jun 6, 2017 •

edited

Loading

sjudeng Jun 3, 2017 •

edited

Loading