You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@sedona.apache.org by "james-willis (via GitHub)" <gi...@apache.org> on 2026/09/18 19:06:32 UTC
[PR] feat(rust/sedona-raster-zarr): keep store clients and opened arrays across load calls [sedona-db]
james-willis opened a new pull request, #1321:
URL: https://github.com/apache/sedona-db/pull/1321
First of the loader-cache PRs from #1224: the two costs that live *inside* the Zarr loader and are paid on every `load` call. Chunk bytes are not cached here; that is the next PR, on the host side.
## What changed
**`sedona-raster-zarr`** (first commit)
`ZarrLoader::load` used to build a fresh `object_store` client for every distinct store and re-open every distinct array, a metadata round trip each, on every call. Since #1252 slices `RS_EnsureLoaded` input by bytes, a query makes more `load` calls than before, and a scan pays these costs on every slice even though it touches each chunk exactly once.
The loader now keeps two handle caches for the life of the registered instance, and never chunk bytes:
- the `ObjectStore` client per scheme and authority (`store_client_key`), so every group in one bucket or on one host shares a client and its connection pool;
- the opened `Array` per (store URI, array path), LRU-bounded at 256 entries with a 5-minute TTL as the only guard against an array being rewritten under a running session.
`ZarrLoader` stops being `Copy`; clones share the handles. `with_array_handle_capacity` and `with_array_handle_ttl` configure a fresh cache before the loader is shared, and `handle_stats()` exposes the reuse counters the tests assert on. Locks are held only around map access, never across an await; two calls racing to open the same array both open it and the later insert wins.
The store map is deliberately a stopgap: once host object stores can be handed across the loader boundary (the direction of #890), the host's registry supplies clients with credentials and pools and this map goes away.
**`sedonadb-zarr`** (second commit)
The extension's private tokio runtime was a current-thread runtime. `ZarrLoader::load` is driven through it by one blocking thread per DataFusion partition at once. On a current-thread runtime every caller's I/O readiness is delivered by whichever caller currently owns the driver, so a CPU-bound chunk decode on that thread stalled the other partitions' fetch completions. It is now a two-worker multi-thread runtime; each `block_on` drives only its own future.
## Measurements
`cargo bench -p sedona-raster-zarr --bench zarr_loader`, one `load` of 64 chunks across 4 arrays of one group on local disk, release build:
| loader | per call |
|---|---|
| handle cache (default) | 798 µs |
| re-open every call (`with_array_handle_ttl(Duration::ZERO)`) | 949 µs |
On local disk an array open is about 38 µs, so the cache saves ~150 µs per call here. On object storage each open is a network round trip, which is the case this is for; the bench pins the mechanism rather than the S3 gain.
## Tests
- `zarr_loader_reuses_array_handles_across_calls`: second call reuses both arrays, one store client built, bytes identical, a clone shares the handles.
- `zarr_loader_reopens_arrays_once_the_handle_ttl_expires`: zero TTL re-opens arrays but keeps the client.
- `zarr_loader_bounds_the_number_of_kept_array_handles`: capacity one with two arrays per call never reuses; zero clamps to one.
- `store_client_key_is_scheme_and_authority`: path-insensitive, host-lowercased keys.
The pre-existing rustdoc error for the `ObjectStoreRegistry::get_store` link in `source_uri.rs` is untouched.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust/sedona-raster-zarr): keep store clients and opened arrays across load calls [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #1321:
URL: https://github.com/apache/sedona-db/pull/1321#discussion_r4067874647
##########
rust/sedona-raster-zarr/src/raster_loader.rs:
##########
@@ -67,16 +78,34 @@ pub const ZARR_FORMAT: &str = "zarr";
/// fan-out, not the process-wide one.
pub const DEFAULT_LOAD_CONCURRENCY: usize = 8;
+/// Default number of opened arrays a loader keeps across `load` calls.
+pub const DEFAULT_ARRAY_HANDLE_CAPACITY: usize = 256;
+
+/// Default time an opened array stays usable. A `load` that needs the
+/// array after this re-reads its metadata, which is the only guard
+/// against an array rewritten or removed under a running session: with
+/// a live handle, a chunk whose key has been deleted comes back as the
+/// array's fill value rather than as an open error, until the TTL lapses.
+pub const DEFAULT_ARRAY_HANDLE_TTL: Duration = Duration::from_secs(300);
+
/// Async raster byte loader for Zarr-backed bands.
///
-/// Stateless across calls: each call builds an `ObjectStore` per distinct
-/// store URI (via [`object_store_for_uri`]) and opens each distinct array
-/// over async storage. Building the store here is the in-process bridge
-/// until a credentialed store can be passed in from DataFusion's
-/// `ObjectStoreRegistry`; see the loader-FFI follow-up.
-#[derive(Debug, Clone, Copy)]
+/// Within one `load` call the batch is reduced to its distinct stores,
+/// arrays and chunks (`LoadPlan`). Across calls the loader keeps two
+/// kinds of handle, never chunk bytes: the `ObjectStore` client per
+/// scheme and authority (a connection pool; see `store_client_key`) and
+/// the opened [`Array`] per `(store URI, array path)`, whose open is a
+/// metadata round trip. Clones share the same handles.
+///
+/// Building the store client here is the in-process bridge until a
+/// credentialed store can be passed in from DataFusion's
+/// `ObjectStoreRegistry`; see the loader-FFI follow-up. Because the client
+/// is kept, the environment (credentials, endpoint, region) is read once
+/// per scheme and authority for the life of the loader, not on every call.
+#[derive(Debug, Clone)]
pub struct ZarrLoader {
concurrency: usize,
+ handles: Arc<HandleCache>,
Review Comment:
If I'm reading this correctly, it's session scoped and not statically scoped, which is great!
##########
python/sedonadb-zarr/src/lib.rs:
##########
@@ -38,14 +38,27 @@ use tokio::runtime::{Builder, Runtime};
/// Process-wide tokio runtime backing the sync Python FFI bridge.
///
-/// `ZarrChunkReader::try_new` is async, but the Python constructor is sync
-/// by contract, so we `block_on` here. Building a runtime per reader is
-/// wasteful; one shared runtime serves every open in the package. `next()`
-/// on the returned reader is pure CPU and never touches this runtime.
+/// `ZarrChunkReader::try_new` and `ZarrLoader::load` are async, but the
+/// Python entry points are sync by contract, so we `block_on` here. One
+/// shared runtime serves every open and every load in the package;
+/// `next()` on a reader is pure CPU and never touches it.
+///
+/// Multi-thread rather than current-thread: `load` is called from one
+/// blocking thread per DataFusion partition at once, and on a
+/// current-thread runtime every caller's I/O readiness is delivered by
+/// whichever caller happens to own the driver, so a CPU-bound chunk decode
+/// on that thread stalls the others' fetches. With workers owning the
+/// driver, each `block_on` only drives its own future.
+///
+/// Like any tokio runtime held in a Python process, the workers do not
+/// survive `fork()`: a forked child that inherits an initialized runtime
+/// must not use this module. Spawn-start multiprocessing is unaffected.
Review Comment:
I believe there's something in either sedonadb python or datafusion python (from whence that came) that detects the forked case and ensures an error occurs (and not a crash or panic).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust/sedona-raster-zarr): keep store clients and opened arrays across load calls [sedona-db]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #1321:
URL: https://github.com/apache/sedona-db/pull/1321#discussion_r4074114894
##########
python/sedonadb-zarr/src/lib.rs:
##########
@@ -38,14 +38,27 @@ use tokio::runtime::{Builder, Runtime};
/// Process-wide tokio runtime backing the sync Python FFI bridge.
///
-/// `ZarrChunkReader::try_new` is async, but the Python constructor is sync
-/// by contract, so we `block_on` here. Building a runtime per reader is
-/// wasteful; one shared runtime serves every open in the package. `next()`
-/// on the returned reader is pure CPU and never touches this runtime.
+/// `ZarrChunkReader::try_new` and `ZarrLoader::load` are async, but the
+/// Python entry points are sync by contract, so we `block_on` here. One
+/// shared runtime serves every open and every load in the package;
+/// `next()` on a reader is pure CPU and never touches it.
+///
+/// Multi-thread rather than current-thread: `load` is called from one
+/// blocking thread per DataFusion partition at once, and on a
+/// current-thread runtime every caller's I/O readiness is delivered by
+/// whichever caller happens to own the driver, so a CPU-bound chunk decode
+/// on that thread stalls the others' fetches. With workers owning the
+/// driver, each `block_on` only drives its own future.
+///
+/// Like any tokio runtime held in a Python process, the workers do not
+/// survive `fork()`: a forked child that inherits an initialized runtime
+/// must not use this module. Spawn-start multiprocessing is unaffected.
Review Comment:
The AI agent wasn't able to find such a feature in a brief search
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust/sedona-raster-zarr): keep store clients and opened arrays across load calls [sedona-db]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis merged PR #1321:
URL: https://github.com/apache/sedona-db/pull/1321
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust/sedona-raster-zarr): keep store clients and opened arrays across load calls [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #1321:
URL: https://github.com/apache/sedona-db/pull/1321#discussion_r4075531244
##########
python/sedonadb-zarr/src/lib.rs:
##########
@@ -38,14 +38,27 @@ use tokio::runtime::{Builder, Runtime};
/// Process-wide tokio runtime backing the sync Python FFI bridge.
///
-/// `ZarrChunkReader::try_new` is async, but the Python constructor is sync
-/// by contract, so we `block_on` here. Building a runtime per reader is
-/// wasteful; one shared runtime serves every open in the package. `next()`
-/// on the returned reader is pure CPU and never touches this runtime.
+/// `ZarrChunkReader::try_new` and `ZarrLoader::load` are async, but the
+/// Python entry points are sync by contract, so we `block_on` here. One
+/// shared runtime serves every open and every load in the package;
+/// `next()` on a reader is pure CPU and never touches it.
+///
+/// Multi-thread rather than current-thread: `load` is called from one
+/// blocking thread per DataFusion partition at once, and on a
+/// current-thread runtime every caller's I/O readiness is delivered by
+/// whichever caller happens to own the driver, so a CPU-bound chunk decode
+/// on that thread stalls the others' fetches. With workers owning the
+/// driver, each `block_on` only drives its own future.
+///
+/// Like any tokio runtime held in a Python process, the workers do not
+/// survive `fork()`: a forked child that inherits an initialized runtime
+/// must not use this module. Spawn-start multiprocessing is unaffected.
Review Comment:
Ah, it's delta-rs: https://github.com/delta-io/delta-rs/blob/87010461cfe01563d91a4b9cd6fa468e2ad5f283/python/src/utils.rs#L10-L31
datafusion-python just links there and we don't handle that either.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org