feat: Honor compression settings for metadata.json on write by emkornfield · Pull Request #1876 · apache/iceberg-rust

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.

BREAKING CHANGE: Make write_to take MetadataLocation and MetadataLocation::with_next_version take properties.

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).

While I think this fix is correct, I think it's much related to this pr? We should move it to a standalone pr.

We needed change this anyways do to the reordering of calls, it seems better to fix it. I'll open a separate PR.

#2115 to make the change isolated.

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.

crates/iceberg/src/catalog/metadata_location.rs

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.

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

My main concern here, is I don't think this is something that clients of MetadataLocation would actually understand and check for a new version of the metadata (i.e. the path of least resistance is to call with_next_version on whatever metadata they used for creation. As a solution, I've created a static factory next_version which combines the two steps for the only call-site that exists for with_next_version. If you think it is better to not deprecated with_next_version I can also revert this change.

crates/iceberg/src/catalog/metadata_location.rs

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

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

emkornfield · 2026-01-20T19:45:29Z

@liurenjie1024 thanks for the review, I think this is ready for another look.

…pression

liurenjie1024 · 2026-01-21T01:50:40Z

crates/iceberg/src/spec/table_properties.rs


+/// Compression codec for metadata files (JSON).
+#[derive(Debug, PartialEq)]
+pub enum MetadataCompressionCodec {


We already have a CompressionCodec, we should move it to spec module and reuse it.

Note that, that compression codec doesn't actually include GZIP which means extra validation for this code path. I'm not opposed to it but given that it will take extra logic to validate valid options for both this PR and the existing code in puffin to not allow writing GZIP and we also have a different concept for Avro files in #1851 it seems better do the refactoring change in a separate PR once everything is merged (conveniently reaching the rule of 3?

I have concerns with this approach. OSS software is somehow different enterprise software development. If everyone just move forward with a lot of diverges and left the cleanup things to others, the codebase will become a mess. I think a better approach would be to have another pr to move the compression codec enum into spec module. And then we can move on with this pr after it got merged.

OK, thank you for the guidance.

Created #2081

Updated to use that code.

) ## Which issue does this PR close? As discussed in the PR for [allowing compressed metadata.json](#1876 (comment)) writes we want CompressionCodec in a central place so it can be used in puffin and for other compression use-cases. Happy to move it to `spec/` as suggested in the original comment but this seemed more natural. ## What changes are included in this PR? This moves compression.rs to a top level (seems like the best location for common code but please let me know if there is a better place. - It adds Gzip as a compression option and replaces current direct use the GzEncoder package. - It adds validation to puffin that Gzip is not currently supported (per spec) ## Are these changes tested? Added unit tests to cover additions and existing tests cover the rest.

…pression

blackmwk

Thanks @emkornfield for this pr, we are quite close !

crates/iceberg/src/catalog/metadata_location.rs

blackmwk · 2026-02-02T01:50:45Z

crates/iceberg/src/catalog/metadata_location.rs


    /// Creates a new metadata location for an updated metadata file.
+    /// Uses compression settings from the new metadata.
+    pub fn next_version(


I'm still confused why this method is better than self.with_next_version? The self.with_next_version looks more reasonable to me since we need the original version, which is contained in self.

The current next_with_version needs to be deprecated either way because it doesn't take new properties (i.e. compression can change). If we don't deprecate it and just replace it it could be viable.

If using the member function is more idiomatic, happy to switch to that, IIRC @liurenjie1024 had issue with that though.

I think I got your point. Then a more idiomatic approach would be adding a new member function as following:

pub fn with_new_metadata(&self, metadata: &TableMetadata) -> Self { ... }

And the user is expected to call as following:

let meta_loc: MetadataLocation = ...; let new_meta_loc = meta_loc.with_next_version().with_new_metadata();

The issue I have with this API is it requires callers to remember with_new_metadata, but the metadata is fundamental for correctly generating the location.

@blackmwk I've updated the APIs here to avoid backwards compatibility, would appreciate your thoughts on the current approach.

blackmwk · 2026-02-02T01:54:56Z

crates/iceberg/src/spec/table_metadata.rs

@@ -468,9 +483,44 @@ impl TableMetadata {
        file_io: &FileIO,
        metadata_location: impl AsRef<str>,


I think we may need to change metadata_location's type to the struct MetadataLocation, rather than impl AsRef<str>. Also we should not infer compression codec from suffix, instead we should infer it from TableMetadata itself. With this change, we could avoid a lot of validation in the method body.

I think we may need to change metadata_location's type to the struct MetadataLocation, rather than impl AsRef.

I was trying to avoid a breaking change here, are there general policies I should be aware of (for some reason I thought there was a preference not have breaking changes). if we don't mind breaking changes, I'm happy to make the change).

Also we should not infer compression codec from suffix, instead we should infer it from TableMetadata itself. With this change, we could avoid a lot of validation in the method body.

I don't believe we are inferring compression codec from the suffix. The complexity here is since we are only deprecating with_next_version, we end up writing the wrong name under the following scenario.

Metadata file 0 - has no compression

Caller uses with_next_version to generate the next location.

Caller updates to use compression = 'gz'

Without this logic we would write out a gzipped file without the compression suffix, which would break other readers (e.g. Java library).

I realize I actually missed handling the same transition for CompressionCodec::None => (json_data, metadata_location.as_ref().to_string()), which would check if the suffix is compressed and remove it.

if we chose to just delete with_next_version (or update to take TableMetadata on the existing method). A lot of complexity goes away (we no longer need parse the suffix, although it might be nice for display purposes) and we don't need the special checks here.

So in short I think the two paths forward are:

Breaking change for MetadataLocation to force taking properties on next version (and not deprecate the method).

Fix the Match on None to handle a similar down-grade.

I think other options introduce a risk that clients can end up writing out metadata json files that other readers will not be able to read.

I have added comments here: https://github.com/apache/iceberg-rust/pull/1876/changes#r2756683326
I think with this approach we will be able provide an idiomatic api.

Responded there. This code is much simpler now that we are storing the codec directly, and taking TableLocation

blackmwk · 2026-02-02T01:57:04Z

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()


While I think this fix is correct, I think it's much related to this pr? We should move it to a standalone pr.

emkornfield · 2026-02-02T04:03:57Z

@blackmwk thanks for the review I think we need to close on a direct for this: https://github.com/apache/iceberg-rust/pull/1876/files#r2752454670

I'll separate out the S3 fix into its own PR.
Use the new compression enum on MetadataLocation.

If we can make breaking changes then:

Then if write-to takes metadata location then we don't need to store compression codec on MetadataLocation at all (we can make it take a compression codec when generating the final path). (I actually need to double check this, there might be other places that try to reserialize the string).
We can eliminate a lot of the validation logic.

blackmwk · 2026-02-03T01:04:40Z

Hi, @emkornfield

I'll separate out the S3 fix into its own PR.
Use the new compression enum on MetadataLocation.

I think this is the right direction to go.

If we can make breaking changes then:

Then if write-to takes metadata location then we don't need to store compression codec on MetadataLocation at all (we can make it take a compression codec when generating the final path). (I actually need to double check this, there might be other places that try to reserialize the string).
We can eliminate a lot of the validation logic.

This is the place where we have to make a breaking change, so I think it's fine.

…pression

emkornfield · 2026-02-05T21:26:26Z

crates/iceberg/src/spec/table_properties.rs

+
+    #[test]
+    fn test_parse_metadata_file_compression_valid() {
+        use super::parse_metadata_file_compression;


need to move these imports out and clean up this code a little bit.

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

emkornfield requested a review from liurenjie1024 January 20, 2026 19:44

emkornfield added 5 commits January 20, 2026 20:05

wip

eaf1570

one more comment

889cd03

fmt

268d830

Merge remote-tracking branch 'databricks/main' into feat/metadata-com…

2fd720f

…pression

replace more magic constants

664e541

liurenjie1024 reviewed Jan 21, 2026

View reviewed changes

emkornfield requested a review from liurenjie1024 January 21, 2026 06:03

emkornfield mentioned this pull request Jan 29, 2026

feat: Factor out CompressionCodec to common location and add Gzip #2081

Merged

emkornfield added 7 commits January 29, 2026 03:49

Merge remote-tracking branch 'databricks/main' into feat/metadata-com…

1bfa1b8

…pression

wip

86854a8

fix

4e0caed

cleanup

0b936e9

wip

e17b626

fmt

f321a03

Clippy

4b102e1

blackmwk reviewed Feb 2, 2026

View reviewed changes

emkornfield requested a review from blackmwk February 2, 2026 04:12

emkornfield mentioned this pull request Feb 2, 2026

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

Draft

emkornfield added 4 commits February 5, 2026 18:56

wip

fa7f1a9

use CompressionCodec on metadata location

f2595df

change write_to

24ccd8f

Merge remote-tracking branch 'databricks/main' into feat/metadata-com…

0bf562b

…pression

emkornfield commented Feb 5, 2026

View reviewed changes

emkornfield added 2 commits February 5, 2026 21:33

fix nits

ab8a4b5

fix s3tables compilation

91df534

	/// 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`.

		@@ -468,9 +483,44 @@ impl TableMetadata {
		file_io: &FileIO,
		metadata_location: impl AsRef<str>,

Conversation

emkornfield commented Nov 19, 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!

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!

Choose a reason for hiding this comment

Uh oh!

emkornfield Feb 2, 2026 • 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!

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!

Uh oh!

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!

Uh oh!

emkornfield commented Jan 20, 2026

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!

blackmwk left a comment

Choose a reason for hiding this comment

Uh oh!

emkornfield commented Nov 19, 2025 •

edited

Loading

emkornfield Nov 21, 2025 •

edited

Loading

emkornfield Feb 2, 2026 •

edited

Loading

emkornfield Nov 21, 2025 •

edited

Loading

emkornfield Feb 2, 2026 •

edited

Loading

emkornfield Feb 5, 2026 •

edited

Loading

emkornfield commented Feb 2, 2026 •

edited

Loading