AVRO-4032: [Rust] Make it possible to set custom compression level for all codec that support it #3095

MarcoLugo · 2024-08-13T17:14:42Z

What is the purpose of the change

This PR adds support for picking a custom compression level for the Zstandard codec. This change does not remove the Zstandard codec variant in order to keep backwards compatibility and a slightly simpler interface for the default case.

Verifying this change

Added new tests:

Compression and decompression, analogous to other codec tests.
A test that makes sure that the codec is converted to a string as zstandard (i.e. same as Zstandard codec) as this is still the same codec.

Documentation

Does this pull request introduce a new feature?
Yes.
If yes, how is the feature documented?
Simple description available in cargo-generated documentation. Validated it using cargo doc --open --no-deps --all-features.

xxchan · 2024-08-14T03:16:42Z

lang/rust/avro/src/codec.rs

+    /// The `Zstandard` codec uses Facebook's [Zstandard](https://facebook.github.io/zstd/) with the
+    /// default compression level.
    Zstandard,
+    #[cfg(feature = "zstandard")]
+    /// This codec is the same as `Zstandard` but allows specifying the compression level.
+    #[strum(disabled)]
+    ZstandardWithLevel(ZstandardLevel),


Since we are in 0.x state, I don't think we need to avoid breaking changes. Directly changing Zstandard looks cleaner.

I agree!
I'd suggest to go even further:

introduce ZstandardOptions (or ZstandardConfig) struct that provides all possible settings - the level and a dictionary. With a Default impl that uses 0 and &[]

the same for all other codecs with their respective settings

WDYT ?

With 71021f2 I've pushed codec settings for Bzip2. Please pull before working on the settings for Zstd!

Excellent idea! I'll go ahead with it for zstd.

I also pushed similar settings for Xz!

Snappy does not provide APIs for setting compression level (or anything other), so there is no need of changes!

Deflate provides its own EncodeOptions but we cannot use it in our Codec enum because it does not implement several traits (Clone, Copy, Eq and PartialEq) ... Constructing EncodeOptions from its fields is not possible because it does not provide such APIs.

So, only Zstd is left! I'll leave it to you @MarcoLugo !

I added a similar struct for zstd but for now I ended up leaving out the dictionary as it would be required for decoding as well and so it would need to go into the header metadata and I'm not sure how to do that at the moment. I'll need to read the avro spec in more detail, maybe this could be a future PR in order to avoid blocking this one?
I also made the new settings structs public so they're visible in the docs and can be used.

The dictionary is a Zstd thingy. It just needs to be the same for both encoding and decoding.
Just pushed 8f4a2f3 for it

I am going to revert the support for dictionary. Dealing with the slice causes a lot of lifetime headaches.
I am about to commit an improvement to the (de)ser of the codec in the Avro binary metadata. We need to store the settings along with the codec name!

I think I hit a problem!

By Avro spec when writing the binary data (.avro) the codec is stored in the header's metadata as a string, e.g. "zstandard". But there is nothing about codec's metadata!
So, if you do something like :

let schema = Schema::parse_str( r#" { "type": "record", "name": "Test", "fields": [ {"name": "f1", "type": "int"}, {"name": "f2", "type": "string"} ] }"#, )?; let mut writer = Writer::with_codec(&schema, Vec::new(), codec); let mut record = Record::new(writer.schema()).unwrap(); record.put("f1", 27_i32); record.put("f2", "foo"); writer.append(record)?; let input = writer.into_inner()?; let mut reader = Reader::new(&input[..])?; ...

Then the Reader will not know anything about the codec's settings used by the writer! The Reader will use the codec with its default settings!
We can store the settings in the metadata's user_metadata, see reader.rs > Block struct. But this will take some more time!

This is what I was fearing earlier regarding the dictionary though I am admittedly less familiar with the spec. I'd be in favor of having the zstd dictionary as a separate ticket/PR. This ticket initially it was about being able to set the level which I think is already a good improvement and we got the ability to set the level for more codecs than zstd too :)

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

martin-g · 2024-08-15T09:51:18Z

I just noticed that Codec::iter() (generated by strum) returns only Null and Deflate and I am not sure why the others are not returned...
It behaves the same way in main branch, so it is not a regression.

Update: my fault! I needed to enable the features ...

cargo run --example generate_interop_data --all-features
   Compiling apache-avro v0.18.0 (/home/martin/git/apache/avro/lang/rust/avro)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.75s
     Running `target/debug/examples/generate_interop_data`
Wrote ../../build/interop/data/rust.avro
Wrote ../../build/interop/data/rust_deflate.avro
Wrote ../../build/interop/data/rust_snappy.avro
Wrote ../../build/interop/data/rust_zstandard.avro
Wrote ../../build/interop/data/rust_bzip2.avro
Wrote ../../build/interop/data/rust_xz.avro

All looks good!

…adata Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

martin-g · 2024-08-15T14:29:49Z

I think I am ready!
@MarcoLugo Please test with your application and let me know how it goes!

martin-g · 2024-08-15T15:01:24Z

lang/rust/avro/src/codec.rs

+    use bzip2::Compression;
+
+    #[derive(Clone, Copy, Eq, PartialEq, Debug)]
+    pub struct Bzip2Settings {


I wonder: do we need **Settings for each codec ?
All 3 classes are the same now.
If we need to add custom codec specific settings later it should not be a big deal - the other codecs will just ignore any setting they don't support. I do not expect many new settings.

This won't work because the codecs use different compression level by default and if more than one feature is enabled then it is not possible to decide what to use as a default.

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

This way there is no need to enable other codecs when running unrelated tests. Otherwise the build fails with: ``` error: function `avro_4032_codec_settings` is never used --> avro/tests/codecs.rs:50:4 | 50 | fn avro_4032_codec_settings(codec: Codec) -> TestResult { | ^^^^^^^^^^^^^^^^^^^^^^^^ ``` Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

MarcoLugo · 2024-08-15T19:41:17Z

I think I am ready! @MarcoLugo Please test with your application and let me know how it goes!

It looks good to me, thank you @martin-g ! It worked well when I used it in my application.

martin-g · 2024-08-15T19:49:28Z

Thank you too, @MarcoLugo !

…r all codec that support it (#3095) * AVRO-4032: add zstd variant that allows for level config * add docs * manual impl for IntoStaticStr to handle 2 variants with same value * AVRO-4032: [Rust] Add codec settings for Bzip2 Signed-off-by: Martin Tzvetanov Grigorov <[email protected]> * AVRO-4032: [Rust] Add settings for Xz codec Signed-off-by: Martin Tzvetanov Grigorov <[email protected]> * add zstd settings * make our new settings structs visible * AVRO-4032: [Rust] Add support for Zstandard dictionary Signed-off-by: Martin Tzvetanov Grigorov <[email protected]> * AVRO-4032: [Rust] Store the codec compression_level in the header metadata Signed-off-by: Martin Tzvetanov Grigorov <[email protected]> * AVRO-4032: [Rust] Minor cleanups Signed-off-by: Martin Tzvetanov Grigorov <[email protected]> * AVRO-4032: Fix the build Signed-off-by: Martin Tzvetanov Grigorov <[email protected]> * AVRO-4032: Add tests for Codec::Null and ::Deflate This way there is no need to enable other codecs when running unrelated tests. Otherwise the build fails with: ``` error: function `avro_4032_codec_settings` is never used --> avro/tests/codecs.rs:50:4 | 50 | fn avro_4032_codec_settings(codec: Codec) -> TestResult { | ^^^^^^^^^^^^^^^^^^^^^^^^ ``` Signed-off-by: Martin Tzvetanov Grigorov <[email protected]> --------- Signed-off-by: Martin Tzvetanov Grigorov <[email protected]> Co-authored-by: Martin Tzvetanov Grigorov <[email protected]> (cherry picked from commit 9379fdb)

MarcoLugo added 2 commits August 13, 2024 12:18

AVRO-4032: add zstd variant that allows for level config

9e69eed

add docs

5b12712

github-actions bot added the Rust label Aug 13, 2024

manual impl for IntoStaticStr to handle 2 variants with same value

0d8c5dd

xxchan reviewed Aug 14, 2024

View reviewed changes

martin-g and others added 5 commits August 14, 2024 14:08

AVRO-4032: [Rust] Add codec settings for Bzip2

71021f2

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

AVRO-4032: [Rust] Add settings for Xz codec

b53d11c

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

add zstd settings

927df5c

make our new settings structs visible

d59f30e

AVRO-4032: [Rust] Add support for Zstandard dictionary

8f4a2f3

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

AVRO-4032: [Rust] Store the codec compression_level in the header met…

bd346f7

…adata Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

martin-g reviewed Aug 15, 2024

View reviewed changes

martin-g added 3 commits August 15, 2024 18:03

AVRO-4032: [Rust] Minor cleanups

aefb98f

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

AVRO-4032: Fix the build

d5d79d2

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

martin-g changed the title ~~AVRO-4032: [Rust] zstd level~~ AVRO-4032: [Rust] Make it possible to set custom compression level for all codec that support it Aug 15, 2024

martin-g merged commit 9379fdb into apache:main Aug 15, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AVRO-4032: [Rust] Make it possible to set custom compression level for all codec that support it #3095

AVRO-4032: [Rust] Make it possible to set custom compression level for all codec that support it #3095

MarcoLugo commented Aug 13, 2024 •

edited by martin-g

Loading

xxchan Aug 14, 2024

martin-g Aug 14, 2024

martin-g Aug 14, 2024

MarcoLugo Aug 14, 2024

martin-g Aug 14, 2024

MarcoLugo Aug 14, 2024

martin-g Aug 15, 2024

martin-g Aug 15, 2024

martin-g Aug 15, 2024 •

edited

Loading

MarcoLugo Aug 15, 2024

martin-g commented Aug 15, 2024 •

edited

Loading

martin-g commented Aug 15, 2024

martin-g Aug 15, 2024

martin-g Aug 15, 2024

MarcoLugo commented Aug 15, 2024

martin-g commented Aug 15, 2024

AVRO-4032: [Rust] Make it possible to set custom compression level for all codec that support it #3095

AVRO-4032: [Rust] Make it possible to set custom compression level for all codec that support it #3095

Conversation

MarcoLugo commented Aug 13, 2024 • edited by martin-g Loading

What is the purpose of the change

Verifying this change

Documentation

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

martin-g Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-g commented Aug 15, 2024 • edited Loading

martin-g commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoLugo commented Aug 15, 2024

martin-g commented Aug 15, 2024

MarcoLugo commented Aug 13, 2024 •

edited by martin-g

Loading

martin-g Aug 15, 2024 •

edited

Loading

martin-g commented Aug 15, 2024 •

edited

Loading