Finish manifest splitting by dcherian · Pull Request #948 · earth-mover/icechunk

dcherian · 2025-05-13T21:05:10Z

Closes #604
Closes #332

TODO:

add splitting config to stateful test?
test for changing the splitting config
error when merging incompatible splits

icechunk/src/change_set.rs

icechunk/src/session.rs

dcherian · 2025-05-13T21:14:57Z

icechunk/src/session.rs

 }

 impl ManifestSplittingConfig {
-    pub fn get_split_sizes(&self, node: &NodeSnapshot) -> SessionResult<ManifestSplits> {


Only the indent has changed

icechunk/src/change_set.rs

dcherian · 2025-06-06T22:50:13Z

icechunk/src/change_set.rs

        &self,
        node_id: &NodeId,
        node_path: &Path,
+        extent: Option<ManifestExtents>,


I will have to learn some lifetime things to make this Option<&ManifestExtents>.

EDIT: haven't tackled this yet

icechunk/src/change_set.rs

icechunk/src/conflicts/detector.rs

dcherian · 2025-06-06T22:52:07Z

icechunk/src/session.rs

    change_set: ChangeSet,
    default_commit_metadata: SnapshotProperties,
+    // This is an optimization so that we needn't figure out the split sizes on every set.
+    splits: HashMap<NodeId, ManifestSplits>,


Stored on Session since we'll need to grab splits for arrays created in a previous session, but being modified in this one & the change_set doesn't have enough info (it receives NodeId, not NodeSnapshot IIRC)

dcherian · 2025-06-06T22:52:31Z

icechunk/src/session.rs

+            // Q: What happens if we set a chunk, then change a dimension name, so
+            //   that the split changes.
+            // A: We ignore it. splits are set once for a node in a session, and are never changed.
+            // self.cache_splits(&node.id, path, &shape, &dimension_names);


We could be stricter and error here if the splits change.

icechunk/src/session.rs

dcherian · 2025-06-06T22:54:26Z

icechunk/src/session.rs

    snapshot_id: &'a SnapshotId,
    change_set: &'a ChangeSet,
    node: NodeSnapshot,
+    extent: Option<ManifestExtents>,


Important: All iterators have gain Option<ManifestExtents> to do some filtering to extents=. This is where I'll need to add lifetimes to switch to Option<&ManifestExtents>

I wonder if for all these cases, instead of using Option it's easier to introduce a (constant) match all ManifestExtents: ManifestExtents::all : ManifestExtents

dcherian · 2025-06-06T22:56:01Z

icechunk/src/session.rs

+    ) -> SessionResult<Option<ManifestRef>> {
+        let mut from = vec![];
+        let mut to = vec![];
+        let chunks = aggregate_extents(&mut from, &mut to, chunks, |ci| &ci.coord);


I had to bring this back.

AFAICT there's no way to update the extents when chunks are deleted without scanning the whole thing. To do so, we'll need to keep track of "how many chunks have chunk index 'i'", decrement that counter when deleting chunks, and then construct a bbox from that histogram when needed. This would be a format change.

dcherian · 2025-06-06T22:56:39Z

icechunk/src/session.rs

-                #[allow(clippy::expect_used)]
-                let split_sizes = self
-                    .split_sizes
+    pub fn get_split_sizes(


no logic changes, just the signature. Arguably, it should've been this way earlier 🤦🏾‍♂️

dcherian · 2025-06-06T22:56:52Z

icechunk/src/session.rs

    Ok(asset_manager.fetch_manifest(manifest_id, manifest_info.size_bytes).await?)
 }

+/// Map the iterator to accumulate the extents of the chunks traversed


no logic changes

paraseba

I made a first pass to the easy parts. Still need to look into the manifest generation, but maybe we can chat about the easy parts first.

It looks great, made a few minor comments.

One thing I still need to understand is how will you know if a manifest split became empty after chunk deletes. That's probably in the parts I haven't read yet.

paraseba · 2025-06-08T19:38:19Z

icechunk/src/format/manifest.rs

+        let ranges = std::iter::zip(self.iter(), other.iter())
+            .map(|(a, b)| max(a.start, b.start)..min(a.end, b.end))
+            .collect::<Vec<_>>();
+        if any(ranges.iter(), |r| r.end < r.start) { None } else { Some(Self(ranges)) }


should the condition be r.end <= r.start? Aren't these [) intervals? One empty set gives an empty intersection

Should we disallow empty ManifestExtents? I thought we were doing that, but apparently not.

I think it would save us from confusion, yes.

icechunk/src/change_set.rs

paraseba · 2025-06-08T20:16:07Z

icechunk/src/change_set.rs

+        splits: &ManifestSplits,
    ) {
+        #[allow(clippy::expect_used)]
+        let (_, extent) = splits.which_extent_and_index(&coord).expect("logic bug. Trying to set chunk ref but can't find the appropriate split manifest.");


I think this is what i meant in my previous comment, isn't the index enough if you saved the SplitManifests in a vector instead of a HashMap?

icechunk/src/session.rs

icechunk/src/change_set.rs

paraseba · 2025-06-08T21:02:50Z

icechunk/src/session.rs

+    None,
+}
+
+/// Important: this is not symmetric.


maybe adding it with a different name to the impl would help make the asymmetry more clear?

Now moved to ManifestExtents.overlap_with(other: &ManifestExtents)

icechunk/src/session.rs

icechunk/src/format/manifest.rs

dcherian · 2025-06-10T01:08:36Z

icechunk/src/change_set.rs

+                // test_cases: increasing size of (multiple) dimensions
+                //             decreasing size of (multiple) dimensions
+                //
+                new_splits


yeah this is ugly. Since we track extents on the change set, we need to update them for any resize 🤦🏾‍♂️

dcherian · 2025-06-11T01:21:42Z

icechunk/src/change_set.rs

+                )
+            });
+            for (extent, their_manifest) in other_splits {
+                manifests.entry(extent).or_default().extend(their_manifest)


this is readable but seems silly for the default case where we can just insert their_manifest.

icechunk-python/tests/test_zarr/test_stateful.py

dcherian · 2025-06-11T20:33:52Z

icechunk/src/change_set.rs

+    // This map keeps track of any chunk deletes that are
+    // outside the domain of the current array shape. This is needed to handle
+    // the very unlikely case of multiple resizes in the same session.
+    deleted_chunks_outside_bounds: BTreeMap<NodeId, HashSet<ChunkIndices>>,


This seems to be the cleanest solution to me.

This commit tried tracking all deleted chunks separately (5af247e) but it's very messy.

dcherian · 2025-06-11T22:25:35Z

icechunk/src/repository.rs

-                - 1 * ((ax > 0) as usize);
-            dbg!(&ax, &add);
-            total_manifests += add;
+            total_manifests +=


number went down!

dcherian · 2025-06-18T23:13:38Z

icechunk-python/tests/test_zarr/test_stateful.py

+import functools
+
+
+def with_frequency(frequency):


this was Claude. looks good!

dcherian · 2025-06-23T17:02:48Z

Some benchmarks for a repo with 500_000 chunk refs, written to 50 manifests with 10_000 refs each.

Appends are much faster as expected. Each append adds 50_000 chunks refs = 5 new manifest files.
we have a slowdown for full rewrites . This is expected since we write each split in serial, this is slow but reduces memory consumption. As an experiment I did a crude async implementation for manifest writes per node, as expected this is a nice speedup.

After discussion with Seba, I have reverted that async write commit, and this PR should be good to go. I'll address #986 in a new PR.

Without async writes

------------------ benchmark 'refs-write test_write_split_manifest_refs_append': 4 tests ------------------
Name (time in ms)                                                                          Median
-----------------------------------------------------------------------------------------------------------
test_write_split_manifest_refs_append[no-splitting-large-1d] (p/s3_e184e2998)            792.6242 (1.64)
test_write_split_manifest_refs_append[no-splitting-large-1d] (p/s3_main_7e03)            815.8424 (1.69)
test_write_split_manifest_refs_append[split-size-10_000-large-1d] (p/s3_e184e2998)       483.8746 (1.0)
test_write_split_manifest_refs_append[split-size-10_000-large-1d] (p/s3_main_7e03)     1,987.4977 (4.11)
-----------------------------------------------------------------------------------------------------------

---------------- benchmark 'refs-write test_write_split_manifest_refs_full_rewrite': 4 tests ----------------
Name (time in s)                                                                             Median
-------------------------------------------------------------------------------------------------------------
test_write_split_manifest_refs_full_rewrite[no-splitting-large-1d] (p/s3_e184e2998)          1.7160 (1.04)
test_write_split_manifest_refs_full_rewrite[no-splitting-large-1d] (p/s3_main_7e03)          1.6557 (1.0)
test_write_split_manifest_refs_full_rewrite[split-size-10_000-large-1d] (p/s3_e184e2998)     3.9933 (2.41)
test_write_split_manifest_refs_full_rewrite[split-size-10_000-large-1d] (p/s3_main_7e03)     4.2187 (2.55)
-------------------------------------------------------------------------------------------------------------

With async writes

------------------ benchmark 'refs-write test_write_split_manifest_refs_append': 4 tests ------------------
Name (time in ms)                                                                          Median
-----------------------------------------------------------------------------------------------------------
test_write_split_manifest_refs_append[no-splitting-large-1d] (m/s3_HEAD_2a68)            709.7038 (2.97)
test_write_split_manifest_refs_append[no-splitting-large-1d] (m/s3_main_7e03)            764.4788 (3.20)
test_write_split_manifest_refs_append[split-size-10_000-large-1d] (m/s3_HEAD_2a68)       238.7356 (1.0)
test_write_split_manifest_refs_append[split-size-10_000-large-1d] (m/s3_main_7e03)     1,726.8433 (7.23)
-----------------------------------------------------------------------------------------------------------

---------------- benchmark 'refs-write test_write_split_manifest_refs_full_rewrite': 4 tests ----------------
Name (time in s)                                                                             Median
-------------------------------------------------------------------------------------------------------------
test_write_split_manifest_refs_full_rewrite[no-splitting-large-1d] (m/s3_HEAD_2a68)          1.6306 (1.22)
test_write_split_manifest_refs_full_rewrite[no-splitting-large-1d] (m/s3_main_7e03)          1.5534 (1.16)
test_write_split_manifest_refs_full_rewrite[split-size-10_000-large-1d] (m/s3_HEAD_2a68)     1.3361 (1.0)
test_write_split_manifest_refs_full_rewrite[split-size-10_000-large-1d] (m/s3_main_7e03)     3.6363 (2.72)
-------------------------------------------------------------------------------------------------------------

This reverts commit 3b52259.

dcherian commented May 13, 2025

View reviewed changes

icechunk/src/change_set.rs Outdated Show resolved Hide resolved

dcherian commented May 13, 2025

View reviewed changes

icechunk/src/session.rs Outdated Show resolved Hide resolved

dcherian commented May 13, 2025

View reviewed changes

dcherian force-pushed the manifest-split-write branch from d5398aa to e5f3d9e Compare May 13, 2025 21:44

paraseba reviewed May 14, 2025

View reviewed changes

dcherian commented May 14, 2025

View reviewed changes

icechunk/src/change_set.rs Outdated Show resolved Hide resolved

dcherian force-pushed the manifest-split-write branch from e5f3d9e to 52a6e4c Compare June 6, 2025 22:48

dcherian commented Jun 6, 2025

View reviewed changes

icechunk/src/change_set.rs Show resolved Hide resolved

dcherian commented Jun 6, 2025

View reviewed changes

icechunk/src/conflicts/detector.rs Show resolved Hide resolved

dcherian commented Jun 6, 2025

View reviewed changes

icechunk/src/session.rs Show resolved Hide resolved

dcherian commented Jun 6, 2025

View reviewed changes

paraseba reviewed Jun 8, 2025

View reviewed changes

dcherian commented Jun 10, 2025

View reviewed changes

icechunk/src/format/manifest.rs Show resolved Hide resolved

dcherian commented Jun 10, 2025

View reviewed changes

dcherian force-pushed the manifest-split-write branch 3 times, most recently from 6fe675b to 6d52d2c Compare June 10, 2025 23:29

dcherian commented Jun 11, 2025

View reviewed changes

dcherian force-pushed the manifest-split-write branch 2 times, most recently from c775d21 to ce67942 Compare June 11, 2025 20:26

dcherian commented Jun 11, 2025

View reviewed changes

icechunk-python/tests/test_zarr/test_stateful.py Outdated Show resolved Hide resolved

dcherian commented Jun 11, 2025

View reviewed changes

dcherian force-pushed the manifest-split-write branch from ce67942 to cd750e6 Compare June 11, 2025 20:35

dcherian commented Jun 11, 2025

View reviewed changes

dcherian added 8 commits June 17, 2025 13:52

More complex test + fixes

2a6d65d

more test

27ad4bc

Fix merging

14d70df

reorg tests

27c7c4b

edits

9e1c8ad

Add conflicting commits test

e974f48

Add test for splits changing in a session

76e58fd

lint

a306abf

dcherian force-pushed the manifest-split-write branch from 50f2a27 to 46d13b6 Compare June 17, 2025 20:21

dcherian added 2 commits June 17, 2025 15:14

Review comments

e719c7f

Use ManifestExtents::ALL sentinel

b7fb385

dcherian force-pushed the manifest-split-write branch from 900f798 to b7fb385 Compare June 17, 2025 21:14

dcherian commented Jun 18, 2025

View reviewed changes

dcherian force-pushed the manifest-split-write branch from 4de248c to 1020ec1 Compare June 18, 2025 23:32

dcherian added 2 commits June 20, 2025 10:11

Update stateful tests

4a2e455

update benchmarks dep group

b179dbc

dcherian force-pushed the manifest-split-write branch from 1020ec1 to b179dbc Compare June 20, 2025 20:29

dcherian added 3 commits June 20, 2025 16:16

small edits

e184e29

Parallel writes

3b52259

more benchmarks

4e68231

dcherian force-pushed the manifest-split-write branch from 2a681ce to 4e68231 Compare June 21, 2025 23:47

dcherian added 2 commits June 23, 2025 11:47

Revert "Parallel writes"

d0c42be

This reverts commit 3b52259.

lint

fcdc2d6

dcherian requested a review from paraseba June 23, 2025 18:08

paraseba approved these changes Jun 24, 2025

View reviewed changes

Merge branch 'main' into manifest-split-write

ed648a8

dcherian merged commit 0f24418 into main Jun 24, 2025
11 checks passed

dcherian deleted the manifest-split-write branch June 24, 2025 20:09

TomNicholas mentioned this pull request Jun 24, 2025

PetaByte-scale virtual ref performance benchmark #401

Open

		import functools


		def with_frequency(frequency):

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dcherian Jun 6, 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

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!

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!

dcherian commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Without async writes

With async writes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

dcherian commented May 13, 2025 •

edited

Loading

dcherian Jun 6, 2025 •

edited

Loading

dcherian commented Jun 23, 2025 •

edited

Loading