Skip to content

feat: mbtiles hash extraction support#1787

Merged
CommanderStorm merged 19 commits intomaplibre:mainfrom
CommanderStorm:mbtiles-hashes
Apr 15, 2025
Merged

feat: mbtiles hash extraction support#1787
CommanderStorm merged 19 commits intomaplibre:mainfrom
CommanderStorm:mbtiles-hashes

Conversation

@CommanderStorm
Copy link
Member

(PR is based on #1786 => merge said PR first before looking at this one.)

This PR adds support to the mbtiles crate to hand out which hash corresponds to a tile.

The flat case might be debatable: Here I chose to md5 hash it to make upstream code more readable. The alternative would be to in this case return None for the hash.

This looks more intimidating than it actully is. Most of the lines are unit-tests.

Copilot AI review requested due to automatic review settings April 12, 2025 18:14
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

@CommanderStorm CommanderStorm marked this pull request as draft April 13, 2025 21:48
@CommanderStorm CommanderStorm marked this pull request as ready for review April 14, 2025 21:33
@CommanderStorm CommanderStorm requested a review from nyurik April 14, 2025 21:33
@nyurik
Copy link
Member

nyurik commented Apr 14, 2025

we really ought to just generate our own test files - perhaps have a short SQLite script that fake-creates a few mbtiles with well understood properties. This way instead of checking in binary blobs, we can check in and generate these files on the fiy, and have an easy to inspect SQL that shows what they have.

@CommanderStorm CommanderStorm enabled auto-merge (squash) April 14, 2025 21:51
Comment on lines +204 to +205
maxzoom: 6
minzoom: 0
Copy link
Member Author

Choose a reason for hiding this comment

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

Agree that generating test files might uncover bugs.
Here we are handling this too simplistic for example.

We are trusting that the metadata is actually valid.

Copy link
Member

Choose a reason for hiding this comment

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

yep, and actually generating sqlite files should be very simple - we can just use the .dump command in sqlite to export the content of all existing files, delete the originals, clean up the generated dumps, remove any unrelated stuff from them, and use them to generate .mbtiles files during testing - e.g. with some extra init-mbtiles just command to set them up in some build dir.

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.

this looks good, but I don't want to merge something that is not being used, i.e. dead code -- can you add usage of the new fn to martin itself?

@CommanderStorm
Copy link
Member Author

This is not actually dead code... mbtiles is a libary

The actual PR implementing ETAGs is a bit messy and needed another refactoring round before being an PR.
I would like to not merge them, to keep this reviewable.

We can leave it open and merge once I get to finishing that work.

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.

fair enough, lets merge, and thanks!

@CommanderStorm CommanderStorm merged commit fc19b62 into maplibre:main Apr 15, 2025
20 checks passed
@CommanderStorm CommanderStorm deleted the mbtiles-hashes branch April 15, 2025 17:32
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