[Geo] Add Well Known Text (WKT) Parsing Support to ShapeBuilders #27417

nknize · 2017-11-16T23:00:29Z

This PR adds WKT support to Geo ShapeBuilders.

This supports the following format:

POINT (30 10)
LINESTRING (30 10, 10 30, 40 40)
POLYGON ((30 10, 40 40, 20 40, 10 20, 30 10))
POLYGON ((35 10, 45 45, 15 40, 10 20, 35 10), (20 30, 35 35, 30 20, 20 30))
MULTIPOINT ((10 40), (40 30), (20 20), (30 10))
MULTIPOINT (10 40, 40 30, 20 20, 30 10)
MULTILINESTRING ((10 10, 20 20, 10 40),(40 40, 30 30, 40 20, 30 10))
MULTIPOLYGON (((30 20, 45 40, 10 40, 30 20)), ((15 5, 40 10, 10 20, 5 10, 15 5)))
MULTIPOLYGON (((40 40, 20 45, 45 30, 40 40)), ((20 35, 10 30, 10 10, 30 5, 45 20, 20 35), (30 20, 20 15, 20 25, 30 20)))

closes #9120

nknize · 2017-11-18T04:05:31Z

@elasticmachine please test this.

cbuescher

@nknize thanks, I did a first round of review. Most changes in the builders look fine, however I think the WKT parser currently is a bit too lenient in what it accepts as valid WKT values (I left one example, but I think there are many more edge cases that are not currently tested and should be rejected).
It would also be great to test the parser not only with well formed values that the current builders output but add test for values that should be rejected for various reasons. Not sure how the current parser can be tweaked to do this, but I think it would be good to add this before this can be merged.

cbuescher · 2017-11-21T14:26:35Z

core/src/main/java/org/elasticsearch/common/geo/GeoShapeType.java


    static {
+        BBOX = "BBOX";


nit: Can you move this assignment to L281 for better readability or is this unsafe to use it in the following initialization then?

cbuescher · 2017-11-21T14:34:54Z

core/src/main/java/org/elasticsearch/common/geo/GeoShapeType.java

        for (GeoShapeType type : values()) {
            shapeTypeMap.put(type.shapename, type);
+            // add 'BBOX' wkt name support for Envelope
+            if (type.equals(ENVELOPE)) {
+                shapeTypeMap.put(ENVELOPE.wktName().toLowerCase(Locale.ROOT), type);


Can this be done just after the values() loop?

cbuescher · 2017-11-21T14:36:28Z

core/src/main/java/org/elasticsearch/common/geo/GeoShapeType.java

@@ -300,6 +311,10 @@ public abstract ShapeBuilder getBuilder(CoordinateNode coordinates, DistanceUnit
                                            ShapeBuilder.Orientation orientation, boolean coerce);
    abstract CoordinateNode validate(CoordinateNode coordinates, boolean coerce);

+    public String wktName() {


nit: add some javadoc that the WKT identifier is the shapename.

cbuescher · 2017-11-21T14:38:08Z

core/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java

+
+    public String toWKT() {
+        StringBuilder sb = new StringBuilder();
+        sb.append(type().wktName());


Just curious, this will be lowercase, no? The examples in the issue and PR are all uppercase, does this matter?

Per the WKT spec, casing doesn't matter

cbuescher · 2017-11-21T14:43:14Z

core/src/main/java/org/elasticsearch/common/geo/builders/EnvelopeBuilder.java

+        sb.append(bottomRight.x);
+        sb.append(GeoWKTParser.COMMA);
+        sb.append(GeoWKTParser.SPACE);
+        // @todo support Z??


nit: doesn't really matter but mostly I see this as // TODO. Might be better to keep it similar to be able to grep similar things or for other tooling.

cbuescher · 2017-11-21T16:07:10Z

core/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java

+            throws IOException, ElasticsearchParseException {
+        CoordinatesBuilder coordinates = new CoordinatesBuilder();
+        coordinates.coordinate(parseCoordinate(stream));
+        while (nextCloserOrComma(stream).equals(COMMA)) {


Close or Comma will also again allow for things like "( 1, 2, 3) 4 )" being accepted, no?

Cleaned up coordinate list parsing to better handle coordinates with and without parens.

cbuescher · 2017-11-21T16:08:49Z

core/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java

+        if (nextEmptyOrOpen(stream).equals(EMPTY)) {
+            return null;
+        }
+        // todo orientation?


What needs to be done here?

Actually, nothing. This is handled in the builder. Removing todo.

cbuescher · 2017-11-21T16:10:02Z

core/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java

+        }
+        // todo orientation?
+        PolygonBuilder builder = new PolygonBuilder(parseLine(stream), ShapeBuilder.Orientation.RIGHT);
+        if (nextWord(stream).equals(LPAREN)) {


What if next word is something else than LPAREN?

cbuescher · 2017-11-21T16:11:21Z

core/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java

+        // todo orientation?
+        PolygonBuilder builder = new PolygonBuilder(parseLine(stream), ShapeBuilder.Orientation.RIGHT);
+        if (nextWord(stream).equals(LPAREN)) {
+            while (nextCloserOrComma(stream).equals(COMMA)) {


same problem as above I think, only that unmatched closing brackets will create holes here.

cbuescher · 2017-11-21T16:12:39Z

core/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java

+            case StreamTokenizer.TT_EOL: return EOL;
+            case StreamTokenizer.TT_NUMBER: return NUMBER;
+        }
+        return "'" + (char)stream.ttype + "'";


nit: whitespace

nknize · 2017-11-22T20:48:43Z

Thanks for the review @cbuescher

I went ahead and added more strict parsing (to include better paren parsing for coordinate lists). I also added tests for comments and illegal geometry types. I think this is close. If you wouldn't mind giving another review. I will try to add geometry collection testing before committing.

Also, since this is a fresh feature, what we can do is set this as experimental for 6.1 and flush it out in bug fixes. Thoughts?

cbuescher

@nknize I took another look and left two comments. In general I would still like to have more (and maybe not randomized, but simple edge cases) malformed tests, maybe a few typical cases per shape. Also Polygons with holes don't seem to get parsed correctly and I think tests are also missing for those cases.

cbuescher · 2017-11-23T15:24:10Z

core/src/test/java/org/elasticsearch/common/geo/GeoWKTShapeParserTests.java

+            RandomShapeGenerator.createShape(random(), RandomShapeGenerator.ShapeType.POLYGON));
+        Coordinate[] coords = builder.coordinates()[0][0];
+        LinearRing shell = GEOMETRY_FACTORY.createLinearRing(coords);
+        Polygon expected = GEOMETRY_FACTORY.createPolygon(shell, null);


Should this also work with holes? I just tried parsing the second polygon example from the issue decription and it fails (POLYGON ((35 10, 45 45, 15 40, 10 20, 35 10), (20 30, 35 35, 30 20, 20 30))) I suspect the second argument is a hole so I was trying to see of this was tested, seems like this needs to be added still.

👍 Good catch! I agree we should have tests for holes. This is a widely used use case. Will add that next.

Is it difficult to add testing at elast one hole here? If no I'd prefer to do so. GeoJsonShapeParserTests#testParseMultiPolygon() seems to do this as well, maybe something similar can be added here?

I added an explicit test for polygons with holes below in testParsePolygonWithHole, are you asking for a random polygon with random hole? That's a bit more tricky to generate a random polygon with a random inscribed polygon as a hole. Probably not really necessary as long as we've tested at least one polygon with a hole?

Yes, I saw the explicit test and I was hoping for a randomized polygon/hole thing, but I just realized that also GeoJsonShapeParserTests doesn't do this fully randomized. So if it is difficult I'm also fine if there is only the explicit test.

cbuescher · 2017-11-23T15:29:06Z

core/src/test/java/org/elasticsearch/common/geo/GeoWKTShapeParserTests.java

+    }
+
+    private void assertExpected(Shape expected, ShapeBuilder builder) throws IOException {
+        boolean isMalformed = randomBoolean();


I would prefer at least one good and one malformed case per test run. Otherwise we only catch bigger errors on multiple repetions and not e.g. on PR tests.

I'll add an explicit malformed test for common typos.

Thanks for adding the test, I would still prefer to test both variants (a malformed one and a correct one) on each test run for each random shape. It should be relatively easy to add this to the test given that all the parts (random shape generation, adding malformed content) is there.

nknize · 2017-11-27T18:48:28Z

@cbuescher just added explicit tests for the following:

polygon with hole (can add one for multi polygon w/ and w/o holes)
a malformed poly caused by common typos (can expand this test to include other common typos if we'd like).
self crossing polygon

Will update documentation but I think this is close to ready. Let me know what you think.

cbuescher

@nknize thanks, I left two more comments regarding tests. Also, since you mentioned docs, I think at least a short info about the possibility to use WKT for shapes should be part of this PR as well.

cbuescher · 2017-11-28T10:51:22Z

core/src/test/java/org/elasticsearch/common/geo/GeoWKTShapeParserTests.java

+    }
+
+    private void assertExpected(Shape expected, ShapeBuilder builder) throws IOException {
+        boolean isMalformed = randomBoolean();


Thanks for adding the test, I would still prefer to test both variants (a malformed one and a correct one) on each test run for each random shape. It should be relatively easy to add this to the test given that all the parts (random shape generation, adding malformed content) is there.

cbuescher · 2017-11-28T10:58:25Z

core/src/test/java/org/elasticsearch/common/geo/GeoWKTShapeParserTests.java

+            RandomShapeGenerator.createShape(random(), RandomShapeGenerator.ShapeType.POLYGON));
+        Coordinate[] coords = builder.coordinates()[0][0];
+        LinearRing shell = GEOMETRY_FACTORY.createLinearRing(coords);
+        Polygon expected = GEOMETRY_FACTORY.createPolygon(shell, null);


Is it difficult to add testing at elast one hole here? If no I'd prefer to do so. GeoJsonShapeParserTests#testParseMultiPolygon() seems to do this as well, maybe something similar can be added here?

cbuescher · 2017-11-29T12:32:32Z

@nknize thanks for updating the test, if the polygon + hole randomization is tricky, let's leave that out. I think this is ready once some documentation is added.

cbuescher · 2017-11-30T10:31:06Z

core/src/main/java/org/elasticsearch/common/geo/GeoUtils.java

+        }
+
+        /** gets the field name */
+        public String fieldName() {


Can you use get/setFieldName to make the method naming more consistent?

cbuescher · 2017-11-30T10:44:41Z

core/src/main/java/org/elasticsearch/common/geo/GeoUtils.java

@@ -50,6 +53,16 @@
    public static final String LATITUDE = "lat";
    public static final String LONGITUDE = "lon";
    public static final String GEOHASH = "geohash";
+    public static final ParseField FIELD_FIELD = new ParseField("field");


Is it possible to move all the BBOX parsing (ParseFields and static method) to the new GeoBBox class and make that one a top level class? Stuffing all of this code into the a Utils class doesn't feel optimal to me.

cbuescher · 2017-11-30T10:46:30Z

core/src/main/java/org/elasticsearch/common/geo/GeoUtils.java

@@ -549,6 +630,32 @@ public boolean advanceExact(int target) throws IOException {
        }
    }

+    /** wraps a {@link Rectangle} to include an optional field name for queries */
+    public static class GeoBBox extends Rectangle {


If it is only used by the GeoBoundingBoxQueryBuilder I'd probably make it an inner class there.

cbuescher · 2017-11-30T10:47:14Z

core/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java

@@ -61,7 +61,13 @@
    private GeoWKTParser() {}

    public static ShapeBuilder parse(XContentParser parser)
-            throws IOException, ElasticsearchParseException {
+        throws IOException, ElasticsearchParseException {


nit: weird indentation, could this throws clause move to the previous line

cbuescher · 2017-11-30T10:55:52Z

core/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java

+    }
+
+    /** parse geometry from the stream tokenizer */
+    private static ShapeBuilder parseGeometry(StreamTokenizer stream, GeoShapeType shapeType)


Is this extra method needed? I would combine it with the previous one that seems to be its only caller atm.

cbuescher · 2017-11-30T10:56:59Z

core/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java

+    /** parse geometry from the stream tokenizer */
+    private static ShapeBuilder parseGeometry(StreamTokenizer stream, GeoShapeType shapeType)
+            throws IOException, ElasticsearchParseException {
+        switch (shapeType) {


I get a warning for a missing GEOMETRYCOLLECTION case label here. Even if that shouldn't be possible, maybe add this to the switch (and/or a default case) that throws.

cbuescher · 2017-11-30T11:10:30Z

core/src/main/java/org/elasticsearch/common/geo/GeoUtils.java

+    /** wraps a {@link Rectangle} to include an optional field name for queries */
+    public static class GeoBBox extends Rectangle {
+        /** optional fieldname for queries */
+        private String fieldName;


Why do we need the fieldname as part of the parsed bbox structure. As far as I can see this gets only parsed when an inner field parameter is seen, the use of which seems both undocumented and it also doesn't get rendered when using the GeoBoundingBoxQueryBuilder#doXContent method. I have a hope that we can remove this maybe?

cbuescher · 2017-11-30T11:24:57Z

core/src/main/java/org/elasticsearch/common/geo/GeoUtils.java

+                currentFieldName = parser.currentName();
+                token = parser.nextToken();
+                if (FIELD_FIELD.match(currentFieldName)) {
+                    fieldName = parser.text();


Should this inner field parameter really still supported? I cannot find it mentioned in the docs and it doesn't seem to get written when using one of our builders. As far as I understand the whole bbox json should look like this:

"some.field.foo" : { ... various formats defining the box ... }

This is probably legacy and should be removed (probably in another PR?)

cbuescher · 2017-11-30T11:28:01Z

@nknize I just did another round of reviews on the last commit adding WKT parsing support to the GeoBoundingBoxQueryBuilder. Does this need to be part of this PR? I agree it should be added to complete the WKT support (don't know what needs to be done with the other query builders that can contain shapes), but since the existing PR is already big as is (and was close to merging when the documentation is added) I'd probably do this in a separate PR.

nknize · 2017-11-30T23:46:46Z

@cbuescher I agree. After I made the push I figured it would be best to do it in a separate PR. So I'll go ahead and do that. Going to commit the updated docs here shortly and consider this PR done (after the doc review).

nknize · 2017-12-01T00:37:44Z

@cbuescher added some WKT documentation to the geo-shape.asciidoc. If you're good with this I'll commit to 6.2 and 7.0 and open a separate PR for adding WKT support to GeoBoundingBox queries. Then move back to Z value support
/cc @clintongormley

cbuescher

@nknize thanks for pulling out the query builder changes, left a short question for the latest docs commit and apparently some console example still needs fixing, but other than that it looks good to me. Will give the rest another quick look but I think once the docs test passes this is good to merge.

cbuescher · 2017-12-01T09:30:31Z

docs/reference/mapping/types/geo-shape.asciidoc

+}
+--------------------------------------------------
+// CONSOLE
+// TEST[skip:https://github.com/elastic/elasticsearch/issues/23836]


Interesting, does the same problem describe in the bug not apply to the polygon example above?

cbuescher · 2017-12-01T10:03:59Z

docs/reference/mapping/types/geo-shape.asciidoc

+--------------------------------------------------
+POST /example/doc
+{
+    "location" : "POLYGON ((100.0 0.0, 101.0 0.0, 101.0 1.0, 100.0 1.0, 100.0 0.0),


gradle :docs:check fails here, maybe because of the line break. Not sure how to fix though.

If the line break is the problem, it's ok to have long lines. The code blocks on the docs will scroll

cbuescher · 2017-12-01T10:33:29Z

core/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java

+    /** parse geometry from the stream tokenizer */
+    private static ShapeBuilder parseGeometry(StreamTokenizer stream) throws IOException, ElasticsearchParseException {
+        final GeoShapeType type = GeoShapeType.forName(nextWord(stream));
+        switch (type) {


I get a warning here that GEOMETRYCOLLECTION is missing, looking at the added documentation this should be supported I think. If it is missing here as a type, we also need a test for it to catch this.

cbuescher · 2017-12-01T10:33:59Z

core/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java

+            case ENVELOPE:
+                return parseBBox(stream);
+        }
+        throw new IllegalArgumentException("Unknown geometry type: " + type);


nit: might throw in a DEFAULT case instead.

cbuescher · 2017-12-01T10:37:14Z

core/src/test/java/org/elasticsearch/common/geo/GeoWKTShapeParserTests.java

+        assertValidException(builder, IllegalArgumentException.class);
+    }
+
+    // todo: add geocollection


I think we need that test.

cbuescher

@nknize I took a brief look at the added geometry collection support and left a small suggstion on improving the test. Also documentation tests still need fixing, I think after that this PR is ready.

cbuescher · 2017-12-04T11:24:28Z

core/src/test/java/org/elasticsearch/common/geo/GeoWKTShapeParserTests.java

+        List<Coordinate> multiPointCoords = randomLineStringCoords();
+        MultiPointBuilder multiPointBuilder = new MultiPointBuilder(multiPointCoords);
+        GeometryCollectionBuilder builder = new GeometryCollectionBuilder()
+            .shape(polygonBuilder).shape(lineStringBuilder).shape(multiPointBuilder);


Can this be randomized so the GeometryCollectionBuilder has between 0 and e.g. 2 or 3 shapes?

nknize · 2017-12-04T17:55:13Z

Thanks for such a great review @cbuescher Looks like I've fixed the doc failures (at least on my test run, we'll see what elasticmachine has to say) and I added some randomization to GeometryCollection test. Pending everything passes green, I think this is good to go. Let me know what you think.

nknize · 2017-12-05T03:07:17Z

@elasticmachine please test this

cbuescher · 2017-12-05T10:36:47Z

core/src/test/java/org/elasticsearch/common/geo/GeoWKTShapeParserTests.java

-            // assert empty shape collection
-            assertEquals(shapeCollection(expected).isEmpty(), builder.build().isEmpty());
-        }
+        GeometryCollectionBuilder gcb = RandomShapeGenerator.createGeometryCollection(random());


Great to have this, but it doesn't seem to test the no-shapes case. Would be nice to add this as another case (maybe wrapped in if (rarely()), we don't need to test that edge case very often.

cbuescher

@nknize thanks for adding the randomization to the test, I left a small note regarding the test simplification. The rest looks good, and since the doc tests seem to pass I think this is ready to be merged once you get a clear CI pass.

This commit adds WKT support to Geo ShapeBuilders. This supports the following format: POINT (30 10) LINESTRING (30 10, 10 30, 40 40) BBOX (-10, 10, 10, -10) POLYGON ((30 10, 40 40, 20 40, 10 20, 30 10)) POLYGON ((35 10, 45 45, 15 40, 10 20, 35 10), (20 30, 35 35, 30 20, 20 30)) MULTIPOINT ((10 40), (40 30), (20 20), (30 10)) MULTIPOINT (10 40, 40 30, 20 20, 30 10) MULTILINESTRING ((10 10, 20 20, 10 40),(40 40, 30 30, 40 20, 30 10)) MULTIPOLYGON (((30 20, 45 40, 10 40, 30 20)), ((15 5, 40 10, 10 20, 5 10, 15 5))) MULTIPOLYGON (((40 40, 20 45, 45 30, 40 40)), ((20 35, 10 30, 10 10, 30 5, 45 20, 20 35), (30 20, 20 15, 20 25, 30 20))) GEOMETRYCOLLECTION (POINT (30 10), MULTIPOINT ((10 40), (40 30), (20 20), (30 10))) closes elastic#9120

synhershko · 2017-12-05T17:08:56Z

Yay! thanks!

nknize added :Analytics/Geo Indexing, search aggregations of geo points and shapes :Query DSL v6.1.0 v7.0.0 labels Nov 16, 2017

nknize requested a review from cbuescher November 16, 2017 23:00

cbuescher self-assigned this Nov 17, 2017

cbuescher requested changes Nov 21, 2017

View reviewed changes

nknize force-pushed the feature/WKTParsing branch from 54b9036 to bdd1380 Compare November 22, 2017 20:35

nknize force-pushed the feature/WKTParsing branch from 97b22a0 to fc6604d Compare November 23, 2017 04:47

cbuescher reviewed Nov 23, 2017

View reviewed changes

nknize added v6.2.0 and removed v6.1.0 labels Nov 23, 2017

nknize force-pushed the feature/WKTParsing branch 3 times, most recently from b80c0a4 to ae0a54d Compare November 27, 2017 18:46

cbuescher reviewed Nov 28, 2017

View reviewed changes

cbuescher requested changes Nov 30, 2017

View reviewed changes

nknize force-pushed the feature/WKTParsing branch from 0deb57c to 3c44e57 Compare November 30, 2017 23:45

nknize force-pushed the feature/WKTParsing branch from 00afb21 to f27b2e2 Compare December 1, 2017 00:36

cbuescher reviewed Dec 1, 2017

View reviewed changes

nknize force-pushed the feature/WKTParsing branch from f389b08 to bde013c Compare December 3, 2017 03:38

cbuescher reviewed Dec 4, 2017

View reviewed changes

nknize force-pushed the feature/WKTParsing branch from c70f729 to 4f73780 Compare December 4, 2017 17:59

cbuescher reviewed Dec 5, 2017

View reviewed changes

cbuescher approved these changes Dec 5, 2017

View reviewed changes

nknize force-pushed the feature/WKTParsing branch from 63ed67b to c6c063a Compare December 5, 2017 15:31

nknize force-pushed the feature/WKTParsing branch from c6c063a to 8bcf539 Compare December 5, 2017 16:59

nknize merged commit 8bcf539 into elastic:master Dec 5, 2017

This was referenced Dec 5, 2017

Adding another way to Create a polygon elastic/elasticsearch-net#2925

Closed

[GEO] Add WKT Support to GeoBoundingBoxQueryBuilder #27692

Merged

colings86 added the >enhancement label Jan 22, 2018

clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query DSL labels Feb 14, 2018

codebrain mentioned this pull request May 23, 2018

Add WKT Support to Geo elastic/elasticsearch-net#3256

Closed

jpountz removed the :Search/Search Search-related issues that do not fall into other categories label Jan 29, 2019

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

arteam mentioned this pull request Jan 5, 2022

Use Java 14 switch expressions #82178

Merged

[Geo] Add Well Known Text (WKT) Parsing Support to ShapeBuilders #27417

[Geo] Add Well Known Text (WKT) Parsing Support to ShapeBuilders #27417

Conversation

nknize commented Nov 16, 2017 • edited Loading

nknize commented Nov 18, 2017

cbuescher 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

nknize commented Nov 22, 2017

cbuescher 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

nknize commented Nov 27, 2017

cbuescher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher commented Nov 29, 2017

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

cbuescher commented Nov 30, 2017

nknize commented Nov 30, 2017

nknize commented Dec 1, 2017

cbuescher 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

cbuescher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nknize commented Dec 4, 2017

nknize commented Dec 5, 2017

Choose a reason for hiding this comment

cbuescher left a comment

Choose a reason for hiding this comment

synhershko commented Dec 5, 2017

nknize commented Nov 16, 2017 •

edited

Loading