Skip to content

feat(mbtiles): add Mbtiles::stream_coords and Mbtiles::stream_tiles#1847

Merged
CommanderStorm merged 12 commits intomaplibre:mainfrom
Murtaught:mbtiles-streams
May 24, 2025
Merged

feat(mbtiles): add Mbtiles::stream_coords and Mbtiles::stream_tiles#1847
CommanderStorm merged 12 commits intomaplibre:mainfrom
Murtaught:mbtiles-streams

Conversation

@Murtaught
Copy link
Contributor

This PR adds two new methods to Mbtiles struct from mbtiles crate.

  • Mbtiles::stream_coords() returns a Stream (aka "async iterator") over all tile indexes in the database (TileCoord struct).
  • Mbtiles::stream_tiles() returns a stream over all tiles, i.e. both tile index and its data. A type alias pub type Tile = (TileCoord, Option<Vec<u8>>) is introduced to martin-tile-utils crate.

This allows us to efficiently iterate over all tiles in an .mbtiles file.

Discussion of this feature is here.

Murtaught added 3 commits May 23, 2025 18:30
- [TileCoord::checked()]
- [TileCoord::checked_inverted()]
- [Mbtiles::stream_tiles()] - streams tile coords + their data, and
- [Mbtiles::stream_coords()] - only streams tile coords.

By "streaming" I mean [futures::Stream] trait, aka "async iterator".
@Murtaught
Copy link
Contributor Author

Ooops, using anyhow::Result in tests was a mistake. I'll fix that and rebase on top of main.

Copy link
Member

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

I have attached two minor refactorings/renamings.
(this is a public api => naming is here important)

Comment on lines 352 to 373
fn parse_tile_index(z: Option<i64>, x: Option<i64>, y: Option<i64>) -> Option<TileCoord> {
let z: u8 = z?.try_into().ok()?;
let x: u32 = x?.try_into().ok()?;
let y: u32 = y?.try_into().ok()?;
TileCoord::checked_inverted(z, x, y)
}

fn validate_tile_index(
z: Option<i64>,
x: Option<i64>,
y: Option<i64>,
filepath: &str,
) -> MbtResult<TileCoord> {
parse_tile_index(z, x, y).ok_or_else(|| {
MbtError::InvalidTileIndex(
filepath.to_string(),
format!("{z:?}"),
format!("{x:?}"),
format!("{y:?}"),
)
})
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite like how this code snakes four functions deep, most of which are minimal code.
This is a bit unnecessary.

Lets inline these two at least:

  • validate_tile_index
  • checked_inverted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair points.

Let's get rid of checked_inverted().

But I didn't quite get it how you want me to inline validate_tile_index(). Should I duplicate error creation logic in both stream_tiles() and stream_coords()?

Murtaught and others added 2 commits May 24, 2025 21:11
@Murtaught Murtaught requested a review from CommanderStorm May 24, 2025 18:24
Copy link
Member

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Great job, thanks

@CommanderStorm CommanderStorm enabled auto-merge (squash) May 24, 2025 19:08
@CommanderStorm CommanderStorm changed the title Tile streaming for [Mbtiles] feat(mbtiles): add Mbtiles::stream_coords and Mbtiles::stream_tiles May 24, 2025
@CommanderStorm CommanderStorm merged commit 70f48ca into maplibre:main May 24, 2025
20 checks passed
@Murtaught Murtaught deleted the mbtiles-streams branch May 24, 2025 19:55
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

Comments