Use rtree spatial index from PlanetilerConfig bounds by default for Geopackages #635
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
|
Kudos, SonarCloud Quality Gate passed! |
https://github.com/onthegomap/planetiler/actions/runs/5678745487 ℹ️ Base Logs 5e7159dℹ️ This Branch Logs b2d4a34 |
erik
reviewed
Jul 27, 2023
Comment on lines
+152
to
+154
| FeatureIndexManager indexer = new FeatureIndexManager(geoPackage, | ||
| features); | ||
| indexer.setIndexLocation(FeatureIndexType.RTREE); |
There was a problem hiding this comment.
It's been a while, but if I remember correctly, this will actually build the index if it doesn't already exist in the DB before running the query.
Looking at the PR where I initially added this, should be possible to check for the presence of the index before trying to use it: #413 (comment)
There was a problem hiding this comment.
That seems awkward and unexpected behavior to modify the database in-place, can we disable that? If it's being unzipped every time it could be slow for a large geopackage.
Maybe detect the absence of the index and ignore?
There was a problem hiding this comment.
(nvm, I see you mentioned exactly that behavior I suggested) so the next step is addressing possible projection mismatches; we need to find some geopackages in the wild that are neither WGS84 or webmerc
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.
This is a first attempt at addressing #478 .
The existing issue is that all Shapefile or Geopackage features are read by every run, even if the
--boundsargument is passed to limit the tile output or the bounds are determined implicitly based on theosm.pbf. To reduce the runtime we can:.fbn,.fbxand/orqixsidecars depending on what software created the Shapefile. Maybe GeoTools can read all of these, but I'm not familiar with its internals.rtree_indexes (A vanilla SQLite feature, no Spatialite required)ne_50m_admin_0_boundary_lines_landas tables we care about. IMO, this doesn't reduce the runtime by much relative to spatial indexing, so it may be an unworthy optimization.My proposal here is to not change any APIs, but change the default behavior of Geopackage to use the determined
PlanetilerConfigbounds and apply the spatial index. Remaining todos:rtree_indexes will always exist on gpkg or are optionalAnother relevant format would be FlatGeobuf, which has an optional spatial index as part of the single file, but I don't see key advantages of FGB over GPKG in most Planetiler use cases where data lives on disk.