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

emkornfield · 2025-11-19T22:13:09Z

Which issue does this PR close?

Split off from #1851

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

What changes are included in this PR?

This change honors the compression setting for metadata.json file (write.metadata.compression-codec).

Are these changes tested?

Add unit test to verify files are gzipped when the flag is enabled.

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

emkornfield · 2025-11-19T22:13:22Z

CC @kevinjqliu @liurenjie1024

liurenjie1024 · 2025-11-20T10:17:51Z

crates/iceberg/src/spec/table_properties.rs

    key: &str,
    default: T,
-) -> Result<T, anyhow::Error>
+) -> crate::error::Result<T>


Suggested change

) -> crate::error::Result<T>

) -> Result<T>

Unnecessary fully qualified name.

liurenjie1024 · 2025-11-20T10:19:58Z

crates/iceberg/src/spec/table_properties.rs

    /// The target file size for files.
    pub write_target_file_size_bytes: usize,
+    /// Compression codec for metadata files (JSON)
+    pub metadata_compression_codec: String,


This should be sth like Option<String>, none should be None in memory.

liurenjie1024 · 2025-11-20T10:29:36Z

crates/iceberg/src/spec/table_metadata.rs

+
+                // Modify filename to add .gz before .metadata.json
+                let location = metadata_location.as_ref();
+                let new_location = if location.ends_with(".metadata.json") {


I don't think we should simply replace the suffix with this approach. Currently all metadata location generated logic are in MetadataLocation, we should add a new method in like following and deprecate old method as following:

impl MetadataLocation { #[deprecate("Use new_with_table instead.")] pub fn new_with_table_location(table_location: impl ToString) -> Self { } pub fn new_with_table(table: &Table) -> Self { .... }

The table option seemed a little awkward with the control flow on how it is used istead I added a method that takes table properties. Also added a similar method for incrementing the number.

I modified this logic a bit and kept. My concern here is if we only deprecate the new_with_table_location then users can write out metadata.json files in a format that would be unreadable by other readers.

emkornfield · 2025-11-21T01:49:39Z

crates/catalog/s3tables/src/catalog.rs

                    .map_err(from_aws_sdk_error)?;
-                let warehouse_location = get_resp.warehouse_location().to_string();
-                MetadataLocation::new_with_table_location(warehouse_location).to_string()
+                get_resp.warehouse_location().to_string()


this is a semantic change because up the update to creation.location below, need to investigate further what the correct thing to do here is.

good point, from the comment above (L451-452) it seems like the "s3tables catalog" generates the metadata file location and does not allow any modification

so maybe we should just use

let metadata_location = MetadataLocation::new_with_properties(metadata_location, None) .to_string();

so as to not do any path modification before writing

I think the original code was wrong. As best I can test warehouseLocation is actually the table location.

I've updated it to assign to table_location and then assign it below (as far as I can tell, the .location field is meant to be table location and not the metadata.json file).

kevinjqliu

Generally LGTM, a few nit comments.
would be good to get a second pair of 👀

crates/iceberg/src/spec/table_properties.rs

kevinjqliu · 2025-11-21T03:45:28Z

crates/iceberg/src/catalog/metadata_location.rs

+
+    /// Creates a completely new metadata location starting at version 0,
+    /// with compression settings from the table's properties.
+    /// Only used for creating a new table. For updates, see `with_next_version`.


Suggested change

/// Only used for creating a new table. For updates, see `with_next_version`.

/// Only used for creating a new table. For updates, see `with_next_version_and_properties`.

since with_next_version is also deprecated

I'll wait till I understand @liurenjie1024 concerns about with_next_version_and_properties to update this.

kevinjqliu · 2025-11-21T03:56:40Z

crates/catalog/s3tables/src/catalog.rs

                    .map_err(from_aws_sdk_error)?;
-                let warehouse_location = get_resp.warehouse_location().to_string();
-                MetadataLocation::new_with_table_location(warehouse_location).to_string()
+                get_resp.warehouse_location().to_string()


good point, from the comment above (L451-452) it seems like the "s3tables catalog" generates the metadata file location and does not allow any modification

so maybe we should just use

let metadata_location = MetadataLocation::new_with_properties(metadata_location, None) .to_string();

so as to not do any path modification before writing

liurenjie1024

Thanks @emkornfield for this pr, looks much better now, still need some refinements.

liurenjie1024 · 2025-11-21T10:07:45Z

crates/iceberg/src/catalog/metadata_location.rs

+    /// Creates a completely new metadata location starting at version 0,
+    /// with compression settings from the table's properties.
+    /// Only used for creating a new table. For updates, see `with_next_version`.
+    pub fn new_with_properties(


Suggested change

pub fn new_with_properties(

pub fn new_table(table: &Table)

We may do refactoring to table properties, and we may need other

I think the issue I have with taking a table here is that metadata location is an input to building a table, how about taking TableMetadataRef as input?

Or are you suggesting changing the return type of Table, so that this method updates it?

Hi, @emkornfield changing the input param to take a &TableMetadata sounds good to me.

liurenjie1024 · 2025-11-21T10:09:04Z

crates/iceberg/src/catalog/metadata_location.rs

+        since = "0.8.0",
+        note = "Use with_next_version_and_properties instead to properly handle compression settings changes"
+    )]
    pub fn with_next_version(&self) -> Self {


We don't need change this method, it's not a constructor.

Same question as the comment below, lets consolidate conversation there.

liurenjie1024 · 2025-11-21T10:09:16Z

crates/iceberg/src/catalog/metadata_location.rs

+    /// Creates a new metadata location for an updated metadata file.
+    /// Takes table properties to determine compression settings, which may have changed
+    /// from the previous version.
+    pub fn with_next_version_and_properties(&self, properties: &HashMap<String, String>) -> Self {


We don't need this.

Can you clarify what you mean by we don't need it? I updated crates/iceberg/src/catalog/mod.rs to use it, if new properties change compression settings, I'm not sure how this would be reflected otherwise? I might have missed it but I don't think there is anything in the spec that prevents users from changing compression properties.

If properties has been changes, we should build a new instance of this MetadataLocation struct.

liurenjie1024 · 2025-11-21T10:17:19Z

crates/iceberg/src/catalog/metadata_location.rs

+        properties
+            .get(TableProperties::PROPERTY_METADATA_COMPRESSION_CODEC)
+            .and_then(|codec| match codec.to_lowercase().as_str() {
+                "gzip" => Some(".gz".to_string()),


We should make ".gz" a constant rather a magic literal.

Co-authored-by: Kevin Liu <kevinjqliu@users.noreply.github.com>

…nfield/iceberg-rust into feat/metadata-compression

emkornfield added 12 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

fix clippy

41a8c1c

clippy again

51e781e

wip

253bf59

address comments

8bdb52d

no clone needed

9d27116

test compression works

d1ee0b2

comments

393622f

update tests

46fdb8e

address comments

ec96917

Remove Avro compression changes, keep only JSON metadata compression

46efa9e

liurenjie1024 reviewed Nov 20, 2025

View reviewed changes

emkornfield added 9 commits November 20, 2025 18:19

address some comments

e96fe1a

don't reference deprecated method

53d0997

don't call deprecated method

4f033e8

with_next_version

2db8b89

refactor

8b63161

fix imports

9308050

fix compile

9bd46d6

fix clipp

f3276ce

remove ;

27cbc0b

emkornfield commented Nov 21, 2025

View reviewed changes

kevinjqliu approved these changes Nov 21, 2025

View reviewed changes

liurenjie1024 reviewed Nov 21, 2025

View reviewed changes

emkornfield and others added 4 commits November 21, 2025 08:54

Update crates/iceberg/src/spec/table_properties.rs

bdc6bb5

Co-authored-by: Kevin Liu <kevinjqliu@users.noreply.github.com>

address comment on .gz

05fdd81

Merge branch 'feat/metadata-compression' of github.com-personal:emkor…

5d6404c

…nfield/iceberg-rust into feat/metadata-compression

add tests

207c0bf

fixes

e83c449

	/// Only used for creating a new table. For updates, see `with_next_version`.
	/// Only used for creating a new table. For updates, see `with_next_version_and_properties`.

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

emkornfield commented Nov 19, 2025

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

Uh oh!

emkornfield commented Nov 19, 2025

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!

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

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!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

emkornfield Nov 21, 2025 •

edited

Loading

emkornfield Nov 21, 2025 •

edited

Loading