Add method on SessionContext to add all extensions from one library - #1679
Conversation
f152621 to
725fa74
Compare
ntjohnson1
left a comment
There was a problem hiding this comment.
I don't have a fully coherent thought here. IIUC this is mostly to manage life times across the FFI boundary. I do wonder if there is a slightly cleaner way to mange this but this seems fine for now to provide a safer avenue.
I wonder if it makes sense to have a todo for some datafusion python extension skill/s. Being able to generate the 3 library example from the skill might be a nice smoke test to verify. I suspect after getting things setup for ballista/datafusion-distributed keeping it up to date shouldn't be too bad but I do suspect it will require some guidance to make sure they are doing it safely.
milenkovicm
left a comment
There was a problem hiding this comment.
LGTM, thanks @timsaucer
i guess the only open point is chaining of codecs raised in previous pr
|
Following up on comment apache/datafusion-ballista#2252 (review) and follow up on this PR. I might be wrong but
As Basically we could implement query planner in python (limited but working) wdyt @timsaucer and @ntjohnson1 ? |
e95bc43 to
4f16fa6
Compare
|
Thanks for chasing this down — the Where it actually breaks. This PR already survives it on the datafusion-python side. Two fixes, both much cheaper than a new API:
On A It also buys very little over what already ships: plan_bytes = df.logical_plan().to_bytes(ctx)
batches = my_grpc_client.execute(plan_bytes) # your gRPC logic, in Python
result = ctx.from_arrow(batches) # any __arrow_c_stream__ objectAll three exist today, no new API. The only thing If the goal is partial delegation rather than whole-plan delegation, the design that answers it already exists: a table provider exported from Python ( The part that makes me most hesitant is the stated rationale: "As One genuinely open question, independent of all of the above — and I think it is the same "chaining of codecs" point you flagged. Once Ballista's codec is installed on the Python session it becomes a chain entry, and chain entries write a framed payload ( Proposal: keep #1679 as is, since the file-format gap predates it and is orthogonal, and I will open the upstream issue for (2). If you still want the callback route after the above, let us give it its own issue so the design can be argued on its own terms. |
Installing FFI extension codecs and query planners by chaining the existing with_* methods can bind task-context providers to intermediate contexts that are later collected, breaking the weak provider reference over the FFI boundary. with_extensions creates one destination context, passes it to each extension factory so components bind to that exact context, and installs everything in a single state write. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
MyPlannerExtension in the query-planner example crate implements the __datafusion_session_extension__ protocol from Rust: it extracts the destination context's task-context provider, binds fresh observing codecs and a planner to it, and returns SessionExtensionComponents. Its codecs record the max_rows config value resolved through the weak provider, letting tests prove the provider targets the returned context rather than the source. Documents with_extensions as the preferred API in the FFI guide. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A DataFrame does not keep its SessionContext alive. FFI components hold a weak task-context provider, so operations that reach an FFI codec after the context is collected fail with a clean out-of-scope error rather than crashing. Lock that behavior in with a test and document the ownership contract in the FFI guide and with_extensions docstring. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Single-underscore methods on internal pyo3 classes (such as SessionContext._install_extensions) are private support methods for the Python wrappers and do not require a public wrapper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A codec-only bundle installed on a context that already holds an FFI planner must rebind that planner to the new chains, so the planner decodes through the bundle's codecs. Codec ids are derived from the exporting class, so two bundles shipping the same codec class collide and the install is refused. Declaring __datafusion_codec_id__ on the object a bundle hands over resolves it, and both chains then install. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The docs build runs Sphinx with --fail-on-warning. SessionExtensionComponents documented its fields in both a napoleon `Attributes:` section and the dataclass class-body annotations, so autoapi emitted each field twice and the build failed with six "duplicate object description" warnings. Move each field's description to a per-field docstring under its annotation so autoapi renders exactly one entry per field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
QueryPlannerExportable, SessionExtensionComponents, and SessionExtensionExportable describe how an extension library plugs into a session, not how a SessionContext behaves. Give them their own module so context.py does not keep absorbing the extension surface as it grows. extensions.py imports SessionContext, the codec protocols, and CapsuleType under TYPE_CHECKING only, so context.py can import from it at runtime without a cycle. All three names remain importable from datafusion and datafusion.context; QueryPlannerExportable stays out of the top-level __all__ as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The example was marked `+SKIP` because the main suite has no built FFI extension to import, which is exactly how such an example rots. Parse the statements out of the live docstring in the query-planner example suite, drop the skip, and execute each one against a real extension bundle. Only names are redirected: `my_extension` resolves to a stand-in combining this repository's provider codecs and planner, and `SessionContext` supplies the config that planner reads. A renamed method, a changed signature, or a wrong expected output now fails CI, which already runs this suite. Also drop the `extensions` Args entry's restatement of the type hint and say instead what the hint does not: install order is chain order. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f8327c8 to
ceaf752
Compare
`_derive_for_extensions` minted a new `Arc<SessionContext>` via `new_with_state(self.ctx.state())`. Every other `with_*` method shares `Arc::clone(&self.ctx)`, and `new_with_state` carries the session id over, so `with_extensions` returned a second live session claiming the same `session_id()` as the source while holding independent `SessionState`. Configuration and the function registry diverged, catalogs stayed shared, and both handles reported the same `__datafusion_codec_id__` — which is `session:<session_id>` and exists precisely to distinguish codec chains, so installing both on a third session was refused as a duplicate id. The fork also bought nothing. It was introduced to keep components from binding to an intermediate context that could be collected, but there is one `Arc<SessionContext>` per session, so no such intermediate exists; deriving one is what creates the hazard. Rule 6 of the ffi-capsule-protocol skill already said to mutate `SessionState` in place rather than derive a replacement. Delete `_derive_for_extensions` and hand the receiver to the extension factories. `_install_extensions` already returned a handle sharing `Arc::clone(&slf.borrow().ctx)`, so removing the fork upstream of it is the whole change. Atomicity is unaffected: both codec chains are built as locals and state is written exactly once, at the end, in `set_session_query_planner`. Replace `test_with_extensions_provider_targets_returned_context`, which is vacuous once the session is shared, with `test_with_extensions_shares_the_session_with_the_source`. It asserts matching session ids and that a `SET` issued through the source after installation is visible to the provider the bundle bound. Reintroducing the fork fails it. Update the prose that described the fork-era design: the `with_extensions` docstring and `SessionExtensionComponents` / `SessionExtensionExportable` in `datafusion.extensions`, the `with_extensions` and "What a derived context shares" sections of the FFI guide, the query planner example's README and `extension.rs` comments, and two test docstrings. Note the shared-session mechanism in Rule 6 of the skill, since `with_extensions` is where it is easiest to get wrong. `enable_url_table` is once again the only method that mints a second `Arc<SessionContext>` for a session; its comment, the FFI guide, and the skill now also record that it forks state while keeping the session id, tracked as a bug in #1708. Also add the missing doctest to `SessionExtensionComponents` and a pointer to `with_extensions` from the upgrade guide, which described only the low-level install path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A codec handed to `with_extensions` as a bare `PyCapsule` fell through to `anon:<uuid4>`, an id private to the session that installed it. Plans written through it are undecodable anywhere else, and `with_extensions` accepts no `codec_id=` to override that — so the workaround was to wrap the capsule in an object declaring `__datafusion_codec_id__`, which nothing documented. A distributed engine has to decode its plans in another process, so the shape it would naturally ship — a Rust bundle handing over capsules, as `MyPlannerExtension` does — was the one shape that could not work. The bundle is the stable name that was missing. It is a plain Python object, so its `module.QualName` is library-owned and exactly as stable across processes as an exporting codec class's, which arm 3 of `derive_codec_id` already trusts. The capsule was unnameable only because a capsule carries no type of its own, not because nothing stable was in reach. Resolve a capsule's id through the contributing bundle, using `derive_codec_id` itself so the bundle inherits the same `__datafusion_codec_id__` escape hatch against a class rename. The fallback applies only where randomness would have: an id declared on the handed-over object, or that object's own class, still wins, so an extension can name a codec directly. Two bare capsules of one kind from one bundle collide and are refused. Numbering them by position would be exactly the id `codec.rs` rejects for `anon:` — one another library can mint the same value from — and would break stored plans the first time the bundle reordered what it returns. `resolve_codec_id` gains the bundle argument, `_install_extensions` takes (codec, bundle) pairs, and the collision message now names both routes to a distinct identity; it previously offered only `codec_id=`, which is unreachable from `with_extensions`. Covered in `python/tests/test_context.py`, which reaches every arm without a built extension library: the bundle-derived name, an extension pinning its own id, an id on the handed-over object winning, an exporting object keeping its own, and the two-capsule collision. The cross-FFI case is pinned in the query planner example, where a Rust bundle's capsules must report `datafusion_ffi_query_planner_example.MyPlannerExtension` and no id may be `anon:`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The doc comment said "the final state is written through this context's own `state_ref()`", which overstates it. `set_session_query_planner` returns early when there is no planner to bind, and the codec chains live on the returned `PySessionContext` fields rather than in `SessionState` — so a codec-only install onto a session with no FFI planner writes nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Mark `SessionExtensionExportable` `@runtime_checkable` and have `with_extensions` check it with `isinstance` rather than `hasattr`, so the annotation and the runtime check are the same statement, and callers can ask the question too. Covered by a doctest on the protocol. Replace the leading-underscore skip in `test_wrapper_coverage` with a named allowlist. The pattern also excused `DataFrame._repr_html_`, which a wrapper does have to provide, so a two-method need was weakening coverage for every private name. Removing `_install_extensions` from the allowlist fails the test, so the entry is load-bearing rather than decorative. Say in `_CodecOnlyExtension` that retaining the context is what the protocol tells real extensions not to do, and that it is kept only so a test can assert which context the factory was handed. Let the docstring-example shim in the query planner example accept a config positionally, the way the real constructor does. Editing the docstring to `SessionContext(config)` now fails as a doctest diff rather than as a `TypeError` inside the harness. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reverses "Name a bundle's bare capsules after the bundle". Deriving a capsule's id from the bundle that contributed it reads the identity off the wrong object: the bundle is whatever the caller passed to with_extensions, so an application that packages several libraries as one bundle of its own stamps its identity onto the inner libraries' codecs. Their pinned __datafusion_codec_id__ is discarded and there is nothing the inner library can do about it, since its object never reaches _install_extensions. Nothing fails at install time; the mismatch surfaces as an undecodable plan in the process that reads it, naming an id nobody wrote in source. So with_extensions now refuses a bare capsule and names the getter to implement. An id read off the handed-over object is composition-stable by construction, which the new tests pin at both layers. This also decouples a codec's wire identity from the bundle's Python class name, which is what __datafusion_codec_id__ exists for, and closes the case where a bundle built by a factory function contributed a wire id containing "<locals>". The low-level methods keep accepting capsules: they take codec_id=, so the random anon: arm still has an escape hatch. Query planners are unaffected, carrying no wire id. MyPlannerExtension gains BundledLogicalCodec and BundledPhysicalCodec, small pyclasses holding the bound FFI codec and declaring pinned ids, as the reference shape for a library whose plans leave the process. Also, unrelated to the above but adjacent in the docs: with_extensions never said that a bundle-supplied planner replaces an installed one rather than layering, and the SessionExtensionComponents example that showed a codec was fully skipped with undefined names. Both fixed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A session chains many codecs and dispatches between them by id, so codecs accumulate and their order does not affect decoding. A session holds exactly one query planner, so planners cannot accumulate — they compose by nesting, each wrapping the one before it. Collecting both from a single hook forced with_extensions to refuse more than one planner per call, because every factory ran before anything was installed and so no bundle could see another bundle's planner to wrap it. Two libraries that each ship a planner could not be installed together at all, and splitting them across two calls silently discarded the first. Codecs now come from __datafusion_session_extension__ and planners from a new __datafusion_session_planner__(ctx, fallback), which runs once per bundle in argument order after every codec is installed. Each receives the planner built so far; wrapping it nests this bundle outside the previous one, so the last bundle listed ends up outermost. A bundle implements either hook or both, which also lets a library that ships only an optimizing planner stop returning empty components. SessionExtensionComponents loses its query_planner field. Running the planner hooks after every codec is installed is what makes a nested planner safe. The rebuild that follows a later codec install reaches only the outermost layer, so a fallback captured against a partial chain would stay stale; there is now no "afterwards" within a call. Atomicity is unchanged. _install_extension_codecs writes nothing — the chains belong to the returned handle — so phase one is transactional for free, and the nest is built in memory with _install_extension_planner performing the single session write after the last hook returns. A hook that raises in either phase leaves the caller's context as it was. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bundle shipped a planner and codecs, but the halves never met: the planner emitted a stock GlobalLimitExec and the codecs delegated everything to the default codec. So the example asserted by structure that a planner and its codecs belong together without demonstrating why, and no test would have caught a bundle whose planner emits a node its own codec cannot encode. DistributedQueryPlanner now wraps its result in a DistributedExec, a type private to this library, and ObservingPhysicalExtensionCodec claims it by downcast and rebuilds it from its inputs. Nothing else in the session knows the type, which is the reason the two ship as one bundle. The observing codecs stop being dead weight in the process — they were previously never consulted, and decode_max_rows_seen had no caller. Also documents what codec order does and does not control, which building this surfaced. Decoding routes by id and is never order-dependent. Encoding stops at the first codec that claims the node, so a codec claiming a broad category — MyPhysicalExtensionCodec claims any ForeignExecutionPlan — takes nodes from any library installed after it. The query still succeeds; only the library that wrote the bytes changes, which breaks a plan that has to decode elsewhere. That gives a bundle two reasons to want different positions for its two halves. The guide now says to contribute each half at its own position with a small adapter rather than reordering, since the hooks are independent, and treats the low-level sequence as the last resort it is: it works, but it hands back responsibility for codec-before-planner ordering and leaves a hand-layered fallback holding the codecs it captured. No attempt is made to express every permutation from one call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The physical observer earns its place now that it claims the bundle's own DistributedExec, but the logical one never did and cannot: it declines all four methods to the default codec, and this library defines no logical extension node for it to claim. The FFI logical codec does not carry arbitrary LogicalPlan::Extension nodes anyway, so there is no logical analogue to give it. Measuring a query confirms it: every record_task_ctx firing comes from the physical decode path and none from the logical one. BundledLogicalCodec now wraps DefaultLogicalExtensionCodec directly, which keeps what the logical half actually demonstrated -- a bundle contributing both codec kinds under ids it declares -- and drops 54 lines of trait impl and hand-written Debug that fed an accessor nothing could observe. Also narrows PlannerObservations::used_fallback back to private. It is read only through MyQueryPlanner::used_fallback in the same module; its neighbours need pub(crate) because extension.rs reads them, and it does not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two loose ends from review. QueryPlannerExportable was the only member of the extension protocol family left in the submodule while SessionExtensionComponents, SessionExtensionExportable and SessionPlannerExportable were exported from the package root. It types the planner a __datafusion_session_planner__ hook returns, so a bundle author needs it just as much, and one family member importing differently from the rest is a papercut with no upside. Also fixes two doc references that stopped resolving when these classes moved out of context.py: a bare :class:`QueryPlannerExportable` and a bare :py:class:`SessionExtensionComponents`, both now spelled with their module the way the neighbouring datafusion.user_defined references are. with_extensions() with no arguments raised instead of installing nothing. That put it out of family: every sibling varargs method -- DataFrame.select, filter, sort, drop, window -- accepts zero arguments and returns a no-op result, and the two existing "at least one" guards in the codebase both cover cases with no meaningful identity element, which this is not. Installing no extensions has an obvious answer, and a caller assembling the list from a plugin registry should not have to special-case it being empty. Phase two still runs, so the empty case rebinds an existing FFI planner to unchanged chains; a test pins that the planner survives it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The planner-install section still said a bundle exposes __datafusion_session_extension__, which stopped being the whole protocol when planners moved to a hook of their own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`myst_heading_anchors` was 3, but the extension-bundles section added in this branch cross-references its own `####` subsections. Sphinx warns `'myst' cross-reference target not found: 'when-codec-order-does-matter'` and renders that link as plain text; the docs build does not pass `-W`, so it went unnoticed. Bumping to 4 rather than promoting the heading keeps the four subsections nested under `### Extension bundles: with_extensions`, where they belong. Only one other `####` heading exists under `docs/source/`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`with_extensions` ended every call with `_install_extension_planner`, which rebuilds `SessionState` to rebind an existing FFI planner to this handle's codec chains. When the call installed no codec there is nothing to rebind against, so the rebuild is at best churn — and at worst it drags a planner that is sitting on another handle's codecs onto this one's, silently undoing that install. `with_python_udf_inlining` already guards its no-op toggle for exactly this reason; `with_extensions` now guards the same way. `test_with_extensions_installing_nothing_leaves_the_planner_alone` covers both shapes of "installed nothing": no arguments at all, and a bundle whose hooks answer empty. Both fail without the guard, with the planner left on an empty logical chain and the query erroring out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`logical_extension_codecs=codec` instead of `(codec,)` is the easy mistake to make, and it surfaced as `'MyCodec' object is not iterable` raised by an `extend` call inside `with_extensions` — naming neither the field nor the hook that built the value. `__post_init__` now checks it, so the error lands in the extension library's own frame and says which field is wrong and how to spell one codec. It also normalizes each field to a tuple. The declared type is a tuple and the class is frozen, so a list left in place would be a mutable member of an immutable value, and a generator would be exhausted by the first read. A str is refused rather than normalized: it is iterable, so it would otherwise become a tuple of characters and fail much later as that many bogus codecs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`with_extensions` accepts a bundle implementing either hook — the runtime check tests against both protocols, `test_with_extensions_accepts_a_planner_only_extension` pins it, and the FFI guide's `PlannerOf` adapter recommends contributing only the planner half. The annotation named `SessionExtensionExportable` alone, so a type checker rejected the very shape the guide tells authors to write. Two doc comments also went stale. `test_with_extensions_no_extensions_keeps_an_installed_planner` still described phase two running and rebinding an existing planner, which the no-op guard now skips outright, and `__datafusion_codec_id__` listed a `_install_extensions` method that never existed under that name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `SessionExtensionComponents` doctest took its codec capsule off a `SessionContext()` that was dropped on the same line. An FFI codec holds its task-context provider weakly, so that capsule names a session that is already gone — the doctest only reads an id back so it passes, but it is the exact shape the FFI guide warns against. It now keeps the context in a name. `SessionPlannerExportable` called returning `fallback` a wrap that "contributes nothing". It is not: the capsule the first bundle receives wraps the session's planner for export, so handing it back installs it as a foreign planner and every later plan crosses an FFI boundary that was not there before. `None` is the no-op. Corrected in the protocol docstring, the FFI guide's canonical section, and the `_PlannerExtension` test helper that repeated the claim. `__post_init__` walked a written-out list of field names. It now walks `dataclasses.fields`, filtered on the `_codecs` suffix so a codec field added later is normalized without anyone remembering to name it, and a future field that is not a codec collection is left alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The method rebinds nothing. It imports whatever a `__datafusion_session_planner__` hook returned — an object exposing the getter or a raw capsule — and hands back a capsule, so the next hook receives one either way; its own doc comment already said "re-export". Meanwhile "rebind" means something specific and different in this file: rebuilding an installed planner against a handle's codec chains, which is what `set_session_query_planner` does. Freeing the word keeps the two apart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`__datafusion_session_planner__` was documented as receiving a context that carries the final codec chains, and `MyPlannerExtension` relies on exactly that when it takes the host's codecs off `ctx` instead of minting its own. The other side was never stated: `__datafusion_session_extension__` runs before anything is installed, so its `ctx` is the same session with the chains the receiver already had — missing this call's codecs, including the bundle's own. Both hooks hand back a valid task-context provider, which is what components actually need, so the difference only bites an author who reads codec chains off the context. Recorded on `SessionExtensionExportable`, in the FFI guide's two-phase section, and in Rule 2 of the capsule-protocol skill. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`test_with_extensions_threads_the_planner_through_in_order` never checked an order: it asserted each hook recorded one fallback and that the second was not None, both of which hold for a host that ran the hooks backwards. `test_with_extensions_skips_a_planner_hook_returning_none` asserted only that downstream ran, while its comment claimed the skipped hook had not become downstream's fallback. `_PlannerExtension` now takes an optional shared list the hooks append themselves to, so order is observable. The threading test asserts that list, plus that the second hook's fallback is not the object the first was handed -- the host re-exports every return value before passing it on. A capsule is opaque from Python, so that cannot separate a re-export of the first planner from a fresh read of the session's; the comment says so and points at the FFI suite's `test_with_extensions_nests_planners_in_argument_order`, which pins the nesting by asserting the outer planner delegated. The skip test records the skipped hook's fallback too, and asserts both hooks ran, that downstream was handed a different capsule, and that the resulting context still queries. Each new assertion was mutation-tested: reversing the planner loop, dropping the `if supplied is None: continue`, and replacing the re-export with a straight pass-through each fail exactly one of these tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`QueryPlannerExportable` said `session` is the `datafusion.context.SessionContext` the planner is being installed on. It is not. The capsule getters are called from Rust and receive the PyO3 context, so `isinstance(session, SessionContext)` is False -- while its repr reads `datafusion.SessionContext`, because the pyclass declares `module = "datafusion"`. It carries every capsule getter and `__datafusion_codec_id__`, which is all the protocol needs, so the fix is to say duck-type it rather than to change what is passed. The two bundle hooks are the exception and do receive the wrapper, since `with_extensions` dispatches them from Python; the ffi.md section on capsule getters now draws the same distinction. The `with_extensions` `Raises:` section listed ValueError for colliding codec ids only. A getter returning a capsule of the wrong kind also raises it -- `Expected name 'datafusion_query_planner' in PyCapsule, instead got 'datafusion_logical_extension_codec'` -- which `test_with_extensions_rejects_bad_codec_capsule` already pins. The `datafusion.extensions` module docstring said phase two runs the planner hook "once per bundle". Once per bundle that implements it; a bundle implements either hook or both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The user guide had no page for someone who installs an extension library and wants to run queries with it. `with_extensions` was described only in a 123-line docstring and in the contributor-guide FFI page, which opens by explaining that Rust has no stable ABI — the wrong altitude for a reader who just wants their queries to run somewhere else. Adds `user-guide/extensions.md`: what an extension library is, which kinds register directly versus needing `with_extensions`, the two failure modes that actually bite (a collected context, a version mismatch), and how to check what a session was taught. It names no capsule, no ABI, and no codec id; the only occurrences of `FFI` and `TaskContextProvider` are inside the error string a reader would be searching for. Splits `distributing-work.md` into a directory. The page was entirely about pickling expressions to worker pools and treated query-level distribution as two stubs at the end, so nothing on the site connected `with_extensions` to distribution at all — which is the road a data scientist is actually looking for. The index now asks who owns the partitioning decision and routes accordingly; `query-engines.md` carries the missing bridge and absorbs the two upstream work-in-progress sections. Wires `sphinx-reredirects`, which has been a declared dependency since #1578 without ever being enabled, so the old `distributing-work.html` URL keeps working. Also fixes three pointers that went stale in the MyST migration and still named `.rst` files, a malformed `ref:` role in udf-and-udfa.md that rendered as literal text, and a doubled "the" in data-sources.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`contributor-guide/ffi.md` had grown to 795 lines serving three different
readers at once, filed under a section whose index says it is for people
contributing to this repository. An engineer at delta-rs or a distributed-engine
vendor is neither a contributor nor an end user; they consume a published,
versioned protocol, and the only description of it lived behind a heading
telling them the page was not for them.
Adds a third top-level section, `extension-guide/`, so the sidebar reads User
Guide / Extension Guide / Contributor Guide — one per audience. It carries the
`(ffi)=` label, so every existing reference keeps resolving, including the
`:ref:`ffi`` inside `context.py`'s docstring that ships in the wheel.
The maintainer-facing rationale moves to `contributor-guide/ffi-internals.md`:
the weak-`Arc` scheme, why repairing an orphaned provider cannot work, why
planner codec rebinding is one level deep, and the two upstream issues. An
extension vendor should not be reading "that is a bug rather than a design, do
not copy the pattern" halfway down their integration guide.
The PyO3 `frozen` policy moves to `contributor-guide/pyo3-guidelines.md`. It is
project review policy with nothing to do with FFI, and extracting it repairs a
prose bug: it had been spliced into the middle of "Implementation Details", so
the sentence "If you were interfacing with a library that provided the above
`FFI_TableProvider`" resumed 46 lines after the snippet it referred to. Those
two halves are rejoined on `capsule-protocol.md`.
Fills the coverage gap the split exposed. The codebase exports 18 capsule
getters and the old page documented 7; the remaining 11 appeared on no page
that even listed them. The section index now carries a table of all 18, and
`table-providers.md` and `functions.md` document the catalog family, table
functions, physical optimizer rules, and extension options for the first time.
Corrects the argument rule while moving it. The old section asserted that
capsule getters "receive the SessionContext they are being installed on", which
is true for the codec, planner, and table-function hooks but not for the
catalog family: `CapsuleGetterArg::LogicalCodec` passes the host's logical
codec as a bare capsule, and `__datafusion_table_provider__` gets a session
from `SessionContext.register_table` but a codec capsule from
`Schema.register_table`. Nothing breaks, because every implementation passes
the argument to `ffi_logical_codec_from_pycapsule`, which handles both — so the
rule is now stated by capability rather than by type, and the upgrade guide
says which hooks changed.
Also converts the guide's five in-page heading links to labelled `{ref}`
targets, since heading anchors rot silently on rewording, and redirects the old
`contributor-guide/ffi.html` URL.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The extension docstrings and the FFI guide had grown the same six claims 3-5 times each, in wording that had already started to drift. `with_extensions` was 123 lines — the longest docstring in the package by 30 — and most of it argued a design rather than stating a contract. Routes each recurring claim to one home and leaves a one-line pointer elsewhere. Session sharing and context lifetime move onto the `SessionContext` class docstring, since they are properties of the type and that is exactly why five methods had each reworded them. The `None`-vs-`fallback` contract moves onto `SessionPlannerExportable`, since it is a return-value contract of one method. The two-phase rationale, the objects-not-capsules argument, and the codec-order argument stay in the guide, which owns the "why". The duck-type-the-session rule moves from `QueryPlannerExportable` to `LogicalExtensionCodecExportable`, which is already the designated `session` reference for that family and where the codec protocols were pointing for it anyway. It also gains the fact the old text was missing: across the protocol this argument is not always a session, so duck-typing it is not a style preference. Not every docstring shrank. `__datafusion_query_planner__` was 4 lines with no example while `set_query_planner` told callers to capture the fallback through it, so it grew to 25. Several others grew by gaining the `Args`/`Returns`/ `Raises` sections they were missing, and by trading `+SKIP` examples for runnable ones — a `SessionContext` satisfies the capsule-getter protocols, so its own exported capsule stands in for a library's without a build step. The total across these sixteen docstrings is roughly flat, at 577 lines before and 629 after; what changed is that the rationale left and the contract arrived. The 21 lines of `dataclasses.fields` rationale on `SessionExtensionComponents.__post_init__` become comments in the method body. That is the fix rather than touching `autoapi_options`: `__post_init__` is a *special* member, so dropping `private-members` would not hide it, and dropping `special-members` would delete the entire `__datafusion_*__` reference surface. `conf.py` now records why that setting is deliberately left alone, and de-duplicates the four re-exported `extensions` classes the way it already did for `DataFrame` and `SessionContext`. Adds `python/tests/test_docstrings.py`, which is what would have caught the 123-liner: a 95-line ceiling with no waiver list, a doctest-presence check over the extension protocol, and a check that a docstring naming the guide actually links it. The last two each found a real defect on first run. Also fixes what this branch made newly load-bearing in the serialization surface: `plan.py`'s single-backtick RST rendered `LogicalExtensionCodec` and friends as italics rather than links into the new API, and the `global_ctx()` fallback in `Expr.from_bytes` now silently yields a context with no extension codecs, which before this branch lost only registrations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four links in the two example READMEs pointed into `docs/source/.../ffi.md` with heading anchors. Sphinx does not link-check those, so deleting the page would have left them silently 404ing. They now use published URLs, which also fixes a second problem: a relative path into `docs/source` only renders on github.com and is broken for anyone reading the README from crates.io, an sdist, or a vendored copy. `grep -rn "docs/source" examples/` is now empty. `.ai/skills/ffi-capsule-protocol/SKILL.md` named the deleted page as "where the truth is", and AGENTS.md sends agents to that skill before they touch a capsule getter. It now points at the specific extension-guide pages and separately at `ffi-internals.md`. `llms.txt` filed the whole subject under "Optional" as "extending the Python bindings" — the wrong shelf for the headline feature of a major release, since that section means "skippable on a tight context budget". It gains an Extensions and distribution section, `datafusion.extensions` and `datafusion.ipc` in the API list, both FFI example crates, and the corrected `distributing-work` URL. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six findings from a read-through of the new extension guide, plus the API rename one of them turned into. The "process local tokens" note sat in the guide index with nothing around it to explain what a token was or why the reader should care. It moves to a new `extension_codec_durable_metadata` section in `codecs.md`, where the reader is already thinking about what goes in a payload: what to encode, then what the examples do instead, and the three consequences that follow from parking live objects in a process-local map — no double decode, no fan-out, and a leak for any plan that never reaches a decoder. The checklist item now points there instead of at the guide index. The hook reference loses its `Capsule name` column, which restated `datafusion_<thing>` for every row when the naming rule already derives it, and gains a `Contributes` column instead. The argument column stays: those four values are protocol, not a signature, and only 4 of the 18 hooks have a Python definition to link at all — the rest are host-side imports, so links into Rust source would rot faster than the table. Staleness is handled by `test_hook_reference_table_lists_every_hook` instead, which greps `crates/` and `python/datafusion/` for `__datafusion_*__` and diffs the set against the table rows. Verified it fails when a row is dropped. `capsule-protocol.md` described `abi_stable`, which datafusion-ffi no longer uses. It now describes `stabby` and the part that is not stabby: `FFI_Option` and `FFI_Result` are datafusion-ffi's own, because stabby's require `T: IStable` and the `FFI_*` structs hold self-referential function pointers. The conversion example converts to `Arc<dyn TableProvider>` rather than naming `ForeignTableProvider`, since the `From` impl compares library markers and returns the original `Arc` when both sides are the same library. Three other snippets on that page had gone stale with it: `FFI_TableProvider::new` with three arguments, `PyCapsule::new_bound`, and a receiving snippet whose variable was named `codec`. In `table-providers.md` all five `Registered with` cells now render `Receiver.method`, so the schema row reads `Catalog.register_schema` rather than a bare dotted path, with one sentence on reaching a `Catalog` first. `Other session components` was in `functions.md`, where an optimizer rule and a config struct are neither functions nor tables; it becomes its own page, `other-components.md`, carrying the `extension_other_hooks` label so the index rows still resolve. Three guide pages named individual tests, which invites exactly the divergence the reference is supposed to prevent. They name the suite now. Finally the phase-one bundle hook. `__datafusion_session_extension__` reused the name of the whole thing it is a hook on — `with_extensions` takes extensions and `SessionExtensionExportable` is the bundle protocol — while its sibling `__datafusion_session_planner__` is named for its content. It is now `__datafusion_session_components__`, matching both its sibling and the `SessionExtensionComponents` it returns, which leaves room for the UDF and provider fields that will join the codec fields later. The protocol class follows it to `SessionComponentsExportable`, since that file's convention is one class per hook name. Neither name has shipped, so the upgrade guide needs no before-and-after; it introduces both hooks as new in this release and names the new one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`test_hook_reference_table_lists_every_hook` scanned every byte of `crates/**.rs` and `python/datafusion/**.py` for `__datafusion_*__`, so comments and docstrings counted as evidence a hook exists. A doc-comment contrasting a hook with one that was removed, or naming a hypothetical, would have had to be deleted or added to the guide's table, and neither is right. Count sites instead. On the Rust side a site is a string literal holding nothing but the hook name -- what `hasattr`, `getattr`, and `call_capsule_getter` are handed -- or a `fn` of that name, which is a hook the host implements itself. Error strings that merely embed a name no longer count; each already sits beside a real lookup. On the Python side, read the syntax tree rather than the text: a method being defined, an attribute being accessed, or a string standing alone. A docstring is one string node holding the whole docstring, so prose drops out without a rule of its own. The dispatched set is unchanged at 18, still matching the table exactly. Verified in both directions: a comment naming a removed hook now passes, while adding a real lookup for an undocumented name still fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Ok, I did a lot of cleanup and I think this solution is far better than the original version. The big line number diff in the PR is because I revamped the documentation site to have 3 different audiences: datafusion-python contributors, users of the package, and people writing extensions. That split generated a lot of new documentation, which I think is a good thing. |
Three maturin builds and three test invocations for #1719, plus the documentation change that keeping three example trees requires. The plan had been to retire `datafusion-ffi-query-planner-example` and fold it into the new engine. That is now off, on evidence from building the engine: roughly fifteen of its forty-seven tests cover planner *layering*, and `dfx_engine`'s planner structurally cannot delegate to a `fallback`. Delegating hands physical planning back to the host, which returns opaque `ForeignExecutionPlan` nodes the engine can neither serialize nor split — a stage-splitting planner has to plan for itself. So the new example has nothing for those tests to nest, and deleting the crate would delete real coverage of the most subtle part of #1679's contract. Three trees then, with distinct jobs, which the guide now states up front rather than leaving a reader to infer: `examples/distributed` is the worked example and the place to start; `datafusion-ffi-example` is the capsule-protocol test bed, one of every hook exercised hard; and `datafusion-ffi-query-planner-example` is the planner-composition test bed. The guide's "three roles in a query" section described only the latter two. Two stale claims fixed while in there. `examples/README.md` linked three `sql-on-*.py` files that do not exist. The planner example's README said its planner "owns no serializable types of its own and deliberately uses only built-in physical nodes", which stopped being true when `DistributedExec` was added — and the sentence mattered, because owning a node is exactly why that library ships its codec and planner as one bundle. The `actionlint` pre-commit hook needs Docker and could not run here; the workflow files are otherwise lint-clean and parse as YAML. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Which issue does this PR close?
Part 3 of 3 in the split of #1672. These are enabled as a github stack so you should be able to swap between the 3 PRs in the github interface (above, next to the "Open" oval).
Rationale for this change
Working on the Ballista integration showed that chaining the low-level
with_*methods is easy to get wrong. FFI codecs and planners carry a weak task-context provider, and a query planner is built against whatever codec chains exist at the moment it is installed — so installing a codec afterwards leaves the planner encoding through a chain that is missing a library, and the caller is responsible for an ordering rule nothing enforces.SessionContext.with_extensionsmakes the whole installation one step so there is no "afterwards".What changes are included in this PR?
SessionContext.with_extensions(*extensions)installs one or more extension bundles in a single transaction. A bundle is a reusable configuration object — typically shipped by a compiled extension library — that implements one or both of two new protocol hooks. Nothing is written to the session until every hook has returned and every capsule has been validated, so a failing bundle leaves the session as it was.Codecs and planners install in two phases, because they compose differently. A session chains many codecs and dispatches between them by id, so codecs accumulate and their order does not affect decoding. A session holds exactly one query planner, so planners cannot accumulate — they compose by nesting, each wrapping the one before it. Phase one calls every bundle's
__datafusion_session_components__(ctx), which returns its codecs as aSessionExtensionComponents, and installs all of them. Phase two then calls every bundle's__datafusion_session_planner__(ctx, fallback), in argument order, handing each the planner built so far; wrappingfallbacknests that bundle outside the previous one, so the last bundle listed ends up outermost. A bundle implements whichever hooks apply, so a library shipping only an optimizing planner does not have to return empty components.Running the planner hooks after every codec is installed is the point of the split. The rebuild that follows a later codec install reaches only the outermost planner layer (upstream apache/datafusion#24762), so a fallback captured against a partial chain would stay stale forever. Within a
with_extensionscall there is no "afterwards" for any layer. This is also what lets several planner-shipping libraries be installed together at all: collecting planners from one hook meant every factory ran before anything was installed, so no bundle could see another's planner to wrap it.Codecs must be handed over as objects exposing the capsule getter, never as bare
PyCapsuleobjects. A codec's wire id is read off the object it arrives as, and a capsule has no type to read one from;with_extensionstakes nocodec_id=, so the capsule is refused with a message naming the getter to implement. Deriving the id from the contributing bundle instead is not a fix — the bundle is whatever object the caller passed, so an application that packages a library inside a bundle of its own would silently re-tag that library's payloads and they would stop decoding in the process that reads them. The inner library cannot defend against that no matter what it declares, and the mismatch does not surface until a decode fails elsewhere. Wrapping keeps the identity with the codec, which is what__datafusion_codec_id__is for.Ownership and lifetime. Like the individual
with_*methods, the returned context is a handle on the same session as the receiver: catalogs, tables, registered functions, and configuration are the one session, and the planner is installed on that shared session even if the returned handle is discarded. Only the Python-side codec chains belong to the returned handle. There is oneArc<SessionContext>per session, so every weak provider a bundle creates stays valid for as long as any handle on that session is alive. The context-outlives-DataFramecontract is documented and tested: aDataFrameoutliving every handle on its session fails with a clean out-of-scope error rather than crashing.Example library.
MyPlannerExtensionindatafusion-ffi-query-planner-exampleis a complete Rust implementation of both hooks, including taking the host's task-context provider off the supplied context and wrapping its codecs inBundledLogicalCodec/BundledPhysicalCodecso they carry declared ids. Its planner emits aDistributedExec— an execution plan node private to that library — and its physical codec claims that node by downcast and rebuilds it on decode. That pairing is the normal shape for a bundle: a planner that emits its own executor is only useful alongside the codec that can serialize it, which is why the two ship together and why every codec is installed before any planner is bound.Documentation is reorganised by audience.
docs/source/contributor-guide/ffi.mdwas one 545-line page serving three different readers at once, andwith_extensionshad nowhere to go in it. It is replaced bydocs/source/extension-guide/, eleven pages for people writing an extension library: an index carrying a reference table of every__datafusion_*__hook, thenwhy-ffi,capsule-protocol,table-providers,functions,codecs,bundles,query-planners,other-components,sessions, and an authorchecklist. The framing that only matters when changing datafusion-python itself moved tocontributor-guide/ffi-internals.md, and a newuser-guide/extensions.mdcovers using a library someone else published. Separately,user-guide/distributing-work.mdsplits intodistributing-work/{index,expressions,query-engines}.md, since choosing a strategy and picklingExprto a worker pool are different questions.conf.pygainssphinx-reredirectsstubs so the two old URLs keep resolving.extension-guide/bundles.mdis wherewith_extensionsis documented: why the phases are split, a runnable pure-Python bundle, the codecs-are-objects rule, and what codec order does and does not control — decoding routes by id and is never order-dependent, while encoding stops at the first codec that claims a node, so a codec claiming a broad category can take nodes from a library installed after it. The query still succeeds; only the library that wrote the bytes changes, which breaks a plan that has to decode elsewhere. Where a bundle needs one position for its codec and another for its planner, the guide shows contributing each half separately with a small adapter rather than reordering, and treats the low-level sequence as the last resort it is.docs/source/user-guide/upgrade-guides.mdpoints atwith_extensionsfrom the planner-install section. Rule 2 of.ai/skills/ffi-capsule-protocol/SKILL.mddocuments the planner hook's extra argument and the codecs-are-objects rule; Rule 6 calls outwith_extensionsas where "a session keeps oneArc<SessionContext>for life" is easiest to get wrong, since "bind the components to the context you are about to return" reads like an instruction to derive one first.The docstring conventions that split enforces are now testable.
AGENTS.mdgains "One canonical home per claim" — a claim about one callable lives on that callable, a claim about a type lives on the class, and why the API is shaped this way lives in the guide — and "Examples that need a compiled extension", which asks every# doctest: +SKIPblock to be preceded by a runnable one and mirrored by a test that runs it for real.python/tests/test_docstrings.pychecks the shape rather than the content: no docstring exceeds a length ceiling, every extension-protocol member carries at least one example, no docstring sends the reader to "the guide" without a resolvable Sphinx role, and the hook reference table matches the hooks the package actually dispatches.Are there any user-facing changes?
New public APIs:
SessionContext.with_extensions,SessionExtensionComponents, and theSessionComponentsExportable/SessionPlannerExportableprotocols with their__datafusion_session_components__and__datafusion_session_planner__hooks. These live in a newdatafusion.extensionsmodule, and all four names are re-exported from the package root.QueryPlannerExportablemoved fromdatafusion.contexttodatafusion.extensionsand is now exported from the root alongside the rest of the family. The context-outlives-DataFrameownership contract is now documented.Two published documentation URLs move:
contributor-guide/ffi.htmlbecomesextension-guide/index.html, anduser-guide/distributing-work.htmlbecomesuser-guide/distributing-work/index.html. Both old URLs get a redirect stub, so inbound links keep working — though a link that pinned a heading anchor on the old single page lands at the top of the new one.No breaking changes to existing APIs. Both new hooks are new in this PR, so no shipped extension library implements them yet and no upgrade-guide migration entry is needed for them.
Review notes
Several things changed during review and the earlier revisions read differently. Recording them here so the discussion above stays legible.
The installation no longer derives a separate context. It used to —
_derive_for_extensions, usingSessionContext::new_with_state(self.ctx.state())— and bound the factories against that. Becausenew_with_statecarries the session id over while minting a freshArc<RwLock<SessionState>>, that produced two live sessions reporting onesession_id()with independent state: configuration and the function registry diverged, catalogs stayed shared, and both handles reported the same__datafusion_codec_id__, which issession:<session_id>and exists precisely to tell codec chains apart. The fork also bought nothing. It was meant to stop components binding to an intermediate context that could later be collected, but with oneArc<SessionContext>per session there is no such intermediate — deriving one is what creates the hazard, which is why Rule 6 of the capsule-protocol skill already said to mutateSessionStatein place rather than derive a replacement.test_with_extensions_shares_the_session_with_the_sourceasserts matching session ids and that aSETissued through the source after installation is visible to the provider the bundle bound; reintroducing the fork fails it.Bare capsules were briefly named after the contributing bundle, and are now refused instead. Naming them looked like it closed the gap that made the natural shape for a distributed engine — a Rust bundle handing over capsules — the one shape that could not produce portable plans. It did not: because the id came from whichever object the caller passed to
with_extensions, wrapping one bundle inside another silently re-tagged the inner library's codecs, which is the ordinary way an application presents several libraries as one. Requiring an object instead puts the identity on the codec, where composition cannot move it, and deletes the resolution arm rather than documenting a footgun.test_with_extensions_codec_ids_survive_compositionpins it.with_extensionsaccepted at most one planner per call before the two-phase split. That was the right refusal for a single-hook protocol — every factory ran before anything was installed, so two bundles that both captured the session's planner would both capture the default and one would silently win — but it meant two libraries that each ship a planner could not be installed together, and splitting them across two calls discarded the first with no diagnostic. The planner hook removes the refusal by having the host thread each planner into the next rather than letting bundles capture one.The phase-one hook was renamed. It was
__datafusion_session_extension__, with a matchingSessionExtensionExportableprotocol, which reused the name of the whole thing it is a hook on —with_extensionstakes extensions, and the bundle protocol is the extension. Its sibling__datafusion_session_planner__is named for its content instead. It is now__datafusion_session_components__, matching both that sibling and theSessionExtensionComponentsit returns, and leaving room for the UDF and provider fields that will join the codec fields later. The protocol class follows it toSessionComponentsExportable. Neither spelling has shipped.with_extensions()with no arguments no longer raises. Every sibling varargs method —DataFrame.select,filter,sort,drop,window— accepts zero arguments and returns a no-op result, and the two existing "at least one" guards in the codebase both cover cases with no meaningful identity element. Installing no extensions has an obvious answer, and a caller assembling the list from a plugin registry should not have to special-case it being empty.Related follow-ups, neither of which this PR needs.
enable_url_tableis now once again the only method that mints a secondArc<SessionContext>for a session, and it forks state while keeping the session id (#1708). An agent skill for extension authors, as suggested in review, is #1707.