iOS compilation failure due to zstd

[Parth]

I'm working on lockbook, a cross platform, secure note taking app. We recently found our build broken while updating our rust version and traced this behavior to zstd. This failure is described here: gyscos/zstd-rs#337

I do see, however that zstd is an optional feature here

tantivy/Cargo.toml

Line 31 in 5379c99

zstd = { version = "0.13", optional = true, default-features = false }

but is used normally here

tantivy/sstable/Cargo.toml

Line 19 in 5379c99

zstd = { version = "0.13", features = ["experimental"] }

Is it the intention for zstd to be truly optional? If so, may I send a PR to make sstable reflect this? This would allow us to remove zstd from our dependency tree and proceed with our upgrade.

Or is it the case that this feature is truly not optional and does need to be solved with zstd itself?

Relevant cargo tree output copied from the zstd issue:

zstd v0.13.3
└── tantivy-sstable v0.3.0
    └── tantivy-columnar v0.3.0
        └── tantivy v0.22.0
            └── lb-rs v0.9.22 (/Users/parth/Documents/lockbook/lockbook/libs/lb/lb-rs)
                ├── lb-c v0.9.22 (/Users/parth/Documents/lockbook/lockbook/libs/lb/lb-c)
                │   └── workspace-ffi v0.9.22 (/Users/parth/Documents/lockbook/lockbook/libs/content/workspace-ffi)
                └── workspace v0.9.22 (/Users/parth/Documents/lockbook/lockbook/libs/content/workspace)
                    └── workspace-ffi v0.9.22 (/Users/parth/Documents/lockbook/lockbook/libs/content/workspace-ffi)

[PSeitz]

I think zstd could be optional in sstable and should be straightforward to add in a PR. cc @trinity-1686a

[trinity-1686a]

sstable's codec is written in such a way that compression is already optional (enabled/disabled based on if it's helping or not with size). It should be rather easy to disable zstd entirely from sstables. Obviously sstables written with zstd enabled would not always be readable by a software built without zstd, but a software with zstd support would be able to read both and wouldn't notice a thing.

https://github.com/quickwit-oss/tantivy/blob/main/sstable/src/delta.rs#L60 is the 1st place you'll see zstd, if you make if block_len > 2048 { into an unconditional false, no zstd code can be reached at encoding time.
https://github.com/quickwit-oss/tantivy/blob/main/sstable/src/block_reader.rs#L88 is the 2nd place you'll see some zstd, making that entire branch return an error (while style verifying if what you get is a compressed block) seems the way to go

[Parth]

Thanks for the insight @trinity-1686a, starting to whip up a PR but want to clarify:

Obviously sstables written with zstd enabled would not always be readable by a software built without zstd

Does this interfere with the guarantee expressed in the changelog here? As part of my PR should I remove the compatibility guarantee and add more details about the change?

[PSeitz]

That should be okay, if you keep the the serialized format compatible.

[trinity-1686a]

i guess this is an odd position. Currently the support for zstd is disabled by default in tantivy, but mandatory in sstable on which tantivy depends. Piggybacking on tantivy feature flag would break things for people not enabling zstd (their old index wouldn't be entirely readable without enabling that feature), but having multiple feature flags is going to confuse people, and rightly so.
So we probably need to either put a disclaimer in the patchnote, or enable zstd-compression by default in tantivy

[Parth]

Thanks for the guidance @PSeitz / @trinity-1686a. Looking forward to your feedback ^