Add roundtrip serialization property tests for types in change sets file by ebarylko · Pull Request #1525 · earth-mover/icechunk

ebarylko · 2025-12-25T09:08:03Z

Summary

There are property tests in icechunk/src/change_set.rs which check that the composition of serializing and deserializing a ChangeSet is equivalent to the identity function.

Changes

icechunk/src/change_set.rs

Added arbitraries for the ChangeSet type
Added a roundtrip serialization test for the ChangeSet type

icechunk/src/format/manifest.rs

Changed the ManifestExtents strategy used for some of the tests

icechunk/src/strategies.rs

Added new strategies used for constructing the ChangeSet type in icechunk/src/change_set.rs

icechunk/src/storage/s3.rs

Upgraded the behaviour version for the aws config

paraseba

Looks great, a couple of minor comments

paraseba · 2025-12-26T15:45:20Z

icechunk/src/strategies.rs

+
+type SplitManifest = BTreeMap<ChunkIndices, Option<ChunkPayload>>;
+
+pub fn chunk_indices2() -> impl Strategy<Value = ChunkIndices> {


why not using the existing strategy?

I thought it would be an easier way to use the existing chunk_indices function by having the new function generate the inputs used for the old function.

paraseba · 2025-12-26T15:46:17Z

icechunk/src/strategies.rs

+}
+
+pub fn split_manifest() -> impl Strategy<Value = SplitManifest> {
+    btree_map(chunk_indices2(), option::of(chunk_payload()), 3..10)


An issue here is that all chunk indices in a single split manifest should have the same number of dimensions

I see. I will change this so all generated chunk indices are of the same size.

paraseba · 2025-12-26T15:47:05Z

icechunk/src/strategies.rs

 }

-pub fn manifest_extents(ndim: usize) -> impl Strategy<Value = ManifestExtents> {
+pub fn manifest_extents2(ndim: usize) -> impl Strategy<Value = ManifestExtents> {


Let's delete this function if we believe the new one is superior

Although the new function is simpler, it does not work when used in the test_property_extents_widths test in manifest.rs.

I still have not figured out how to replicate the property that the test expects in the new function. That is why I left both versions of the function

Do you happen to know why the delta values fall in [0, 99] for the test_property_extents_width test?

Feel free to modify that test as needed. There is no profound reason for the values selected there.

I see. Thank you for clarifying that.

paraseba · 2025-12-26T15:50:25Z

icechunk/src/change_set.rs

+    use proptest::prelude::*;
+
+    prop_compose! {
+        fn edit_changes()(num_of_dims in any::<u16>().prop_map(usize::from))(


to make it more realistic let's keep number of dimensions in [1, 20]

I see. I will shrink the range of values for num_of_dims.

paraseba · 2026-01-07T19:23:02Z

icechunk/src/strategies.rs

 }

-pub fn manifest_extents(ndim: usize) -> impl Strategy<Value = ManifestExtents> {
+pub fn manifest_extents2(ndim: usize) -> impl Strategy<Value = ManifestExtents> {


Feel free to modify that test as needed. There is no profound reason for the values selected there.

paraseba · 2026-01-07T19:28:34Z

icechunk/src/strategies.rs

+
+type SplitManifest = BTreeMap<ChunkIndices, Option<ChunkPayload>>;
+
+// pub fn chunk_indices2() -> impl Strategy<Value = ChunkIndices> {


let's delete this

Ok. I will do that.

Let's delete this please @ebarylko

I am sorry. I did not realize I forgot to delete the commented code

Just deleted it.

paraseba · 2026-01-07T19:29:19Z

icechunk/src/strategies.rs

+//         .prop_flat_map(|(dim, data)| chunk_indices(dim, data))
+// }
+
+pub fn chunk_indices2(dim: usize) -> impl Strategy<Value = ChunkIndices> {


maybe a name like large_chunk_indices

That sounds good to me. I will apply that change.

paraseba · 2026-01-16T01:23:21Z

icechunk/src/strategies.rs

    })
 }

+pub fn manifest_extents(ndim: usize) -> impl Strategy<Value = ManifestExtents> {


Can we unify the two manifest_extents functions? Otherwise let's use better names and document the different between the two

At the moment, I have not found a way to do so. I will change the names and add a docstring for each function.

paraseba · 2026-01-16T01:24:15Z

icechunk/src/strategies.rs

+
+type SplitManifest = BTreeMap<ChunkIndices, Option<ChunkPayload>>;
+
+// pub fn chunk_indices2() -> impl Strategy<Value = ChunkIndices> {


Let's delete this please @ebarylko

paraseba reviewed Dec 26, 2025

View reviewed changes

paraseba reviewed Jan 7, 2026

View reviewed changes

ebarylko force-pushed the add-roundtrip-serialization-property-tests-for-types-in-change-sets-file branch from ca70a53 to 7da443d Compare January 7, 2026 20:36

ebarylko added 27 commits January 13, 2026 14:13

Made some changes

9b85ef4

Made a small change

a32cea0

Added strategies

029bc67

Added an arbitrary

3b973ab

Added an arbitrary

7eff64a

Added an arbitrary

61b0a68

Made some changes

7ecd3ae

Changed the tests

65d0d8c

Made some changes

b71655d

Made changes to the arbitrary

8fff7ba

Made some changes

30cb75c

Changed an arbitrary

8fa831e

Made more changes

a1fc48a

Made a small change

65056c2

Modified a strategy

292d327

Changed an arbitrary for generating manifests

e3b2bc9

Changed an arbitrary

393807f

Made a change

4b55610

Made a change

49cac1d

Made some changes

9592545

Moved an arbitrary

9660715

Made some changes

94a5eb6

Made a small change

ce2c5cf

Made a small update

3420f21

Removed tempfile

a798664

Made a small change

edd65ee

Made a small change

3795aff

ebarylko added 8 commits January 13, 2026 14:13

Made a small change

7ed02e9

Fixed an arbitrary

d02e651

Made a change

2ff5e65

Removed unused code

0abd0da

Made some changes

4158020

Formatted the code

1a5a0ab

Made some changes

abfc22e

Made some changes

e7da217

ebarylko force-pushed the add-roundtrip-serialization-property-tests-for-types-in-change-sets-file branch from bd3bfe8 to e7da217 Compare January 13, 2026 22:13

ebarylko added 2 commits January 15, 2026 15:21

Updated aws-sdk-s3 crate to a version without a vulnerability

4abbcd4

Updated behaviour version

fceaacc

paraseba reviewed Jan 16, 2026

View reviewed changes

Changed the names of the manifest extent arbitraries

e50cc15

paraseba approved these changes Jan 16, 2026

View reviewed changes

paraseba enabled auto-merge (squash) January 16, 2026 02:17

paraseba disabled auto-merge January 16, 2026 02:17

paraseba merged commit 078280b into earth-mover:main Jan 16, 2026
15 of 17 checks passed

ebarylko deleted the add-roundtrip-serialization-property-tests-for-types-in-change-sets-file branch March 3, 2026 17:11


		type SplitManifest = BTreeMap<ChunkIndices, Option<ChunkPayload>>;

		pub fn chunk_indices2() -> impl Strategy<Value = ChunkIndices> {


		type SplitManifest = BTreeMap<ChunkIndices, Option<ChunkPayload>>;

		// pub fn chunk_indices2() -> impl Strategy<Value = ChunkIndices> {

Conversation

ebarylko commented Dec 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

icechunk/src/change_set.rs

icechunk/src/format/manifest.rs

icechunk/src/strategies.rs

icechunk/src/storage/s3.rs

Uh oh!

paraseba 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ebarylko commented Dec 25, 2025 •

edited

Loading