Skip to content

Commit f7c448e

Browse files
timsaucerclaude
andcommitted
Correct three claims in the extension bundle docs
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>
1 parent 0868f39 commit f7c448e

3 files changed

Lines changed: 39 additions & 8 deletions

File tree

‎docs/source/contributor-guide/ffi.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -412,6 +412,11 @@ library that must be the only planner does it deliberately — but it is not com
412412
and nothing detects it. Returning `None` contributes no planner and leaves `fallback`
413413
in place.
414414

415+
`None` is the no-op, not `fallback`. The capsule handed to the first bundle wraps the
416+
session's planner for export, so returning it unchanged installs that planner as a
417+
foreign one and every plan built afterwards crosses an FFI boundary it did not before.
418+
A bundle that decides at runtime it has nothing to contribute returns `None`.
419+
415420
Three libraries that each ship a planner therefore install like this, with the
416421
outermost last:
417422

‎python/datafusion/extensions.py‎

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@
4848

4949
from __future__ import annotations
5050

51-
from dataclasses import dataclass
51+
from dataclasses import dataclass, fields
5252
from typing import TYPE_CHECKING, Any, Protocol, runtime_checkable
5353

5454
if TYPE_CHECKING:
@@ -135,7 +135,15 @@ class SessionExtensionComponents:
135135
...
136136
... def __datafusion_logical_extension_codec__(self, session=None):
137137
... return self._capsule
138-
>>> capsule = SessionContext().__datafusion_logical_extension_codec__()
138+
139+
The context stays in scope for as long as the codec does. An
140+
``FFI_LogicalExtensionCodec`` holds its task-context provider *weakly*,
141+
so a capsule taken off a throwaway ``SessionContext()`` names a session
142+
that is already gone and fails on first use with ``TaskContextProvider
143+
went out of scope over FFI boundary``:
144+
145+
>>> ctx = SessionContext()
146+
>>> capsule = ctx.__datafusion_logical_extension_codec__()
139147
>>> components = SessionExtensionComponents(
140148
... logical_extension_codecs=(NamedCodec(capsule),)
141149
... )
@@ -183,8 +191,17 @@ def __post_init__(self) -> None:
183191
and the class is frozen, so a list left in place would be a mutable
184192
member of an immutable value, and a generator would be exhausted by
185193
the first read.
194+
195+
Driven off :py:func:`dataclasses.fields` rather than a written-out
196+
list, so a codec field added later is normalized without anyone
197+
remembering to name it here. The ``_codecs`` suffix is what marks a
198+
field as one of them, leaving room for a future field that is not a
199+
codec collection and must not be turned into a tuple.
186200
"""
187-
for name in ("logical_extension_codecs", "physical_extension_codecs"):
201+
for field in fields(self):
202+
name = field.name
203+
if not name.endswith("_codecs"):
204+
continue
188205
value = getattr(self, name)
189206
# A str is iterable, so it would otherwise normalize into a tuple
190207
# of characters and fail much later as that many bogus codecs.
@@ -257,6 +274,12 @@ class SessionPlannerExportable(Protocol):
257274
not left encoding through a chain that a later bundle has grown.
258275
259276
Return ``None`` to contribute no planner and leave ``fallback`` in place.
277+
That is the no-op, and it is not the same as returning ``fallback``: the
278+
capsule the first bundle receives wraps the session's planner for export, so
279+
handing it back installs it as a foreign planner and every later plan crosses
280+
an FFI boundary that was not there before. A bundle with nothing to
281+
contribute returns ``None``.
282+
260283
Ignoring ``fallback`` and returning a planner that does not delegate to it
261284
is legal and means "replace" — but it discards every planner listed before
262285
this one, including any the session already had.
@@ -274,8 +297,9 @@ class SessionPlannerExportable(Protocol):
274297
... def __datafusion_session_planner__(self, ctx, fallback):
275298
... # A real library returns its own planner wrapping
276299
... # `fallback`, e.g. ``my_library.Planner(fallback=fallback)``.
277-
... # Handing it straight back is the degenerate wrap: valid,
278-
... # and contributes nothing.
300+
... # Handing it straight back is the degenerate wrap: legal,
301+
... # but it still installs `fallback` as a foreign planner.
302+
... # Return None instead to contribute nothing.
279303
... return fallback
280304
>>> isinstance(MyEngineExtension(), SessionPlannerExportable)
281305
True

‎python/tests/test_context.py‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -935,9 +935,11 @@ def __datafusion_session_extension__(self, ctx):
935935
class _PlannerExtension:
936936
"""Contributes a planner, recording the fallback it was handed.
937937
938-
Passing ``fallback`` straight back through is the degenerate wrap: the
939-
resulting session plans exactly as it did before, which is what lets a
940-
pure-Python test assert the threading without a real layering planner.
938+
Passing ``fallback`` straight back through is the degenerate wrap: it plans
939+
the same queries to the same plans, which is what lets a pure-Python test
940+
assert the threading without a real layering planner. It is not a no-op —
941+
the capsule gets installed, so the session ends up planning through a
942+
foreign planner — but nothing here depends on that either way.
941943
"""
942944

943945
def __init__(self):

0 commit comments

Comments
 (0)