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 basic copying functionality #712

Merged
merged 7 commits into from
Jun 17, 2023

Conversation

upsicleclown
Copy link
Collaborator

@upsicleclown upsicleclown commented Jun 12, 2023

Copy an existing .mbtiles file to a new file.

mbtiles copy  <src_file.mbtiles> <dst_file.mbtiles>

Optionally filters by zooms. Supports de-duplicated and simple mbtiles

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.

thanks! I left a few inline comments.

Overall, good approach, and I like that you attach the source rather than dst - might solve a few weird cases. My overall understanding of the process (correct if i miss something)

  • open source file in a read-only mode
  • detect mbtiles type + any other validations
  • close source file

  • open dst file
  • check that SELECT count(*) = 0 FROM sqlite_schema; returns 1, and error that we don't support copying to the non-empty mbtiles yet
  • set/update any initial pragmas/vacuum
  • attach source file
  • copy sql schema
  • copy data using one of the copy methods

I will think about this some more. Also, I suspect i might be wrong in overal structure, still thinking how to organize it best. One thing that kinda bothers me is that the API currently does not consume the options when it starts copying - a better path might be

  • a "builder" option struct CopyOptions
  • a function pub async fn copy_mbtiles(options: CopyOptions) (consumes options)
  • a bunch of private functions that actually do the copying

martin-mbtiles/src/queries/is_deduplicated_mbtiles.sql Outdated 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 Outdated 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 Outdated Show resolved Hide resolved
martin-mbtiles/src/tile_copier.rs Outdated Show resolved Hide resolved
@upsicleclown upsicleclown force-pushed the add-copying-mbtiles-functionality branch from 9912b32 to defea92 Compare June 14, 2023 20:41
@upsicleclown upsicleclown force-pushed the add-copying-mbtiles-functionality branch from defea92 to cc625de Compare June 14, 2023 20: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.

a few minor nits, but looks good! Need to implement some tests too... I wonder if we should implement some basic info dump first so that we can use them in the integration tests? Otherwise we won't have a good way to test if the copying actually happened... So perhaps mbtiles summary <file> that prints a nice output of sorts? It could show something like...

(definitly a separate PR, possibly later, this is not a blocker for this PR!)

 file: <file>
 type: deduplicated mbtiles
tiles: 123,456 tiles sized 10..45,455,574, 12,345.678 on average
available zooms:
  0: 2,000bytes
  1: 4 tiles sized 10..45,455,574, 12,345.678 on average
  2: 16 tiles sized 10..45,455,574, 12,345.678 on average
  ...
metadata:
  <all metadata fields here?>

martin-mbtiles/src/bin/main.rs Outdated Show resolved Hide resolved
martin-mbtiles/src/bin/main.rs Outdated Show resolved Hide resolved
martin-mbtiles/src/tile_copier.rs Outdated Show resolved Hide resolved
}
}

async fn copy_standard_compliant_tiles(&self, conn: &mut SqliteConnection) -> MbtResult<()> {
Copy link
Member

Choose a reason for hiding this comment

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

they are all standard compliant - need a better name that matches the ones in the Type enum. BTW, the Type enum seems kinda weird tbh, maybe MbtType instead for clarity, and consistency with the errors?

martin-mbtiles/src/tile_copier.rs Outdated Show resolved Hide resolved
martin-mbtiles/src/tile_copier.rs Outdated Show resolved Hide resolved
@nyurik
Copy link
Member

nyurik commented Jun 15, 2023

P.S. You will need to run just bless

@upsicleclown upsicleclown force-pushed the add-copying-mbtiles-functionality branch from c6a7c72 to 89e414d Compare June 15, 2023 22:05
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.

nits

martin-mbtiles/src/tile_copier.rs Outdated Show resolved Hide resolved
martin-mbtiles/src/tile_copier.rs Show resolved Hide resolved
Co-authored-by: Yuri Astrakhan <yuriastrakhan@gmail.com>
@upsicleclown upsicleclown force-pushed the add-copying-mbtiles-functionality branch from 40c5bc2 to 51efcbc Compare June 16, 2023 19:02
@nyurik nyurik marked this pull request as ready for review June 16, 2023 23:48
@nyurik nyurik enabled auto-merge (squash) June 16, 2023 23:51
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, thanks!

@nyurik nyurik merged commit bc140e0 into maplibre:main Jun 17, 2023
16 checks passed
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.

None yet

2 participants