fix: directory offset encoding by michaelkirk · Pull Request #111 · stadiamaps/pmtiles-rs

michaelkirk · 2026-02-20T22:22:00Z

Note: I used an LLM to port the tests, but I visually inspected them and they appear equivalent.

michaelkirk · 2026-02-20T22:23:35Z

-            let offset_to_write = if entry.offset == last_offset + u64::from(entry.length) {
+        let mut last_length = 0;
+        for (i, entry) in self.entries.iter().enumerate() {
+            let offset_to_write = if i > 0 && entry.offset == last_offset + u64::from(last_length) {


Compare to:

https://github.com/protomaps/go-pmtiles/blob/39722fde57646c59c23e67a8cf739a59fc657590/pmtiles/directory.go#L312

nyurik · 2026-02-20T22:24:32Z

+    #[cfg(feature = "write")]
+    fn directory_roundtrip_gzip() {
+        let entries = vec![
+            DirEntry {


please add a helper for instantiating DirEntry, or maybe a new() fn, possibly with pub(crate) to not expose it -- so that all this becomes readable oneliners

if you really want this readable by humans, you may instead make a test-only helper like fn entry_id_off_len_rl(...) ... but totally up to you

or DirEntry::from_id_off_len_rl(...) ?

It would result in fewer lines of code, but I disagree that this would be a readability improvement. I admit it's subjective though. Is this a blocker for getting this bug fix merged?

I would agree for the regular code, but the tests are already fairly verbose. I am ok to fix it myself if you want - up to you

Yes, thanks! Please fix up the test phrasing as you see fit.

@nyurik - I'd like to see this bugfix merged. Can you apply your preferred test styling?

Copilot

Pull request overview

Fixes PMTiles directory offset varint encoding and expands directory-related unit tests to better align with upstream go-pmtiles behavior.

Changes:

Correct offset “same-as-previous+length” encoding to use the previous entry’s length (and avoid emitting 0 for the first entry).
Derive PartialEq for DirEntry to support equality assertions in tests.
Add multiple directory roundtrip and find_tile_id behavior tests (ported from go-pmtiles).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-20T22:26:25Z

+
+    // Ported from go-pmtiles/pmtiles/directory_test.go:TestDirectoryRoundtrip (line 10)
+    #[test]
+    #[cfg(feature = "write")]
+    fn directory_roundtrip_gzip() {
+        let entries = vec![
+            DirEntry {


The new roundtrip tests don’t exercise the specific offset-encoding bug fixed above (writing 0 when entry.offset == last_offset + entry.length instead of using the previous entry’s length). Add a case where the previous and current lengths differ and the old encoding would have produced an incorrect decoded offset (e.g., prev: offset=0,len=1; next: offset=2,len=2) to ensure this regression is covered.

codecov · 2026-02-20T23:32:53Z

Codecov Report

❌ Patch coverage is 97.97980% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.88%. Comparing base (9983294) to head (c3df928).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/directory.rs	97.97%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #111      +/-   ##
==========================================
+ Coverage   79.97%   80.88%   +0.91%     
==========================================
  Files          12       12              
  Lines        1648     1732      +84     
  Branches     1648     1732      +84     
==========================================
+ Hits         1318     1401      +83     
- Misses        225      226       +1     
  Partials      105      105

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

nyurik

thanks!

nyurik · 2026-04-17T19:45:32Z

sigh, seems like there are some object-store related issues and the new tests... I tried fixing them, but it didn't really help. @michaelkirk could you take a look? I am not certain my fix is the right one

nyurik · 2026-04-17T21:55:14Z

I wonder if tokio feature should be made always-on in object-store dep

michaelkirk · 2026-04-17T22:10:33Z

I'm pulling out the "fix the build" stuff into another PR. Apparently it's still WIP #119

Note: 2 are currently failing: directory::tests::directory_roundtrip_gzip directory::tests::directory_roundtrip_no_compression

The contiguous-offset check compared against the current entry's length instead of the previous entry's length, producing corrupt output when consecutive entries had different lengths. Also add an `i > 0` guard so the first entry always encodes its full offset (a value of 0 is reserved to mean "contiguous with previous").

## 🤖 New release * `pmtiles`: 0.21.0 -> 0.22.0 (⚠ API breaking changes) ### ⚠ `pmtiles` breaking changes ```text --- failure auto_trait_impl_removed: auto trait no longer implemented --- Description: A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented. ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/auto_trait_impl_removed.ron Failed in: type PmTilesWriter is no longer Send, in /tmp/.tmpQE4q9u/pmtiles-rs/src/writer/mod.rs:23 type PmTilesWriter is no longer Sync, in /tmp/.tmpQE4q9u/pmtiles-rs/src/writer/mod.rs:23 type PmTilesWriter is no longer UnwindSafe, in /tmp/.tmpQE4q9u/pmtiles-rs/src/writer/mod.rs:23 type PmTilesWriter is no longer RefUnwindSafe, in /tmp/.tmpQE4q9u/pmtiles-rs/src/writer/mod.rs:23 type PmTilesStreamWriter is no longer Send, in /tmp/.tmpQE4q9u/pmtiles-rs/src/writer/mod.rs:36 type PmTilesStreamWriter is no longer Sync, in /tmp/.tmpQE4q9u/pmtiles-rs/src/writer/mod.rs:36 type PmTilesStreamWriter is no longer UnwindSafe, in /tmp/.tmpQE4q9u/pmtiles-rs/src/writer/mod.rs:36 type PmTilesStreamWriter is no longer RefUnwindSafe, in /tmp/.tmpQE4q9u/pmtiles-rs/src/writer/mod.rs:36 ``` <details><summary>Changelog</summary> <blockquote> ## [0.22.0](v0.21.0...v0.22.0) - 2026-04-18 ### Fixed - directory offset encoding ([#111](#111)) ### Other - update object_store dependency and fix build ([#119](#119)) - Configurable compression parameters via Compressor trait ([#112](#112)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/).

Copilot AI review requested due to automatic review settings February 20, 2026 22:22

Copilot started reviewing on behalf of michaelkirk February 20, 2026 22:22 View session

michaelkirk commented Feb 20, 2026

View reviewed changes

nyurik reviewed Feb 20, 2026

View reviewed changes

Copilot AI reviewed Feb 20, 2026

View reviewed changes

michaelkirk force-pushed the mkirk/fix-directory-offset-encoding branch from 4784d3a to 76d86fe Compare February 20, 2026 22:27

nyurik approved these changes Apr 17, 2026

View reviewed changes

michaelkirk force-pushed the mkirk/fix-directory-offset-encoding branch from 4f5c7e4 to 34d9b0a Compare April 17, 2026 21:34

michaelkirk and others added 3 commits April 17, 2026 20:30

port tests from go-pmtiles

369c4d2

Note: 2 are currently failing: directory::tests::directory_roundtrip_gzip directory::tests::directory_roundtrip_no_compression

minor test cleanup

c3df928

nyurik force-pushed the mkirk/fix-directory-offset-encoding branch from 4c1fa6c to c3df928 Compare April 18, 2026 00:30

nyurik changed the title ~~Fix directory offset encoding~~ fix: directory offset encoding Apr 18, 2026

nyurik approved these changes Apr 18, 2026

View reviewed changes

nyurik merged commit 1f03a20 into stadiamaps:main Apr 18, 2026
8 checks passed

lseelenbinder mentioned this pull request Apr 18, 2026

chore: release v0.22.0 #115

Merged

Conversation

michaelkirk commented Feb 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nyurik left a comment

Choose a reason for hiding this comment

Uh oh!

nyurik commented Apr 17, 2026

Uh oh!

nyurik commented Apr 17, 2026

Uh oh!

michaelkirk commented Apr 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Feb 20, 2026 •

edited

Loading