Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sync with zarr python 3 beta 2 #388

Merged
merged 11 commits into from
Nov 15, 2024
Merged

Sync with zarr python 3 beta 2 #388

merged 11 commits into from
Nov 15, 2024

Conversation

mpiannucci
Copy link
Contributor

@mpiannucci mpiannucci commented Nov 14, 2024

Closes #387

This relies on a lot of the default functions from zarr's Store so it can most likely be further optimized in the future.

@mpiannucci mpiannucci marked this pull request as ready for review November 15, 2024 14:17
@@ -626,35 +603,40 @@ def supports_listing(self) -> bool:
def supports_deletes(self) -> bool:
return self._store.supports_deletes

def list(self) -> AsyncGenerator[str, None]:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change from async generator to async iterator doesnt change anything on the rust side, its the same prototype

@@ -444,10 +420,6 @@ def async_ancestry(self) -> AsyncGenerator[SnapshotMetadata, None]:
"""
return self._store.async_ancestry()

async def empty(self) -> bool:
"""Check if the store is empty."""
return await self._store.empty()
Copy link
Contributor

Choose a reason for hiding this comment

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

what happened here? did this disappear from the abc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

If the types are the same, I'd keep it, this is probably a much faster impl

Copy link
Contributor Author

@mpiannucci mpiannucci Nov 15, 2024

Choose a reason for hiding this comment

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

Its not the same, takes prefix now. gunna take a stab at it

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 added a simple implmenetation that just uses the rust list_dir function. I think that is right, but there is probably a more optimized way to do it?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is great, it will get fast once we make list_dir fast. Thank you

icechunk-python/python/icechunk/__init__.py Outdated Show resolved Hide resolved
icechunk/src/zarr.rs Show resolved Hide resolved
@@ -735,7 +740,14 @@ impl Store {
Key::Chunk { node_path, coords } => {
let mut guard = self.repository.write().await;
let repository = guard.deref_mut();
Ok(repository.set_chunk_ref(node_path, coords, None).await?)
Copy link
Contributor

Choose a reason for hiding this comment

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

beautiful thank you

@mpiannucci mpiannucci merged commit e4ae00f into main Nov 15, 2024
3 checks passed
@mpiannucci mpiannucci deleted the zarr-3.0b2 branch November 15, 2024 18:21
dcherian added a commit that referenced this pull request Nov 15, 2024
* main:
  Sync with zarr python 3 beta 2 (#388)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sync with zarr python 3 beta 2
2 participants