Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
76 commits
Select commit Hold shift + click to select a range
de9ab79
[WIP] manifest sharding
dcherian Feb 27, 2025
14d9048
WIP
dcherian Mar 4, 2025
6aa7e29
More condition parsing work
dcherian Mar 6, 2025
70412ec
thread config through
dcherian Mar 7, 2025
267c9cf
clippied
dcherian Mar 17, 2025
acf8fa5
WIP proptest
dcherian Mar 17, 2025
2542412
Revert "WIP proptest"
dcherian Mar 17, 2025
d816f8b
Simple test passes!
dcherian Mar 17, 2025
a64252a
Cleanup
dcherian Mar 18, 2025
8ad20e8
Better test
dcherian Mar 18, 2025
ac42185
More tests
dcherian Mar 18, 2025
34da81b
Lossen type
dcherian Mar 18, 2025
d935131
Update gc test
dcherian Mar 18, 2025
d27a2a2
DimensionName(regex)
dcherian Mar 18, 2025
83fef67
more test
dcherian Mar 18, 2025
b47788c
Revert "Lossen type"
dcherian Mar 18, 2025
a56ccce
Add condition parsing tests
dcherian Mar 18, 2025
87af732
WIP
dcherian Mar 19, 2025
63fdc6d
Add notes
dcherian Mar 20, 2025
d491c3e
Iterator -> Stream
dcherian Mar 20, 2025
fc965d7
Optimize reads!
dcherian Mar 21, 2025
b2167f7
Python config
dcherian Mar 24, 2025
dd95c32
Add reprs
dcherian Mar 26, 2025
fb89826
Add from_dict
dcherian Mar 26, 2025
edb1d13
clippy
dcherian Mar 26, 2025
4732a61
[revert] comment out bad test
dcherian Mar 26, 2025
5b70006
Add doctest to Just
dcherian Mar 31, 2025
0ce517e
Add doctest
dcherian Mar 31, 2025
dd9b77d
Merge branch 'main' into split-manifests
dcherian Mar 31, 2025
fc19e7f
Fix appends
dcherian Mar 31, 2025
22deffe
Add updating config on read test
dcherian Mar 31, 2025
2121f9a
Fix types
dcherian Mar 31, 2025
56158c5
add comment to clarify monkey patch
dcherian Mar 31, 2025
1a0215c
Update icechunk-python/python/icechunk/_icechunk_python.pyi
dcherian Apr 1, 2025
deb5a7e
Fix docstring types
dcherian Mar 31, 2025
d749f41
Update type, docstring
dcherian Apr 1, 2025
e648329
ShardDimCondition::Any -> Rest
dcherian Apr 1, 2025
aa82355
Minor cleanup
dcherian Apr 1, 2025
5d2318b
More complex tests
dcherian Apr 1, 2025
10e3b7e
Aggregate extents while grouping shards.
dcherian Apr 1, 2025
137f283
Update reprs
dcherian Apr 1, 2025
1119f81
Benchmarks cleanup
dcherian Apr 1, 2025
f3db41b
Add write benchmark
dcherian Apr 1, 2025
ac6a0ef
Add read benchmark
dcherian Apr 2, 2025
2f42e71
Merge branch 'main' into split-manifests
dcherian Apr 3, 2025
ea50452
Add rust test for large numbers of refs
dcherian Apr 3, 2025
2ea8e2a
Merge branch 'main' into split-manifests
dcherian Apr 3, 2025
33966cb
Add to test_can_read_old.py
dcherian Apr 3, 2025
80f8f5d
shard → split
dcherian Apr 4, 2025
94bbf9e
Merge branch 'main' into split-manifests
dcherian Apr 4, 2025
6fc7eb6
one more rename
dcherian Apr 4, 2025
a76558d
Merge branch 'main' into split-manifests
dcherian Apr 15, 2025
c35b589
Address minor comments.
dcherian Apr 15, 2025
f6156b9
Comment out handling sessionerror.
dcherian Apr 15, 2025
48bebf2
Rest -> Any
dcherian Apr 15, 2025
0d7e01f
Assert len(manifestextents) > 0
dcherian Apr 15, 2025
8c4cc59
New ManifestSplitDim struct
dcherian Apr 15, 2025
872a522
lint
dcherian Apr 15, 2025
77329bf
Merge branch 'main' into split-manifests
dcherian May 7, 2025
8d24d01
Remove unneeded Index
dcherian May 7, 2025
5d6cefa
Add property test
dcherian May 7, 2025
5e34e6c
Add docs
dcherian May 7, 2025
315d1df
Add ManifestSplitCondition.AnyArray
dcherian May 7, 2025
9954cda
fix docs
dcherian May 8, 2025
0b3581d
Add Or condition test
dcherian May 8, 2025
c1c7688
fix docs
dcherian May 8, 2025
9e625cf
fix docs
dcherian May 8, 2025
39125a8
fix docs
dcherian May 8, 2025
b5812d5
Try speeding up docs build
dcherian May 8, 2025
02c95b6
fix docs
dcherian May 8, 2025
8051f92
try fixing docs rendering
dcherian May 12, 2025
bf8936e
Apply suggestions from code review
dcherian May 13, 2025
371c045
more docs
dcherian May 13, 2025
5a955f5
Fix test
dcherian May 13, 2025
163d31c
tweak docs build
dcherian May 13, 2025
0f44af8
Merge branch 'main' into split-manifests
dcherian May 13, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 92 additions & 1 deletion icechunk/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,11 @@ use std::{
use async_trait::async_trait;
use chrono::{DateTime, Utc};
pub use object_store::gcp::GcpCredential;
use regex::bytes::Regex;
use serde::{Deserialize, Serialize};

use crate::{
format::Path,
storage,
virtual_chunks::{ContainerName, VirtualChunkContainer, mk_default_containers},
};
Expand Down Expand Up @@ -128,6 +130,82 @@ impl CachingConfig {
}
}

#[derive(Debug, PartialEq, Eq, Serialize, Hash, Deserialize, Clone)]
#[serde(rename_all = "snake_case")]
pub enum ManifestShardCondition {
Or(Vec<ManifestShardCondition>),
And(Vec<ManifestShardCondition>),
PathMatches { regex: String },
NameMatches { regex: String },
}

//```yaml
//rules:
// - path: ./2m_temperature # regex, 3D variable: (null, latitude, longitude)
// manifest-split-sizes:
// - 0: 120
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make it more clear, could you give an example where we mix index based keys and coord names?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe that's not possible for the same array?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be ok if it's not possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible: Python Example Rust Example

Since Zarr allows dimension_names: Option<Vec<Option<String>>> we have no option ;).

// - path: ./temperature # 4D variable: (time, level, latitude, longitude)
// manifest-split-sizes:
// - "level": 1 # alternatively 0: 1
// - "time": 12 # and 1: 12
// - path: ./temperature
// manifest-split-sizes:
// - "level": 1
// - "time": 8760 # ~1 year
// - "latitude": null # for unspecified, default is null, which means never split.
// - path: ./* # the default rules
// manifest-split-sizes: null # no splitting, just a single manifest per array
//```

impl ManifestShardCondition {
// from_yaml?
pub fn matches(&self, path: &Path) -> bool {
match self {
ManifestShardCondition::Or(vec) => vec.iter().any(|c| c.matches(path)),
ManifestShardCondition::And(vec) => vec.iter().all(|c| c.matches(path)),
// TODO: precompile the regex
ManifestShardCondition::PathMatches { regex } => Regex::new(regex)
.map(|regex| regex.is_match(path.to_string().as_bytes()))
.unwrap_or(false),
// TODO: precompile the regex
ManifestShardCondition::NameMatches { regex } => Regex::new(regex)
.map(|regex| {
path.name()
.map(|name| regex.is_match(name.as_bytes()))
.unwrap_or(false)
})
.unwrap_or(false),
}
}
}

// FIXME: isn't this really another condition?
#[derive(Debug, Hash, PartialEq, Eq, Serialize, Deserialize, Clone)]
pub enum ShardDimCondition {
Axis(usize),
DimensionName(String),
// TODO: Since dimension name can be null,
// i don't think we can have DimensionName(r"*") catch the "Any" case
Any,
}

#[derive(Debug, PartialEq, Eq, Serialize, Deserialize, Clone)]
pub struct ManifestShardingConfig {
// TODO: need to preserve insertion order of conditions, so hashmap doesn't work
pub shard_sizes: Vec<(ManifestShardCondition, Vec<(ShardDimCondition, u32)>)>,
}

impl Default for ManifestShardingConfig {
fn default() -> Self {
let inner = vec![(ShardDimCondition::Any, u32::MAX)];
let new = vec![(
ManifestShardCondition::PathMatches { regex: r".*".to_string() },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this condition just be True instead?

inner,
)];
Self { shard_sizes: new }
}
}

#[derive(Debug, PartialEq, Eq, Serialize, Deserialize, Clone)]
#[serde(rename_all = "snake_case")]
pub enum ManifestPreloadCondition {
Expand Down Expand Up @@ -206,20 +284,33 @@ static DEFAULT_MANIFEST_PRELOAD_CONDITION: OnceLock<ManifestPreloadCondition> =
#[derive(Debug, PartialEq, Eq, Serialize, Deserialize, Clone, Default)]
pub struct ManifestConfig {
pub preload: Option<ManifestPreloadConfig>,
pub sharding: Option<ManifestShardingConfig>,
}

static DEFAULT_MANIFEST_PRELOAD_CONFIG: OnceLock<ManifestPreloadConfig> = OnceLock::new();
static DEFAULT_MANIFEST_SHARDING_CONFIG: OnceLock<ManifestShardingConfig> =
OnceLock::new();

impl ManifestConfig {
pub fn merge(&self, other: Self) -> Self {
Self { preload: other.preload.or(self.preload.clone()) }
Self {
preload: other.preload.or(self.preload.clone()),
// FIXME: why prioritize one over the other?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we use this to overwrite the default config with whatever the user passed, so order is important. But I don't like it, because implementing merge requires you to know this detail

sharding: other.sharding.or(self.sharding.clone()),
}
}

pub fn preload(&self) -> &ManifestPreloadConfig {
self.preload.as_ref().unwrap_or_else(|| {
DEFAULT_MANIFEST_PRELOAD_CONFIG.get_or_init(ManifestPreloadConfig::default)
})
}

pub fn sharding(&self) -> &ManifestShardingConfig {
self.sharding.as_ref().unwrap_or_else(|| {
DEFAULT_MANIFEST_SHARDING_CONFIG.get_or_init(ManifestShardingConfig::default)
})
}
}

#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Default)]
Expand Down
84 changes: 76 additions & 8 deletions icechunk/src/format/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::format::flatbuffers::generated;
use bytes::Bytes;
use flatbuffers::VerifierOptions;
use futures::{Stream, TryStreamExt};
use itertools::Itertools;
use itertools::{Itertools, multiunzip, repeat_n};
use serde::{Deserialize, Serialize};
use thiserror::Error;

Expand All @@ -21,12 +21,6 @@ use super::{
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub struct ManifestExtents(Vec<Range<u32>>);

#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub struct ManifestRef {
pub object_id: ManifestId,
pub extents: ManifestExtents,
}

impl ManifestExtents {
pub fn new(from: &[u32], to: &[u32]) -> Self {
let v = from
Expand All @@ -37,9 +31,83 @@ impl ManifestExtents {
Self(v)
}

pub fn contains(&self, coord: &[u32]) -> bool {
self.iter().zip(coord.iter()).all(|(range, that)| range.contains(that))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to start checking on writes that indexes have the proper size for the metadata

}

pub fn iter(&self) -> impl Iterator<Item = &Range<u32>> {
self.0.iter()
}

pub fn len(&self) -> usize {
self.0.len()
}

pub fn is_empty(&self) -> bool {
self.0.is_empty()
}
}

#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub struct ManifestRef {
pub object_id: ManifestId,
pub extents: ManifestExtents,
}

#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct ManifestShards(Vec<ManifestExtents>);

impl ManifestShards {
pub fn default(ndim: usize) -> Self {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this, but it is certainly tied to ndim.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe ManifestSplits is an enum to avoid this?

enum ManifestSplits {
   Single,
   Multiple(Vec<ManifestExtents>)
}

What I don't like is the empty vector. I wonder if Rust has a NonEmptyVec type, otherwise, a trick people use is:

...
   Multiple{ first: ManifestExtents, rest: Vec<ManifestExtents>}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I don't need the default any more. It was an artifact that appeared because I implemented the core logic before wiring up the config. Now the default gets set when parsing the config using the Array Metadata
image

Self(vec![ManifestExtents(repeat_n(0..u32::MAX, ndim).collect())])
}
pub fn is_empty(&self) -> bool {
self.0.is_empty()
}
pub fn from_edges(iter: impl IntoIterator<Item = Vec<u32>>) -> Self {
let res = iter
.into_iter()
.map(|x| x.into_iter().tuple_windows())
.multi_cartesian_product()
.map(multiunzip)
.map(|(from, to): (Vec<u32>, Vec<u32>)| {
ManifestExtents::new(from.as_slice(), to.as_slice())
});
Self(res.collect())
}

// Returns the index of shard_range that includes ChunkIndices
// This can be used at write time to split manifests based on the config
// and at read time to choose which manifest to query for chunk payload
pub fn which(&self, coord: &ChunkIndices) -> Result<usize, IcechunkFormatError> {
// shard_range[i] must bound ChunkIndices
// 0 <= return value <= shard_range.len()
// it is possible that shard_range does not include a coord. say we have 2x2 shard grid
// but only shard (0,0) and shard (1,1) are populated with data.
// A coord located in (1, 0) should return Err
// Since shard_range need not form a regular grid, we must iterate through and find the first result.
// ManifestExtents in shard_range MUST NOT overlap with each other. How do we ensure this?
// ndim must be the same
// debug_assert_eq!(coord.0.len(), shard_range[0].len());
// FIXME: could optimize for unbounded single manifest
self.iter()
.enumerate()
.find(|(_, e)| e.contains(coord.0.as_slice()))
.map(|(i, _)| i)
.ok_or(IcechunkFormatError::from(
IcechunkFormatErrorKind::InvalidIndexForSharding {
coords: coord.clone(),
},
))
}

pub fn iter(&self) -> impl Iterator<Item = &ManifestExtents> {
self.0.iter()
}

pub fn len(&self) -> usize {
self.0.len()
}
}

#[derive(Debug, Error)]
Expand Down Expand Up @@ -206,7 +274,7 @@ impl Manifest {
}

if array_manifests.is_empty() {
// empty manifet
// empty manifest
return Ok(None);
}

Expand Down
2 changes: 2 additions & 0 deletions icechunk/src/format/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,8 @@ pub enum IcechunkFormatErrorKind {
NodeNotFound { path: Path },
#[error("chunk coordinates not found `{coords:?}`")]
ChunkCoordinatesNotFound { coords: ChunkIndices },
#[error("invalid chunk index for sharding manifests: {coords:?}")]
InvalidIndexForSharding { coords: ChunkIndices },
#[error("manifest information cannot be found in snapshot `{manifest_id}`")]
ManifestInfoNotFound { manifest_id: ManifestId },
#[error("invalid magic numbers in file")]
Expand Down
33 changes: 32 additions & 1 deletion icechunk/src/format/snapshot.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use std::{collections::BTreeMap, convert::Infallible, num::NonZeroU64, sync::Arc};
use std::{
collections::BTreeMap, convert::Infallible, num::NonZeroU64, ops::Index, sync::Arc,
};

use bytes::Bytes;
use chrono::{DateTime, Utc};
Expand Down Expand Up @@ -37,6 +39,17 @@ impl DimensionShape {
pub struct ArrayShape(Vec<DimensionShape>);

impl ArrayShape {
pub fn len(&self) -> usize {
self.0.len()
}
pub fn is_empty(&self) -> bool {
self.0.is_empty()
}

pub fn num_chunks(&self) -> Vec<u32> {
self.max_chunk_indices_permitted().map(|x| x + 1).collect()
}

pub fn new<I>(it: I) -> Option<Self>
where
I: IntoIterator<Item = (u64, u64)>,
Expand Down Expand Up @@ -87,6 +100,15 @@ impl ArrayShape {
}
}

// Implement indexing for immutable access
impl Index<usize> for ArrayShape {
type Output = DimensionShape;

fn index(&self, index: usize) -> &DimensionShape {
&self.0[index]
}
}

#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub enum DimensionName {
NotSpecified,
Expand All @@ -102,6 +124,15 @@ impl From<Option<&str>> for DimensionName {
}
}

impl From<DimensionName> for Option<String> {
fn from(value: DimensionName) -> Option<String> {
match value {
DimensionName::NotSpecified => None,
DimensionName::Name(name) => Some(name),
}
}
}

impl From<&str> for DimensionName {
fn from(value: &str) -> Self {
if value.is_empty() {
Expand Down
Loading
Loading