feat!: Support compression codecs for Avro Files (inlcuding manifest and manifest lists) #1851

emkornfield · 2025-11-13T00:53:58Z

Which issue does this PR close?

Partially fixes [Bug] iceberg-rust does not respect compression settings for metadata & avro #1731

What changes are included in this PR?

Previously these properties where not honored on tabel properties.

Adds table properties for these values.
Plumbs them through for writers.

Are these changes tested?

Added unit tests

BREAKING CHANGE: Adds codec parameter to some public functions. By default start compressing manifests and manifest lists.

Previously these properties where not honored on tabel properties. - Adds table properties for these values. - Plumbs them through for writers.

kevinjqliu · 2025-11-13T03:11:13Z

looks like a clippy issue in CI

error: this function has too many arguments (8/7)
   --> crates/iceberg/src/spec/manifest_list.rs:243:5
    |
243 | /     fn new(
244 | |         format_version: FormatVersion,
245 | |         output_file: OutputFile,
246 | |         metadata: HashMap<String, String>,
...   |
251 | |         compression_level: u8,
252 | |     ) -> Self {
    | |_____________^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
    = note: `-D clippy::too-many-arguments` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::too_many_arguments)]`

emkornfield · 2025-11-13T06:45:26Z

looks like a clippy issue in CI

Yeah working on a fix.

emkornfield · 2025-11-13T18:09:36Z

@liurenjie1024 @kevinjqliu would you have time to review?

kevinjqliu

Generally LGTM, I left a few comments around using the same string as the java implementation

crates/iceberg/src/spec/table_properties.rs

crates/iceberg/src/spec/avro_util.rs

crates/iceberg/src/spec/manifest_list.rs

crates/iceberg/src/transaction/snapshot.rs

crates/iceberg/src/spec/table_metadata.rs

emkornfield · 2025-11-16T00:44:54Z

crates/iceberg/src/spec/avro_util.rs

+///
+/// # Compression Levels
+///
+/// The compression level mapping is based on miniz_oxide's CompressionLevel enum:


clarify this is for gzip.

nit: maybe merge this comment with the one above (L61-L66), the compression levels are explained twice

crates/iceberg/src/spec/manifest/writer.rs

kevinjqliu

this looks great, thanks!
adding the "breaking" label since we're modifying a few public functions

crates/iceberg/src/spec/table_properties.rs

crates/iceberg/src/spec/table_metadata.rs

crates/iceberg/src/transaction/snapshot.rs

crates/iceberg/src/spec/manifest/writer.rs

crates/iceberg/src/spec/avro_util.rs

emkornfield · 2025-11-17T22:02:22Z

@kevinjqliu thanks for the reviews, I also added more e2e tests for manifest and manifest list writers to confirm compression.

emkornfield · 2025-11-18T00:40:05Z

adding the "breaking" label since we're modifying a few public functions

I'm not sure that I can access labels as a contributor, I put a breaking change note in the PR description.

liurenjie1024

Thanks @emkornfield for this pr. In general, I think this is a useful feature implementation. But we should not mix two things in one pr, I would suggest to split them into two prs: one for table metadata, and one for manifests.

liurenjie1024 · 2025-11-18T10:04:11Z

crates/iceberg/src/spec/table_properties.rs

+fn parse_optional_property<T: std::str::FromStr>(
+    properties: &HashMap<String, String>,
+    key: &str,
+) -> Result<Option<T>, anyhow::Error>


We should use this crate's error.

Do we really need this method? I think passing a None to default value in parse_property would be enough?

I'm new to rust but would that work? Doesn't parse_property, always return an extension of String instead of an Option? It is useful to know if the value was actually configured (and thus the option return)?

I think in iceberg all table properties are optional, if they have default values, then we should return a default value, otherwise we should return an error.

I'll see if I can make this work with empty string.

I think in iceberg all table properties are optional, if they have default values, then we should return a default value, otherwise we should return an error.

The problem here is that the default value is dependent on the value for compression, it seems baking this into the parsing layer is coupling two concerns (business logic for parsing) and the actual values. To eliminate this method I've used a sentinel value for the old method. Let me know if that is seems OK.

crates/iceberg/src/spec/table_metadata.rs

emkornfield · 2025-11-19T21:57:27Z

crates/iceberg/src/spec/table_properties.rs


 // Helper function to parse a property from a HashMap
 // If the property is not found, use the default value
-fn parse_property<T: std::str::FromStr>(


this is cleanup because it sounds like the crate's errors are preferred I can revert this.

emkornfield · 2025-11-19T22:04:07Z

@liurenjie1024 split of this PR to just be about Avro. Also based on feedback, I cleaned up some additional imports/Errors that weren't using the Crate's error before.

kevinjqliu

This LGTM, a couple of nit comments

Thanks for adding the additional tests for manifest and manifest list writers
I also added the "breaking" label, forgot to after i commented 😄

kevinjqliu · 2025-11-21T04:43:15Z

crates/iceberg/src/spec/table_properties.rs

+            avro_compression_level: {
+                let level = parse_property(
+                    props,
+                    TableProperties::PROPERTY_AVRO_COMPRESSION_LEVEL,
+                    255u8,
+                )?;
+                if level == 255 { None } else { Some(level) }
+            },


is this not valid here?

avro_compression_level: parse_property( props, TableProperties::PROPERTY_AVRO_COMPRESSION_LEVEL, TableProperties::PROPERTY_AVRO_COMPRESSION_LEVEL_DEFAULT, )?,

avro_compression_level and PROPERTY_AVRO_COMPRESSION_LEVEL_DEFAULT are both Option<u8>

I might be misunderstanding the comment. But I should have cleaned up pub const PROPERTY_AVRO_COMPRESSION_LEVEL_DEFAULT: Option<u8> = None;, I don't think it is used anymore.

In a prior revision I added a method (parse_optional_property) that could return None value directly but @liurenjie1024 questioned whether this was useful.

kevinjqliu · 2025-11-21T04:48:21Z

crates/iceberg/src/spec/avro_util.rs

+    fn default() -> Self {
+        Self {
+            codec: TableProperties::PROPERTY_AVRO_COMPRESSION_CODEC_DEFAULT.to_string(),
+            level: None,


Suggested change

level: None,

level: TableProperties::PROPERTY_AVRO_COMPRESSION_LEVEL_DEFAULT,

removing this.

kevinjqliu · 2025-11-21T04:50:42Z

crates/iceberg/src/spec/avro_util.rs

+///
+/// # Compression Levels
+///
+/// The compression level mapping is based on miniz_oxide's CompressionLevel enum:


nit: maybe merge this comment with the one above (L61-L66), the compression levels are explained twice

kevinjqliu · 2025-11-21T04:58:24Z

crates/iceberg/src/spec/table_properties.rs

    pub write_target_file_size_bytes: usize,
+    /// Compression codec for Avro files (manifests, manifest lists)
+    pub avro_compression_codec: String,
+    /// Compression level for Avro files (None uses codec-specific defaults: gzip=9, zstd=1)


Suggested change

/// Compression level for Avro files (None uses codec-specific defaults: gzip=9, zstd=1)

/// Compression level for Avro files (None uses codec-specific defaults)

nit: these might change in the future, so i think its better to not specify the values here

there are a few other instances of this same comment

emkornfield added 2 commits November 13, 2025 00:45

feat!: Support compression codecs for JSON metadata and Avro

665b970

Previously these properties where not honored on tabel properties. - Adds table properties for these values. - Plumbs them through for writers.

fmt

091d3bc

emkornfield added 2 commits November 13, 2025 06:54

fix clippy

41a8c1c

clippy again

51e781e

kevinjqliu reviewed Nov 15, 2025

View reviewed changes

emkornfield added 2 commits November 16, 2025 00:27

wip

253bf59

address comments

8bdb52d

emkornfield commented Nov 16, 2025

View reviewed changes

crates/iceberg/src/spec/manifest/writer.rs Outdated Show resolved Hide resolved

emkornfield added 2 commits November 16, 2025 00:52

no clone needed

9d27116

test compression works

d1ee0b2

kevinjqliu reviewed Nov 17, 2025

View reviewed changes

emkornfield added 2 commits November 17, 2025 21:51

comments

393622f

update tests

46fdb8e

emkornfield requested a review from kevinjqliu November 17, 2025 22:02

liurenjie1024 requested changes Nov 18, 2025

View reviewed changes

emkornfield added 2 commits November 19, 2025 21:29

address comments

d50cb7d

remove parse optional property

5370f77

emkornfield commented Nov 19, 2025

View reviewed changes

emkornfield changed the title ~~feat!: Support compression codecs for JSON metadata and Avro~~ feat!: Support compression codecs for Avro Files (inlcuding manifest and manifest lists) Nov 19, 2025

fmt

6b3d8ed

emkornfield requested a review from liurenjie1024 November 19, 2025 22:03

emkornfield mentioned this pull request Nov 19, 2025

feat: Honor compression settings for metadata.json on write #1876

Open

kevinjqliu added the breaking label Nov 21, 2025

kevinjqliu approved these changes Nov 21, 2025

View reviewed changes

emkornfield added 2 commits November 21, 2025 18:56

wip

5ec090f

address comments

4337a34

	level: None,
	level: TableProperties::PROPERTY_AVRO_COMPRESSION_LEVEL_DEFAULT,

	/// Compression level for Avro files (None uses codec-specific defaults: gzip=9, zstd=1)
	/// Compression level for Avro files (None uses codec-specific defaults)

feat!: Support compression codecs for Avro Files (inlcuding manifest and manifest lists) #1851

Are you sure you want to change the base?

feat!: Support compression codecs for Avro Files (inlcuding manifest and manifest lists) #1851

Uh oh!

Conversation

emkornfield commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

Uh oh!

kevinjqliu commented Nov 13, 2025

Uh oh!

emkornfield commented Nov 13, 2025

Uh oh!

emkornfield commented Nov 13, 2025

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emkornfield commented Nov 17, 2025

Uh oh!

emkornfield commented Nov 18, 2025

Uh oh!

liurenjie1024 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emkornfield commented Nov 19, 2025

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

emkornfield commented Nov 13, 2025 •

edited

Loading