Skip to content
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

Add force-simple parameter when copying #723

Merged
merged 6 commits into from
Jun 23, 2023

Conversation

upsicleclown
Copy link
Collaborator

@upsicleclown upsicleclown commented Jun 21, 2023

Add --force-simple flag to mbtiles copy tool.
If supplied, ensures the destination file has a tiles table (as opposed to a tiles view made up of images and map tables).

@upsicleclown upsicleclown force-pushed the add-force-simple-copying branch from 4d2da6b to b73553d Compare June 21, 2023 21:33
@upsicleclown upsicleclown marked this pull request as ready for review June 21, 2023 23:13
@upsicleclown upsicleclown requested a review from nyurik June 21, 2023 23:19
Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great! love the :memory: usage for the tests, so much cleaner now. See inline

martin-mbtiles/src/tile_copier.rs Show resolved Hide resolved
martin-mbtiles/src/tile_copier.rs Outdated Show resolved Hide resolved
martin-mbtiles/src/tile_copier.rs Outdated Show resolved Hide resolved
@upsicleclown upsicleclown marked this pull request as draft June 21, 2023 23:42
Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For copying, seems like we may have a bug - when copying schema, we should copy tables before indexes. If we don't have any order by, it may fail

@@ -267,18 +279,40 @@ mod tests {
));
}

#[actix_rt::test]
async fn copy_force_simple() {
let src_filepath = PathBuf::from("../tests/fixtures/files/world_cities.mbtiles");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May want to call them same as other tests for consistency

This comment was marked as resolved.

Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing: please add this function to the tests (welcome to modify it if needed)

    async fn get_one<T>(conn: &mut SqliteConnection, sql: &str) -> T
    where
        for<'r> T: Decode<'r, Sqlite> + Type<Sqlite>,
    {
        query(sql).fetch_one(conn).await.unwrap().get::<T, _>(0)
    }

This way you can get a single value with simpler code:

get_one::<u8>(&mut dst_conn, "SELECT COUNT(DISTINCT zoom_level) FROM tiles;").await

instead of the one you have

query("SELECT COUNT(DISTINCT zoom_level) FROM tiles;")
                .fetch_one(&mut dst_conn)
                .await
                .unwrap()
                .get::<u8, _>(0),

@upsicleclown upsicleclown marked this pull request as ready for review June 22, 2023 20:38
@upsicleclown upsicleclown force-pushed the add-force-simple-copying branch from 43b4d3b to 1661128 Compare June 22, 2023 20:43
Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

almost there! a few comments

martin-mbtiles/src/bin/main.rs Outdated Show resolved Hide resolved
martin-mbtiles/src/tile_copier.rs Outdated Show resolved Hide resolved
martin-mbtiles/src/tile_copier.rs Show resolved Hide resolved
martin-mbtiles/src/tile_copier.rs Outdated Show resolved Hide resolved
martin-mbtiles/src/tile_copier.rs Outdated Show resolved Hide resolved
martin-mbtiles/src/tile_copier.rs Show resolved Hide resolved
@@ -267,18 +279,40 @@ mod tests {
));
}

#[actix_rt::test]
async fn copy_force_simple() {
let src_filepath = PathBuf::from("../tests/fixtures/files/world_cities.mbtiles");

This comment was marked as resolved.

Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome, thank you!

assert_eq!(
Args::try_parse_from(["mbtiles", "copy"])
.unwrap_err()
.kind(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice find!

@nyurik nyurik enabled auto-merge (squash) June 23, 2023 01:50
@nyurik nyurik merged commit 177b72e into maplibre:main Jun 23, 2023
@upsicleclown upsicleclown deleted the add-force-simple-copying branch June 23, 2023 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants