Add basic write support #45

pka · 2024-10-19T14:32:57Z

This is a minimal writer implementation. Before implementing the missing parts, I would like to discuss the basic design.

Main questions:

Should we prepare for other backends (e.g. S3)?
- (Trait could be done later)
Do we want an async API?
- Probably depends on 1)

Missing functionality:

nyurik · 2024-10-21T05:39:39Z

wrt 1 - you mean writing directly to s3? Does s3 allow random access of the already open file (we would need to go back to the directory to update file positions i would think)

pka · 2024-10-21T07:11:13Z

AFAIK, S3 doesn't support seek operations or partial updates, but with a multipart upload, you can defer uploading header and root directory. So a direct upload to S3 would be possble. But this has no high priority for me.

nyurik · 2024-10-21T07:18:54Z

TIL, thx. And lets follow YAGNI - if someone wants it, they will add it :)

pka · 2024-10-23T21:42:02Z

This is now ready for a review.

I realized an important property, which I want to add as a followup: deduplicating non-subsequent tiles. This has performance drawbacks when reading, but probably reduces the file size substantially.

nyurik

looks pretty good! I left a few thoughts

justfile

src/async_reader.rs

src/directory.rs

src/lib.rs

src/writer.rs

pka · 2024-10-24T17:42:58Z

Regarding the failed semver check:

--- failure enum_variant_added: enum variant added on exhaustive enum ---

Description:
A publicly-visible enum without #[non_exhaustive] has a new variant.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/enum_variant_added.ron

Failed in:

     Summary semver requires new major version: 1 major and 0 minor checks failed
  variant PmtError:IndexEntryOverflow in /home/runner/work/pmtiles-rs/pmtiles-rs/src/error.rs:28

Shall I add #[non_exhaustive] or increase the version?

nyurik · 2024-10-25T04:33:14Z

Might as well bump the version

nyurik · 2024-10-25T18:22:33Z

Why do you want to make it non-exhaustive? I am ok to break compat - not a biggie tbh

nyurik · 2024-10-25T18:28:49Z

but come to think of it, errors are a pretty good candidate for non-exhaustive, so yeah, lets keep it going forward... but enabling it would still be a breaking change, right?

nyurik · 2024-10-25T18:29:46Z

one more thing - i think write support is a less used feature, and since it requires extra dependencies, lets make this into an optional feature

pka · 2024-10-25T19:52:18Z

one more thing - i think write support is a less used feature, and since it requires extra dependencies, lets make this into an optional feature

flate2 is already an indirect dependency and countio is quite small. So I thougt it's not worth the hassle.

lseelenbinder · 2024-10-26T09:32:35Z

Ohhh! This is great. Just seeing the PR now for some reason. I'll give it a review shortly. Thank you @pka!

About 3 times faster and very similar compression rate for MVTs

README.md

nyurik · 2024-10-26T22:55:17Z

src/lib.rs

 mod tile;
+mod writer;


could you clarify why cfg(__async) was removed here? We really ought to make this PR behind a feature flag

I was glad to reduce the cfg(__) complexity a bit with 72f5762 - it reminded me of the bad old C/++ #ifndef days. IMO, a pmtile library should come with its core functionality (like calculating a tile id) and dependencies (like flate2) by default. I understand the feature flags for the different backend implementations with heavy or specialized dependencies, but not for everything. That said, if the maintainers prefer a feature flag for writing pmtile files, I can give it a try.

I agree that we shouldn't go overboard on features. My only concern is that if we have additional dependencies due to write support, it is better to make write - optional, simply because write is a relatively rare usage compare to read. My understanding is that you need compression, which might be a significant extra compilation burden - and that IMO should not be levied on users unless needed.

I agree a feature would be nice.

lseelenbinder

First of all, thanks for setting this all up! I'm glad we're getting this functionality added.

I'd like to understand how much impact having the leaf directories at the end of the archive would have on performance. Hopefully it's not too much, because this elegantly solves the need for a temporary file.

Another option is to reserve N bytes at the beginning of every archive for the header + directories, but that's suboptimal as well.

README.md

lseelenbinder · 2024-10-28T08:45:11Z

src/async_reader.rs

@@ -206,6 +209,9 @@ impl<B: AsyncBackend + Sync + Send, C: DirectoryCache + Sync + Send> AsyncPmTile
                    .read_to_end(&mut decompressed_bytes)


The decompressed_bytes declaration should probably move inside of this now, if we're supporting compression modes where it's not needed.

lseelenbinder · 2024-10-28T08:47:37Z

src/lib.rs

 mod tile;
+mod writer;


I agree a feature would be nice.

src/writer.rs

lseelenbinder · 2024-10-28T08:55:36Z

src/writer.rs

+
+    /// Set the compression for metadata and directories.
+    #[must_use]
+    pub fn internal_compression(mut self, compression: Compression) -> Self {


It looks like this (and a few other of the functions) use a Builder pattern without using a Builder struct. Can we split the building from the Writer struct? Then the writer struct will be quite simple (really only needs an add_tile and finalize method).

PmTilesWriter is a builder struct, generating a PmTilesStreamWriter struct with create. Builder is not in the name, because the resulting code looks better without it, IMO.

Oh! Hmm…I see. I think the nomenclature threw me off: I wouldn't expect a PmTilesWriter to produce a PmTilesStreamWriter—one of which is a builder for the other.

Any ideas for a better name? I think the namespacing is probably contributing to it. We probably should have pmtiles::Writer and pmtiles::Reader instead of PmTilesWriter and …PmTilesReader.

If we switched to pmtiles::writer::StreamWriter then we could have pmtiles::writer::Builder without confusion.

I preferred the somewhat confusing names for prettier resulting user code:

let mut writer = Builder::new(TileType::Mvt).create(file)?
vs.
let mut writer = PmTilesWriter::new(TileType::Mvt).create(file)?

I think we can accomplish the same thing with name spacing: pmtiles::writer::StreamBuilder and pmtiles::writer::StreamWriter. It's a pretty common pattern in Rust, and I think keeps things concise and manageable.

Any thoughts on this @nyurik?

It took me a few seconds to understand "name spacing" meant "namespacing" :D Naming is hard...

I am not a big fan of mega-generic names like Builder by itself, nor adding namespace as part of the required pattern... I do like PmTilesWriter::new(...).create(...) as it is cleaner to the user. We could name the resulting struct as PmTilesWriterStream or PmTilesStream or whatever.

Okay, this is a pretty fundamental decision—so I'd say this blocks the MR until we decide.

I did some research, and here's my suggestion.

The "thing" we want to build is called PmTilesWriter (I think "stream" is an implementation detail—99% of use-cases prefer streaming anyways.).

Then we can have a Builder in the same module, but hide the complexity behind a PmTilesWriter::builder() call.

This results in the following pseudocode:

let writer = PmTilesWriter::builder(tile_type).create("file.pmtiles");

The user never needs to know about the underlying builder structure, and it keeps the naming manageable. I'm happy to do the refactor, but just want to make sure there's no strong objections.

src/writer.rs

pka · 2024-10-29T14:31:31Z

I'd like to understand how much impact having the leaf directories at the end of the archive would have on performance. Hopefully it's not too much, because this elegantly solves the need for a temporary file.

Another option is to reserve N bytes at the beginning of every archive for the header + directories, but that's suboptimal as well.

I would not expect a performance impact, even via http. An optimization for initial loading would be adding the entries for z0-z2 to the root directory, which is a TODO copied from the pmtiles-go implementation. Shouldn't be difficult to add to the current implementation.

lseelenbinder · 2024-10-30T07:47:39Z

wrt 1 - you mean writing directly to s3? Does s3 allow random access of the already open file (we would need to go back to the directory to update file positions i would think)

I think this would be theoretically possible if we use multi-part upload, and refrain from uploading the first part until the end, but I'd have to check the docs to confirm this is possible.

lseelenbinder · 2024-10-30T07:56:55Z

When I have the chance, I was planning to grab a copy of this PR and do some additional cleanup that makes sense now. I'll make it dependent against your code, @pka, so we don't have merge issues.

I'm struggling with how much to ask for in this PR vs what we should clean up after. Let's not immediately release after merging to give us some time to clean up the APIs in a more general way.

lseelenbinder · 2024-11-07T08:17:15Z

Okay! We're close. Just have to resolve the naming detail, and I'll be happy to do a cleanup pass after merging.

Add basic write support

5078e8e

pka added 4 commits October 21, 2024 17:06

Compare tile bytewise instead of hashing

bc0a30a

Ignore empty tiles

6c65266

Remove __writer feature

72f5762

Use shared compression function

945ef23

pka force-pushed the writer branch from 98f985a to 945ef23 Compare October 23, 2024 09:50

pka added 3 commits October 23, 2024 19:25

Build and write leaf directories

daef0d6

Add writer builder

1c0adf1

Make writer generig for Write+Seek

21b32d0

pka added 2 commits October 23, 2024 23:44

Track n_tile_contents

fa3a8fe

Add usage example

1c5bd09

pka force-pushed the writer branch from de00949 to 1c5bd09 Compare October 23, 2024 21:44

Use builder API in test

a438510

nyurik reviewed Oct 24, 2024

View reviewed changes

pka added 2 commits October 24, 2024 13:05

Minor cleanups

2c520c7

Check tile_id order

4586c4c

pka added 2 commits October 24, 2024 19:56

Move usage example to README an include it in module docs

bfcd9f8

Rename finish to finalize

8a152e2

pka force-pushed the writer branch from 21e66ba to 8a152e2 Compare October 24, 2024 17:57

pka added 2 commits October 24, 2024 19:58

Directory::from_entries now takes ownership

2ff3c66

Export tile_id function

810d087

Make PmtError non_exhaustive and bump version

d7660df

nyurik requested a review from lseelenbinder October 25, 2024 18:30

Change gzip compression level to 6

e05ca17

About 3 times faster and very similar compression rate for MVTs

nyurik reviewed Oct 26, 2024

View reviewed changes

lseelenbinder requested changes Oct 28, 2024

View reviewed changes

Add links to reference implementations

e6742bb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic write support #45

Add basic write support #45

pka commented Oct 19, 2024 •

edited

Loading

nyurik commented Oct 21, 2024

pka commented Oct 21, 2024

nyurik commented Oct 21, 2024

pka commented Oct 23, 2024

nyurik left a comment

pka commented Oct 24, 2024

nyurik commented Oct 25, 2024

nyurik commented Oct 25, 2024

nyurik commented Oct 25, 2024

nyurik commented Oct 25, 2024

pka commented Oct 25, 2024

lseelenbinder commented Oct 26, 2024

nyurik Oct 26, 2024

pka Oct 27, 2024

nyurik Oct 28, 2024

lseelenbinder Oct 28, 2024

lseelenbinder left a comment

lseelenbinder Oct 28, 2024

lseelenbinder Oct 28, 2024

lseelenbinder Oct 28, 2024

pka Oct 29, 2024

lseelenbinder Oct 29, 2024

lseelenbinder Oct 29, 2024

pka Oct 29, 2024

lseelenbinder Oct 30, 2024

nyurik Oct 30, 2024

lseelenbinder Nov 7, 2024 •

edited

Loading

pka commented Oct 29, 2024

lseelenbinder commented Oct 30, 2024

lseelenbinder commented Oct 30, 2024

lseelenbinder commented Nov 7, 2024

		@@ -206,6 +209,9 @@ impl<B: AsyncBackend + Sync + Send, C: DirectoryCache + Sync + Send> AsyncPmTile
		.read_to_end(&mut decompressed_bytes)

Add basic write support #45

Are you sure you want to change the base?

Add basic write support #45

Conversation

pka commented Oct 19, 2024 • edited Loading

nyurik commented Oct 21, 2024

pka commented Oct 21, 2024

nyurik commented Oct 21, 2024

pka commented Oct 23, 2024

nyurik left a comment

Choose a reason for hiding this comment

pka commented Oct 24, 2024

nyurik commented Oct 25, 2024

nyurik commented Oct 25, 2024

nyurik commented Oct 25, 2024

nyurik commented Oct 25, 2024

pka commented Oct 25, 2024

lseelenbinder commented Oct 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lseelenbinder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lseelenbinder Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

pka commented Oct 29, 2024

lseelenbinder commented Oct 30, 2024

lseelenbinder commented Oct 30, 2024

lseelenbinder commented Nov 7, 2024

pka commented Oct 19, 2024 •

edited

Loading

lseelenbinder Nov 7, 2024 •

edited

Loading