Landmark Routing 1 PR: Add primary key for landmark database and landmark getter via primary key #4224

vesperlou · 2023-07-27T18:12:59Z

Issue

To faciliate landmark storage in tiles, primary key is added in landmark databse to enable getting a landmark via its primary key.

Tasklist

add primary key to the landmarks database
add a method to be able to retreive a landmark from the db via its primary key

Requirements / Relations

Landmark Routing 3: Tile Storage of Associated Landmarks

nilsnolde · 2023-07-28T11:10:32Z

src/mjolnir/landmark_builder.cc

    ret = sqlite3_prepare_v2(db, select, strlen(select), &bounding_box_stmt, NULL);
    if (ret != SQLITE_OK) {
      throw std::runtime_error("Sqlite prepared select statement error: " +
                               std::string(sqlite3_errmsg(db)));
    }
+
+    // prep the landmark getter statement
+    const char* get_landmark = "SELECT id, name, type, X(geom), Y(geom) FROM landmarks WHERE id = ?";


for clarity I'd rename get_landmark to get_landmark_by_id and everything bbox related to get_landmarks_by_bbox (note the plural).

you meant the function name below in this case not the prepared statement right?

both but primarily the (public) function name, right. the prepare statement and variable names could/should be changed similarly, but that's more of a nit

if we decided we wanted batching we'd either have to not use a prepared statement OR use one with a set level of batching. the latter would be like:

Suggested change

const char* get_landmark = "SELECT id, name, type, X(geom), Y(geom) FROM landmarks WHERE id = ?";

const char* get_landmark = "SELECT id, name, type, X(geom), Y(geom) FROM landmarks WHERE id IN (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)";

or maybe even programatically adding more question marks, and then when we fill them out in the query below we'd need to just repeat the last one until we've filled up all the question marks. if the batch size the person gave us in the vector was too small. and do more than 1 batch if the batch size the person gave us in the vector was too large

I've added get_landmarks_by_ids to flexibly fetch multiple landmarks by ids. the statement is not prepared in advance anymore and no min/max limit of landmark number is set. maybe we should set a max number?

src/mjolnir/landmark_builder.cc

nilsnolde · 2023-07-28T11:15:48Z

src/mjolnir/landmark_builder.cc

+    int landmark_type = -1;
+    if (sqlite3_column_type(get_landmark_stmt, 2) != SQLITE_NULL) {
+      landmark_type = sqlite3_column_int(get_landmark_stmt, 2);
+    }


this can't happen can it? if so, we should throw I think. passing on a value of -1 would do crazy things.

i agree, i think we can just skip the if completely and set the type without checking null. we dont ever put a null entry in the db so we cant ever get a null entry out

nilsnolde · 2023-07-28T11:16:56Z

src/mjolnir/landmark_builder.cc

+    if (sqlite3_column_type(bounding_box_stmt, 2) != SQLITE_NULL) {
+      landmark_type = sqlite3_column_int(bounding_box_stmt, 2);


same comment holds here, type can't be optional

nilsnolde

some nits:)

src/mjolnir/landmark_builder.cc

kevinkreiser · 2023-07-28T12:33:55Z

src/mjolnir/landmark_builder.cc

@@ -202,17 +245,18 @@ std::vector<Landmark> LandmarkDatabase::get_landmarks_in_bounding_box(const doub

  int ret = sqlite3_step(bounding_box_stmt);
  while (ret == SQLITE_ROW) {
-    const char* name = reinterpret_cast<const char*>(sqlite3_column_text(bounding_box_stmt, 0));
+    uint32_t landmark_id = static_cast<uint32_t>(sqlite3_column_int(bounding_box_stmt, 0));


Suggested change

uint32_t landmark_id = static_cast<uint32_t>(sqlite3_column_int(bounding_box_stmt, 0));

auto landmark_id = static_cast<int64_t>(sqlite3_column_int64(bounding_box_stmt, 0));

kevinkreiser · 2023-07-28T12:34:34Z

valhalla/mjolnir/landmark_builder.h

@@ -60,7 +60,7 @@ enum class LandmarkType : uint8_t {
  casino = 18,
 };

-using Landmark = std::tuple<std::string, LandmarkType, double, double>;
+using Landmark = std::tuple<uint32_t, std::string, LandmarkType, double, double>;


Suggested change

using Landmark = std::tuple<uint32_t, std::string, LandmarkType, double, double>;

using Landmark = std::tuple<int64_t, std::string, LandmarkType, double, double>;

kevinkreiser · 2023-07-28T12:35:14Z

valhalla/mjolnir/landmark_builder.h

  std::vector<Landmark> get_landmarks_in_bounding_box(const double minLat,
                                                      const double minLong,
                                                      const double maxLat,
                                                      const double maxLong);

+  Landmark get_landmark(const uint32_t pkey);


Suggested change

Landmark get_landmark(const uint32_t pkey);

Landmark get_landmark(const int64_t pkey);

kevinkreiser · 2023-07-28T12:36:30Z

src/mjolnir/landmark_builder.cc

@@ -183,6 +194,38 @@ void LandmarkDatabase::insert_landmark(const std::string& name,
  pimpl->vacuum_analyze = true;
 }

+Landmark LandmarkDatabase::get_landmark(const uint32_t pkey) {


i wonder if we should allow batching, this could improve efficiency when we are fetching these things:

Suggested change

Landmark LandmarkDatabase::get_landmark(const uint32_t pkey) {

std::vector<Landmark> LandmarkDatabase::get_landmark(const std::vector<int64_t>& pkeys) {

good idea. maybe we should allow both cases

test/gurka/test_landmarks.cc

nilsnolde · 2023-08-01T12:42:17Z

src/mjolnir/landmark_builder.cc

+    if (i > 0) {
+      sql += ", ";
+    }
+    sql += "?";


we can put the actual value here instead of ? and omit preparing the statement

done. but i notice sqlite officially suggests to use preparing statement when query includes variables.

Likely because they’re doing some internal validation of the variable type or so. I think in our use case here it’s fine to do as it is now, it’s not a multi-variable statement, just the same many times.

nilsnolde · 2023-08-01T12:44:13Z

src/mjolnir/landmark_builder.cc

-                                                                      const double maxLong) {
+// get multiple landmarks by their ids
+std::vector<Landmark> LandmarkDatabase::get_landmarks_by_ids(const std::vector<int64_t>& pkeys) {
+  sqlite3_stmt* get_landmarks_by_ids_stmt = nullptr;


we should put a TODO to test how this performs and see if we should prepare a default query (like 10-20 elements) and if more come in we extend the statement preparation

nilsnolde · 2023-08-01T23:37:31Z

valhalla/mjolnir/landmark_builder.h

@@ -60,7 +60,7 @@ enum class LandmarkType : uint8_t {
  casino = 18,
 };

-using Landmark = std::tuple<std::string, LandmarkType, double, double>;
+using Landmark = std::tuple<int64_t, std::string, LandmarkType, double, double>;


Hm I just realized this: not that we really need it but why not uint64_t for the index?

because the primary key has to be a signed 64 bit integer to get a free index on that column in sqlite. why they picked signed i do not know but if we want to use unsigned that means we have to add our own index. anyway this is what i read on the internet. if we were going to do something like that we should forget the primary key and use the osmid directly and add an index manually (this could be useful for debugging at some point?) but i figured at the moment its not needed, it did cross my mind though!

kevinkreiser · 2023-08-02T01:47:16Z

valhalla/mjolnir/landmark_builder.h

+   * database connection is read-only or read-write.
+   * @param db_name The file path of the SQLite database to connect to.
+   * @param read_only Set to true to open the database in read-only mode, false for read-write.


very nitpicky but we typically do this extra white space:

Suggested change

* database connection is read-only or read-write.

* @param db_name The file path of the SQLite database to connect to.

* @param read_only Set to true to open the database in read-only mode, false for read-write.

* database connection is read-only or read-write.

*

* @param db_name The file path of the SQLite database to connect to.

* @param read_only Set to true to open the database in read-only mode, false for read-write.

kevinkreiser · 2023-08-02T01:48:01Z

@nilsnolde are you good with the changes? anything else needed here?

vesperlou · 2023-08-02T06:52:39Z

oops @nilsnolde @kevinkreiser I add a new commit to add extra space in docstrings after your approvals. can either of you review again? :)

nilsnolde

🚢

… key (valhalla#4224)

add primary key and landmark getter for landmark database

4aef18e

vesperlou requested review from nilsnolde and kevinkreiser July 27, 2023 18:13

update CHANGELOG.md

3fdf223

nilsnolde reviewed Jul 28, 2023

View reviewed changes

src/mjolnir/landmark_builder.cc Outdated Show resolved Hide resolved

nilsnolde reviewed Jul 28, 2023

View reviewed changes

nilsnolde requested changes Jul 28, 2023

View reviewed changes

kevinkreiser reviewed Jul 28, 2023

View reviewed changes

src/mjolnir/landmark_builder.cc Show resolved Hide resolved

kevinkreiser reviewed Jul 28, 2023

View reviewed changes

vesperlou added 3 commits July 28, 2023 16:53

address code reviews

3d3764c

update public function names

48eb0e5

support getting multiple landmarks by ids flexibly

d666329

vesperlou requested review from kevinkreiser and nilsnolde July 28, 2023 16:53

update CHANGELOG.md

21bbe54

nilsnolde reviewed Jul 30, 2023

View reviewed changes

test/gurka/test_landmarks.cc Show resolved Hide resolved

add more tests for getting landmarks by ids

5942eb9

vesperlou requested a review from nilsnolde July 31, 2023 07:07

vesperlou added 3 commits July 31, 2023 11:26

add docstrings for public interfaces

129603a

format

e4d48fa

refine docstrings

c0f66ad

nilsnolde reviewed Aug 1, 2023

View reviewed changes

remove prepare statement in get_landmarks_by_ids and add note

e5f56f5

vesperlou requested a review from nilsnolde August 1, 2023 15:41

nilsnolde reviewed Aug 1, 2023

View reviewed changes

kevinkreiser reviewed Aug 2, 2023

View reviewed changes

kevinkreiser previously approved these changes Aug 2, 2023

View reviewed changes

nilsnolde previously approved these changes Aug 2, 2023

View reviewed changes

add extra white space in docstrings

69f88fa

vesperlou dismissed stale reviews from nilsnolde and kevinkreiser via 69f88fa August 2, 2023 06:48

nilsnolde approved these changes Aug 2, 2023

View reviewed changes

nilsnolde merged commit 8b0155b into master Aug 2, 2023
8 checks passed

nilsnolde deleted the jz_landmark_database_add_primary_key branch August 2, 2023 08:13

eikes pushed a commit to eikes/valhalla that referenced this pull request Aug 28, 2023

Add primary key for landmark database and landmark getter via primary…

5083cb7

… key (valhalla#4224)

vesperlou changed the title ~~Add primary key for landmark database and landmark getter via primary key~~ Landmark Routing 1 PR: Add primary key for landmark database and landmark getter via primary key Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Landmark Routing 1 PR: Add primary key for landmark database and landmark getter via primary key #4224

Landmark Routing 1 PR: Add primary key for landmark database and landmark getter via primary key #4224

vesperlou commented Jul 27, 2023 •

edited

Loading

nilsnolde Jul 28, 2023 •

edited

Loading

kevinkreiser Jul 28, 2023 •

edited

Loading

nilsnolde Jul 28, 2023

kevinkreiser Jul 28, 2023

vesperlou Jul 28, 2023

nilsnolde Jul 28, 2023

kevinkreiser Jul 28, 2023

vesperlou Jul 28, 2023

nilsnolde Jul 28, 2023 •

edited

Loading

nilsnolde left a comment

kevinkreiser Jul 28, 2023 •

edited

Loading

kevinkreiser Jul 28, 2023

kevinkreiser Jul 28, 2023

kevinkreiser Jul 28, 2023

vesperlou Jul 28, 2023

nilsnolde Aug 1, 2023

vesperlou Aug 1, 2023

nilsnolde Aug 1, 2023

nilsnolde Aug 1, 2023

vesperlou Aug 1, 2023

nilsnolde Aug 1, 2023

kevinkreiser Aug 2, 2023

kevinkreiser Aug 2, 2023

vesperlou Aug 2, 2023

kevinkreiser commented Aug 2, 2023

vesperlou commented Aug 2, 2023

nilsnolde left a comment

	const char* get_landmark = "SELECT id, name, type, X(geom), Y(geom) FROM landmarks WHERE id = ?";
	const char* get_landmark = "SELECT id, name, type, X(geom), Y(geom) FROM landmarks WHERE id IN (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)";

		if (sqlite3_column_type(bounding_box_stmt, 2) != SQLITE_NULL) {
		landmark_type = sqlite3_column_int(bounding_box_stmt, 2);

	uint32_t landmark_id = static_cast<uint32_t>(sqlite3_column_int(bounding_box_stmt, 0));
	auto landmark_id = static_cast<int64_t>(sqlite3_column_int64(bounding_box_stmt, 0));

	using Landmark = std::tuple<uint32_t, std::string, LandmarkType, double, double>;
	using Landmark = std::tuple<int64_t, std::string, LandmarkType, double, double>;

	Landmark get_landmark(const uint32_t pkey);
	Landmark get_landmark(const int64_t pkey);

	Landmark LandmarkDatabase::get_landmark(const uint32_t pkey) {
	std::vector<Landmark> LandmarkDatabase::get_landmark(const std::vector<int64_t>& pkeys) {

Landmark Routing 1 PR: Add primary key for landmark database and landmark getter via primary key #4224

Landmark Routing 1 PR: Add primary key for landmark database and landmark getter via primary key #4224

Conversation

vesperlou commented Jul 27, 2023 • edited Loading

Issue

Tasklist

Requirements / Relations

nilsnolde Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

kevinkreiser Jul 28, 2023 • edited Loading

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

nilsnolde Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

nilsnolde left a comment

Choose a reason for hiding this comment

kevinkreiser Jul 28, 2023 • edited Loading

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

kevinkreiser commented Aug 2, 2023

vesperlou commented Aug 2, 2023

nilsnolde left a comment

Choose a reason for hiding this comment

vesperlou commented Jul 27, 2023 •

edited

Loading

nilsnolde Jul 28, 2023 •

edited

Loading

kevinkreiser Jul 28, 2023 •

edited

Loading

nilsnolde Jul 28, 2023 •

edited

Loading

kevinkreiser Jul 28, 2023 •

edited

Loading