feat(index): replace suffix array dependency - #8015
Conversation
AI Disclosure: This code was written in part by an AI agent.:
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@beinan Could I get your opinion on this as the author of the PR who brought in the dependency? For our part we are not using the FM-index so I'm not in a great position to say if the performance tradeoff is worth it. The other alternative is I could put up a PR that makes this dependency optional. |
There was a problem hiding this comment.
Pull request overview
This PR removes the libsais-rs C-FFI dependency from the Rust FM-index implementation by replacing suffix-array construction with a safe, pure-Rust SA-IS implementation (with accompanying unit tests), while keeping the FM-index query path unchanged.
Changes:
- Add a new safe SA-IS-based suffix array builder (
suffix_array.rs) and unit tests. - Switch FM-index build to use the new suffix array implementation and remove the prior
libsais-rscall sites. - Remove
libsais-rsfromrust/lance-indexdependencies and all affected lockfiles.
Reviewed changes
Copilot reviewed 3 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| rust/lance-index/src/scalar/fmindex/suffix_array.rs | Introduces pure-Rust SA-IS suffix-array construction plus tests. |
| rust/lance-index/src/scalar/fmindex.rs | Wires in the new suffix-array module and deletes the old libsais-rs implementation. |
| rust/lance-index/Cargo.toml | Drops the libsais-rs dependency. |
| Cargo.lock | Removes libsais-rs from workspace lockfile. |
| python/Cargo.lock | Removes libsais-rs from Python lockfile. |
| java/lance-jni/Cargo.lock | Removes libsais-rs from Java JNI lockfile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if is_same && (left == len || symbols[left] != symbols[right]) { | ||
| is_same = false; | ||
| } |
A security audit raised a concern about the dependency
libsais-rsthat was introduced in #7026The audit specifically said:
I'd push back on the "code nobody is reviewing comment" but in general it seems worth some mitigation.
The usage of this dependency is limited in scope, so I had an agent implement the associated code based on the CC0 AtCoder Library algorithm. It is 100% rust with no
unsafeoperations.Performance trade-off
Replacing the highly-optimized
libsaisC implementation with the pure-Rust SA-IS is a known performance regression on suffix-array construction (index build only — query paths are unaffected).Benchmark: suffix arrays built over a realistic FM-index corpus (vocabulary words,
0xFFdocument separators,0x00terminator — matching the actual build path). Both implementations produced byte-identical suffix arrays. Criterion, 10 samples per case.The gap widens with input size:
libsaissustains ~86–102 MiB/s (cache-blocked + SIMD), while the safe SA-IS decays from ~37 to ~18 MiB/s due to scatter-heavy induced sorting. With the default 16 MB partition cap, worst-case per-partition SA build is ~5× slower. Partitions build in parallel viaspawn_cpu, so multi-core wall-clock impact is softened.This is an accepted trade: ~3–5× slower index build in exchange for removing an unaudited unsafe C FFI dependency. If build time becomes a bottleneck, options include parallelizing the induce phase or reducing partition size.