Include only built-in compression algorithms as enum variants #2121
Conversation
|
Just an idea I would to propose for discussion: I think having one really fast compression algorithm (LZ4) and one really flexible one (Zstd) might actually be enough and neither Brotli nor Snappy add anything substantially different. |
adamreichold
force-pushed
the
compression-feature-gate
branch
2 times, most recently
from
21c7194 to
8b8449d
Compare
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## main #2121 +/- ##
==========================================
+ Coverage 94.38% 94.39% +0.01%
==========================================
Files 321 321
Lines 60809 60821 +12
==========================================
+ Hits 57393 57413 +20
+ Misses 3416 3408 -8
|
| ], | ||
| )); | ||
| } | ||
| return Err(serde::de::Error::unknown_variant( |
There was a problem hiding this comment.
that's misleading though.
If the store is compressed with zstd but we don't have the feature flag, we will get an error message saying something like:
It is "zstd" but we expected something in "none", "lz4", "zstd"... etc.
There was a problem hiding this comment.
I will just adjust the deserialization error message to mention only those which were built-in and also point to those which could be enabled via additional feature flags.
There was a problem hiding this comment.
I will just adjust the deserialization error message to mention only those which were built-in and also point to those which could be enabled via additional feature flags.
Ah, the error message itself is generated by Serde. I did update the list of expected variants to match what is built-in though.
Should we keep it that way, or should I switch to custom error message to include reference to those which could be enable but are currently not?
There was a problem hiding this comment.
I suggest using a serde custom error, to tell that the compression format xxx is not available and suggestion that tantivy is maybe missing a compilation flag.
Rationale:
The unknown_variant seem appropriate here, but we really want a correct error message surfaced to the user.
|
I am ok with hiding the enums. Can we however have an explicit error message about what feature is missing when opening an index with a compression format not compiled in the binary? |
I agree. |
adamreichold
force-pushed
the
compression-feature-gate
branch
from
8b8449d to
72d37fd
Compare
This enables compile-time errors when a compression algorithm is requested which is not actually enabled for the current Cargo project. The cost is that indexes using other compression algorithms cannot even be loaded (even though they are not fully accessible in any case). As a drive-by, this also fixes `--no-default-features` on `cfg(unix)`.
…own compression variants.
adamreichold
force-pushed
the
compression-feature-gate
branch
from
72d37fd to
f7288b0
Compare
I will propose this as a separate PR then so please see the PR title and can comment further on this. |
|
@adamreichold thank you! |
This enables compile-time errors when a compression algorithm is requested which is not actually enabled for the current Cargo project. The cost is that indexes using other compression algorithms cannot even be loaded (even though they are not fully accessible in any case).
As a drive-by, this also fixes
--no-default-featuresoncfg(unix).Closes #2119