timsaucer opened a new pull request, #24733:
URL: https://github.com/apache/datafusion/pull/24733
## Which issue does this PR close?
- Part of #19277. That issue asks for FFI versions of both `runtime_env` and
`execution_props`; this PR implements `runtime_env` only, so it does not
close
it.
- Fixes the downstream breakage reported in
[delta-io/delta-rs#4623](https://github.com/delta-io/delta-rs/issues/4623).
## Rationale for this change
A table provider shared over FFI is asked to `scan` by a session in another
library, and the `ExecutionPlan` it returns is later executed with a
`TaskContext` produced by that same session. Providers backed by remote
storage
build their own `ObjectStore` and register it on the session during `scan`,
then
read through it at execution time.
Both `ForeignSession::runtime_env` and the `FFI_TaskContext` conversion
built a
default `RuntimeEnv`. Planning and execution happen on opposite sides of the
boundary, so the registration was never visible where it was needed and the
scan
failed with:
```
No suitable object store found for s3://bucket/. See
`RuntimeEnv::register_object_store`.
```
Registering the store on the consumer's session instead did not help, because
the plan executing on the provider's side reconstructed its own default
environment.
The same gap meant foreign plans allocated from a fresh
`UnboundedMemoryPool`,
so `datafusion.execution.memory_limit` was silently ignored for them and
their
allocations were invisible to the session's accounting.
## What changes are included in this PR?
`RuntimeEnv` is a plain struct whose field list depends on enabled features
(`parquet_encryption` adds one), so passing an `Arc<RuntimeEnv>` as an opaque
pointer would be undefined behaviour between libraries built with different
feature sets. Each component crosses on its own terms instead:
| Component | How it crosses |
| --------------------- | --------------------------------------------- |
| `ObjectStoreRegistry` | Shared, via `FFI_ObjectStoreRegistry` |
| `MemoryPool` | Shared, via `FFI_MemoryPool` |
| `DiskManager` | Configuration copied; each side keeps its own |
| `CacheManager` | Configuration copied; each side keeps its own |
New in `datafusion_ffi::execution`:
- `FFI_RuntimeEnv` / `FFI_RuntimeConfig`
- `FFI_MemoryPool` / `ForeignMemoryPool` / `FFI_TryGrowResult`
- `FFI_ObjectStore` / `FFI_ObjectStoreRegistry` and their `Foreign*`
wrappers,
covering every required `ObjectStore` method plus the `get_ranges` and
`list_with_offset` overrides
A few details worth a reviewer's attention:
- **Local fast path.** A provider that registers its own store and then reads
through it does not pay for any of this. The store round trips through the
foreign registry but `FFI_ObjectStore::as_local` recovers the original
`Arc<dyn ObjectStore>`, so no data crosses the boundary. Making that work
needs a side table keyed on the wrapper's address, because `ObjectStore`
has
no `Any` supertrait and a `dyn ObjectStore` cannot be tested for a concrete
type. A one-line upstream change to `object_store` would replace it with a
downcast; I plan to propose that separately.
- **Error variants are preserved**, not flattened to a message. Optimistic
concurrency control built on `AlreadyExists` and conditional reads built on
`NotModified` / `Precondition` depend on the discriminant, and delta-style
commit protocols would silently lose their concurrency control otherwise.
`ResourcesExhausted` is preserved for the same reason: a dozen spilling
operators match on it to decide whether to spill rather than fail the
query.
- **Byte transfer is zero-copy** in both directions via `Bytes::from_owner`.
- **`GetResultPayload::File` is not forwarded.** A raw file descriptor is not
portable, so payloads always cross as a byte stream.
- **`Extensions` are dropped** on all options structs. They are
`TypeId`-keyed
and `TypeId` is not stable across separately compiled libraries, so the
contents cannot be interpreted on the far side even in principle.
## Are these changes tested?
Yes. 212 tests pass.
The important one is `test_object_store_crosses_ffi_boundary` in
`datafusion/ffi/tests/ffi_integration.rs`, which is a genuine cross-library
test: the table provider is compiled into the cdylib and loaded with
`libloading`, so the two sides have distinct library markers. It registers an
object store on the session during `scan` and reads it back during `execute`,
and also reports the memory pool limit it observes so the host can assert its
limit reached the foreign plan.
I verified that test actually catches the bug by reverting each fix
independently. Both are load bearing, and each reproduces the reported error:
```
No suitable object store found for ffitest://ffi-object-store/.
See `RuntimeEnv::register_object_store`.
```
Unit tests cover error-variant round trips for every `object_store::Error`
variant, `PutMode::Create` and `copy_if_not_exists` surfacing `AlreadyExists`
through the wrapper, memory limits being enforced and reservations released
across the boundary, and the local fast path returning the original store.
## Are there any user-facing changes?
Yes, including breaking changes to public APIs. Documented in the
[56.0.0 upgrade guide](docs/source/library-user-guide/upgrading/56.0.0.md):
- `FFI_TaskContext` gained a `runtime_env` field, changing its ABI. All FFI
providers and consumers must be rebuilt together.
- `impl From<Arc<TaskContext>> for FFI_TaskContext` is removed. It could not
supply a tokio runtime handle, producing a context whose object stores
could
not be polled from a foreign executor. Use `FFI_TaskContext::new`.
- `datafusion-ffi` now depends on `object_store` directly and exposes its
types,
so providers and consumers must agree on the `object_store` version too.
Behavioural changes worth calling out: plans shared over FFI are now bounded
by
the session's memory limit and may return `ResourcesExhausted` where they
previously did not, and `DiskManager` is not shared, so spilling can use up
to
twice `max_temp_directory_size` across the two sides.
Please add the `api change` label.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
