Convert get all tile coords to iterator #463
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
https://github.com/onthegomap/planetiler/actions/runs/4097987936 ℹ️ Base Logs 88daeb4ℹ️ This Branch Logs f35b83c |
|
A couple thoughts:
|
That could possibly work - where would something like tileOrder go though?
I don't see a need for it to return tiles in a specific order, I'd expect it to respect
That seems interesting if we needed to check tile IDs in random order, but I think a memory-efficient iterator would be simplest to implement. We could always layer something like that on top of an efficient iterator if the need arises. Also what do you think instead of iterating over the coordinates and tile data? |
|
Overall LGTM, if the main use case is for unit/integration tests then getting a total Set of all TileCoords is easier for pmtiles reading than the memory-efficient iterator. Iterating over coordinates and tile data can be awkward because of deduplication in both mbtiles/pmtiles as well as RLE in pmtiles, potentially we're feeding a consumer of that iterator a lot of extra work, unless we exposed not just |
|
OK let's start with this for now, at the very least any tile archive should be able to serve the getTile API and most likely listTiles as well, but if it ends up being problematic we could push the list API down into mbtiles and have a separate list API for different implementations. I could maybe see some key/value store supporting random access but not the ability to list tiles. |
|
Kudos, SonarCloud Quality Gate passed! |
bdon
reviewed
Feb 14, 2023
| } | ||
|
|
||
| /** Returns the raw tile data associated with the tile at coordinate {@code x, y, z}. */ | ||
| byte[] getTile(int x, int y, int z); |
There was a problem hiding this comment.
How should this handle the case of the tile not existing? should it be an option return type? It doesn't seem exceptional.
There was a problem hiding this comment.
Ah sorry the javadoc should indicate this, but it should return null if not found.
There was a problem hiding this comment.
Opened #486 to fix the javadoc.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull mbtiles access methods up to
TileArchiveand change signature for listing tile coordinates to return aCloseableIteratorso you can iterate through all coordinates in the archive without pulling the whole list into memory.