Make rtree construction optional #21

karenzshea · 2017-10-02T14:10:33Z

Closes #19

To do:

Put RTree generation into a separate method of Database, rather than part of compact()
Add checks into annotateRouteFromLonLats() for RTree exists, return null or error if it doesn't
Add tests
Get review

daniel-j-h

👍 approach looks good so far!

daniel-j-h · 2017-10-02T14:16:52Z

src/database.cpp

@@ -1,10 +1,19 @@
 #include "database.hpp"

+Database::Database()
+{
+    createRTree = false;


You can set this already in the initializer list like you do below for the second constructor

daniel-j-h · 2017-10-02T14:19:06Z

src/nodejs_bindings.hpp

@@ -34,6 +34,7 @@ class Annotator final : public Nan::ObjectWrap
    static Nan::Persistent<v8::Function> &constructor();

    /* Wrapping Annotator; both database and annotator do not provide default ctor: wrap in ptr */
+    bool createRTree;


Maybe = false; here, too. Otherwise this member attribute is uninitialized until you write to it.

daniel-j-h · 2017-10-02T14:21:17Z

src/nodejs_bindings.cpp

+            {
+                coordinates = parsedCoords->BooleanValue();
+            } else return Nan::ThrowTypeError("Coordinates value should be a boolean"); // todo check what kind of error to throw
+        }


I think there's an else branch missing here for the IsEmpty check - if Get fails the object key isn't there

Hmm, I think it would be OK to let the default behavior here stay, which is that we would effectively ignore an empty options object.

But it also allows for the mistake where the user accidentally spells the option's key wrong

new Annotator({'coordinate': true}) new Annotator({'coordinates': true})

if the key really should be coordinates and is false by default the first line ^ does not set it to true but also does not error out.

daniel-j-h · 2017-10-02T14:21:49Z

src/nodejs_bindings.cpp

+            if (parsedCoords->IsBoolean())
+            {
+                coordinates = parsedCoords->BooleanValue();
+            } else return Nan::ThrowTypeError("Coordinates value should be a boolean"); // todo check what kind of error to throw


If you're done with this changeset don't forget to clang-format

daniel-j-h · 2017-10-02T14:22:42Z

src/database.cpp

-void Database::compact()
+Database::Database()
+{
+    createRTree = false;


You can set this already in the initializer list like you do below in the second constructor

daniel-j-h · 2017-10-02T14:23:22Z

src/database.cpp

-    rtree =
+}
+
+void Database::build_rtree()


I think this function can be marked const (also in header) because it does not modify the Database classes member attributes

See below, can't be marked as const :(

daniel-j-h · 2017-10-02T14:23:37Z

src/database.hpp

+     * Builds the RTree. Needs to be called after
+     * all OSM data parsing has been added.
+     */
+    void build_rtree();


Can be marked const I think

It ends up modifying the rtree member of the Database object

Ah sorry, you're right 👍

karenzshea · 2017-10-06T09:19:38Z

@danpat can you have a look at this today? Would be much appreciated! Also, let me know if you have ideas for more tests to add.

karenzshea · 2017-10-09T10:55:35Z

👋 @danpat Could you make time to review this today?

danpat

Changes look good to me, this is a useful change when folks don't want to do a coordinate lookup.

@karenzshea can you do a quick memory profile with/without the rtree and see what kind of memory savings it creates? I never measured it when I initially built this, it'd be interesting to see just what % of the total belonged to the rtree.

daniel-j-h · 2017-10-10T09:47:16Z

src/nodejs_bindings.cpp

+            }
+            else
+            {
+                return Nan::ThrowSyntaxError("Coordinates value should be a boolean");


this should be a type error - we're expecting a bool but it is not a bool

syntax error e.g. is v8 expecting each open parenthesis ( to be closed again ) but v8 was unable to find )

daniel-j-h · 2017-10-10T10:08:20Z

src/nodejs_bindings.cpp

+            }
+            catch (const RouteAnnotator::RtreeError &e)
+            {
+                std::cerr << e.what() << std::endl;


The printing to cerr here needs to be removed; in the Node.js bindings you should not print to stdout; you're running inside v8, so using the Javascript-bound error features is all you are allowed to do here.

daniel-j-h · 2017-10-10T10:11:13Z

test/load.test.js

+    var coords = [[-120.1872774,48.4715898],[-120.1882910,48.4725110]];
+    coordsFalse.annotateRouteFromLonLats(coords, (err) => {
+      t.equals(err.toString(), 'Error: Annotator not created with coordinates support');


Here and below - can we simply check for error if we check for the exact string then we have to keep it in sync in case the Node bindings slightly change the error message string.

quiqua · 2018-01-11T13:42:47Z

Any chance this might land in master soon?
Is there something I could help with?

karenzshea · 2018-01-11T17:34:43Z

@quiqua hello:) I've pushed some outstanding changes and once tests are passing, I'll merge and publish a release.

karenzshea added 2 commits October 2, 2017 16:08

don't construct RTree by default

83742e2

add build_rtree method

57beb15

daniel-j-h reviewed Oct 2, 2017

View reviewed changes

karenzshea added 4 commits October 2, 2017 17:49

make some members const

ba48da3

coordinates true in load test

7a2a9b5

clang format

d9c1097

error on coordinates request made to unbuilt rtree

5d1f1e0

karenzshea force-pushed the flag-rtree branch from 5e6ba46 to 5d1f1e0 Compare October 5, 2017 11:52

add tests for rtree building

8242d52

karenzshea requested a review from danpat October 5, 2017 15:33

danpat approved these changes Oct 10, 2017

View reviewed changes

daniel-j-h suggested changes Oct 10, 2017

View reviewed changes

edits

633655c

karenzshea force-pushed the flag-rtree branch from 45947fd to 633655c Compare January 11, 2018 16:18

add test for unrecognized options

4424cb5

karenzshea force-pushed the flag-rtree branch from 1c0a123 to cfb9b68 Compare January 24, 2018 16:57

add tests for invalid options

7a08403

karenzshea force-pushed the flag-rtree branch from cfb9b68 to 7a08403 Compare January 24, 2018 18:14

karenzshea merged commit bb68794 into master Jan 29, 2018

karenzshea deleted the flag-rtree branch January 29, 2018 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make rtree construction optional #21

Make rtree construction optional #21

karenzshea commented Oct 2, 2017 •

edited

Loading

daniel-j-h left a comment

daniel-j-h Oct 2, 2017

daniel-j-h Oct 2, 2017

daniel-j-h Oct 2, 2017

karenzshea Oct 2, 2017

daniel-j-h Oct 3, 2017

daniel-j-h Oct 2, 2017

daniel-j-h Oct 2, 2017

daniel-j-h Oct 2, 2017

daniel-j-h Oct 3, 2017

daniel-j-h Oct 2, 2017

karenzshea Oct 2, 2017

daniel-j-h Oct 3, 2017

karenzshea commented Oct 6, 2017

karenzshea commented Oct 9, 2017

danpat left a comment

daniel-j-h Oct 10, 2017

daniel-j-h Oct 10, 2017

karenzshea Oct 10, 2017

daniel-j-h Oct 10, 2017

quiqua commented Jan 11, 2018

karenzshea commented Jan 11, 2018

Make rtree construction optional #21

Make rtree construction optional #21

Conversation

karenzshea commented Oct 2, 2017 • edited Loading

daniel-j-h 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

karenzshea commented Oct 6, 2017

karenzshea commented Oct 9, 2017

danpat 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

quiqua commented Jan 11, 2018

karenzshea commented Jan 11, 2018

karenzshea commented Oct 2, 2017 •

edited

Loading