Skip to content

Commit 2425559

Browse files
timsaucerclaude
andcommitted
docs: scope the planner codec rebind to one layer
Installing a codec rebuilds the installed planner against it, but the rebuild reaches exactly one `ForeignQueryPlanner`. A planner that resolved a fallback at install time keeps that fallback's codecs, and neither side can repair it: the host has no handle past the first layer, and the planner library cannot re-derive codecs at plan time because `FFI_QueryPlanner` holds them by value and `Session` exposes no accessor for the host's current ones. Tracked upstream in apache/datafusion#24762. The examples cannot demonstrate it. Their fallback lives in the same cdylib as its wrapper, and `From<&FFI_QueryPlanner>` short-circuits on a matching `library_marker_id`, so a same-library hop never serializes. Measured: a layered planner produces the same codec traffic as a flat one. What is demonstrable is that the session's planner tracks whichever handle wrote it last, so re-installing a planner from the original handle rebinds the session back to that handle's codecs rather than picking up a codec installed through a derived one. Pinned by a new test as the sequel to `test_a_discarded_derived_context_still_rebinds_the_planner`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 6882297 commit 2425559

6 files changed

Lines changed: 98 additions & 2 deletions

File tree

.ai/skills/ffi-capsule-protocol/SKILL.md

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,23 @@ reintroduce a `with_query_planner` that pretends otherwise — the only way to
160160
give a handle its own planner is a fresh `Arc<SessionContext>`, which is what
161161
Rule 6 forbids.
162162

163+
Installing a codec rebuilds the installed planner against it, and that rebuild
164+
reaches exactly one layer. `FFI_QueryPlanner::new_with_ffi_codecs` unwraps one
165+
`ForeignQueryPlanner`; a fallback that planner resolved at install time sits in
166+
its library's private data with no handle on this side, and cannot re-derive
167+
codecs itself because `FFI_QueryPlanner` holds them by value and `Session`
168+
exposes no accessor for the host's current ones. So do not promise that install
169+
order is free — for a layered planner it is not. The examples cannot show this:
170+
their fallback lives in the same cdylib as its wrapper, and `datafusion-ffi`
171+
short-circuits a same-library hop rather than serializing. A fix has to come
172+
from upstream; tracked in
173+
[apache/datafusion#24762](https://github.com/apache/datafusion/issues/24762).
174+
175+
The session's planner also tracks whichever handle wrote it last, so
176+
re-installing a planner on the original handle rebinds the session back to that
177+
handle's codecs. `test_reinstalling_a_planner_rebinds_the_session_to_that_handles_codecs`
178+
pins that; changing it should be deliberate.
179+
163180
## Where the truth is
164181

165182
- `docs/source/contributor-guide/ffi.md` — the protocol, the fork caveat.

docs/source/contributor-guide/ffi.md

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -369,6 +369,43 @@ installed planner instead of layering another one. To chain planners, have the n
369369
planner wrap the capsule returned by `SessionContext.__datafusion_query_planner__()`,
370370
captured before the new planner is installed, and delegate to it explicitly.
371371

372+
### Rebinding a planner's codecs is one level deep
373+
374+
The rebuild above swaps the codecs on the installed `ForeignQueryPlanner` handle, and
375+
only that handle. A planner that wraps a fallback resolved that fallback when *it* was
376+
installed, and holds the result inside its own library's private data — behind a
377+
`create_physical_plan` function pointer, with no Python-side handle. A codec installed
378+
afterwards therefore reaches the outer planner and not the fallback, which keeps
379+
whichever codecs were in force when it was imported.
380+
381+
Neither side can repair that:
382+
383+
- **The host cannot reach it.** `FFI_QueryPlanner::new_with_ffi_codecs` unwraps exactly
384+
one `ForeignQueryPlanner` layer. There is no deeper handle to unwrap — the same
385+
situation as a codec embedded in a registered `FFI_CatalogProvider`.
386+
- **The planner library cannot re-derive it.** `FFI_QueryPlanner` holds its codecs by
387+
value, and `Session` exposes no accessor for the ones the host currently has, so
388+
`create_physical_plan` cannot pick them up from the session it is handed. The rebuild
389+
has to be eager, and an eager rebuild only sees the top layer.
390+
391+
A fix has to come from upstream, and is tracked in
392+
[apache/datafusion#24762](https://github.com/apache/datafusion/issues/24762).
393+
394+
The stale codecs stay usable rather than dangling — they hold weak handles to the one
395+
`Arc<SessionContext>` that Rule 6 keeps alive — so the effect is a fallback hop
396+
serializing with an older codec, not a failure. It is also invisible to the examples
397+
here, which use one fallback in the same cdylib as its wrapper; `datafusion-ffi`
398+
short-circuits a same-library hop rather than serializing, so no codec runs. A fallback
399+
in a *different* library would serialize, and would do it with the codecs it was
400+
imported with.
401+
402+
So install the codecs before a layered planner. If a codec has to go in afterwards,
403+
install the outer planner again *on the handle that holds the new codec* — that re-runs
404+
its getter, which re-imports the fallback against that handle's codecs. Re-installing on
405+
the original handle rebinds the session's planner back to the original handle's codecs
406+
instead, which is the trap
407+
`test_reinstalling_a_planner_rebinds_the_session_to_that_handles_codecs` pins.
408+
372409
## Alternative Approach
373410

374411
Suppose you needed to expose some other features of DataFusion and you could not wait

examples/datafusion-ffi-example/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,6 @@ ctx = ctx.with_physical_extension_codec(provider_physical_codec)
4343
ctx.set_query_planner(planner)
4444
```
4545

46-
Installing a codec after the planner rebuilds the planner against it, so this order is a recommendation rather than a requirement. Planner-last states the ownership flow more clearly.
46+
Installing a codec after the planner rebuilds the planner against it, so this order is a recommendation rather than a requirement. Planner-last states the ownership flow more clearly. The exception is a planner that wraps a fallback: the rebuild reaches the installed planner only, not the fallback inside it, so codecs-first is a requirement there. See [Rebinding a planner's codecs is one level deep](../../docs/source/contributor-guide/ffi.md#rebinding-a-planners-codecs-is-one-level-deep), which also covers why re-installing a planner rebinds the session to the codecs of whichever handle it was installed on.
4747

4848
For the limits behind that choice — why there is one external codec owner rather than a registry, which node kinds survive the boundary, and what a derived context shares with the context it came from — see [Query Planners Across Multiple Libraries](../../docs/source/contributor-guide/ffi.md#query-planners-across-multiple-libraries) in the contributor guide.

examples/datafusion-ffi-query-planner-example/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,6 @@ ctx.set_query_planner(MyQueryPlanner())
5555

5656
`MyPlannerConfig` is transferred through the foreign session. `MyQueryPlanner` reads `ffi_query_planner.max_rows`, creates the plan with `DefaultPhysicalPlanner`, and adds a built-in `GlobalLimitExec`. The test changes the setting with `SET` and verifies the new row limit.
5757

58-
The provider's codec pair is attached to the planner when it is installed and is also used to decode the returned physical plan in `datafusion-python`. This planner deliberately uses only built-in physical nodes. Install the codecs before the planner where possible; installing a codec afterwards rebuilds the planner against it, but planner-last order is easier to audit.
58+
The provider's codec pair is attached to the planner when it is installed and is also used to decode the returned physical plan in `datafusion-python`. This planner deliberately uses only built-in physical nodes. Install the codecs before the planner where possible; installing a codec afterwards rebuilds the planner against it, but planner-last order is easier to audit. That rebuild is one level deep — a planner constructed with `fallback=` keeps the codecs its fallback was imported with — so codecs-first is a requirement rather than a preference once planners are layered. See [Rebinding a planner's codecs is one level deep](../../docs/source/contributor-guide/ffi.md#rebinding-a-planners-codecs-is-one-level-deep).
5959

6060
For the limits behind that choice — why there is one external codec owner rather than a registry, which node kinds survive the boundary, and what a derived context shares with the context it came from — see [Query Planners Across Multiple Libraries](../../docs/source/contributor-guide/ffi.md#query-planners-across-multiple-libraries) in the contributor guide.

examples/datafusion-ffi-query-planner-example/python/tests/_test_three_library_query_planner.py

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -535,6 +535,41 @@ def test_a_discarded_derived_context_still_rebinds_the_planner():
535535
assert later.table_provider_encode_calls() > 0
536536

537537

538+
def test_reinstalling_a_planner_rebinds_the_session_to_that_handles_codecs():
539+
"""A planner is built against the codecs of the handle it is installed from.
540+
541+
The sequel to the test above, and the trap it sets up. Once a discarded
542+
derived handle has rebound the session's planner to its codec, installing
543+
the same planner again from the *original* handle rebuilds it against that
544+
handle's codec instead -- which never changed. The session's planner tracks
545+
whichever handle wrote it last, not the newest codec installed anywhere.
546+
547+
So "re-install the planner after installing a codec" only repairs anything
548+
when it is done from the handle holding the new codec.
549+
"""
550+
ctx, original_logical, _physical_codec = codec_context()
551+
planner = MyQueryPlanner()
552+
ctx.set_query_planner(planner)
553+
554+
later = MyLogicalExtensionCodec()
555+
# Deliberately discarded, exactly as in the test above.
556+
ctx.with_logical_extension_codec(later)
557+
gc.collect()
558+
559+
ctx.sql('SELECT "A" FROM numbers ORDER BY "A"').collect()
560+
assert later.table_provider_encode_calls() > 0
561+
assert original_logical.table_provider_encode_calls() == 0
562+
563+
# `ctx`'s own codec field never changed, so this rebuilds the planner
564+
# against `original_logical` and drops `later` from the session's planner.
565+
ctx.set_query_planner(planner)
566+
encodes_by_later = later.table_provider_encode_calls()
567+
568+
ctx.sql('SELECT "A" FROM numbers ORDER BY "A"').collect()
569+
assert original_logical.table_provider_encode_calls() > 0
570+
assert later.table_provider_encode_calls() == encodes_by_later
571+
572+
538573
def test_query_planner_requires_provider_codec():
539574
config = SessionConfig().with_extension(MyPlannerConfig(max_rows=2))
540575
ctx = SessionContext(config)

python/datafusion/context.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1788,6 +1788,13 @@ def set_query_planner(self, planner: QueryPlannerExportable | _PyCapsule) -> Non
17881788
:meth:`~SessionContext.__datafusion_query_planner__`, captured
17891789
*before* the new planner is installed.
17901790
1791+
Install any extension codecs before a layered planner. Installing a
1792+
codec afterwards rebuilds the installed planner against it, but not the
1793+
fallback inside it, which keeps the codecs it was imported with. Note
1794+
also that the planner is built against the codecs of the context this
1795+
method is called on, so installing the same planner again on a different
1796+
handle rebinds the session's planner to *that* handle's codecs.
1797+
17911798
Args:
17921799
planner: Object exposing ``__datafusion_query_planner__`` (see
17931800
:class:`QueryPlannerExportable`) or a raw

0 commit comments

Comments
 (0)