Skip to content

Make extension codecs composable - #1678

Draft
timsaucer wants to merge 11 commits into
mainfrom
feat/ffi-composable-codecs
Draft

Make extension codecs composable#1678
timsaucer wants to merge 11 commits into
mainfrom
feat/ffi-composable-codecs

Conversation

@timsaucer

@timsaucer timsaucer commented Aug 7, 2026

Copy link
Copy Markdown
Member

Which issue does this PR close?

Part 2 of 3 in the split of #1672. Part 1 (#1677) is merged; this branch is rebased on it.

Rationale for this change

Supporting a foreign planner surfaced a codec problem: a query can involve three independent native libraries (datafusion-python, a provider library, and a planner library), and each library needs its extension codecs active on the session at the same time. Previously, installing a logical or physical extension codec replaced the prior codec, so the second library's install silently discarded the first — plans then failed later with a confusing decode error.

What changes are included in this PR?

Codecs compose instead of replacing

with_logical_extension_codec / with_physical_extension_codec append to a codec chain rather than replacing the prior codec. Encoding consults the chain in install order and the first codec to write bytes claims the object, so installing a library can claim objects nothing else claimed but never takes over an object an earlier codec was already encoding. Each codec encodes into a scratch buffer, so a failed attempt leaves no partial bytes. Ok with an empty buffer is treated as "no opinion" rather than a claim, so later codecs still get a turn.

with_python_udf_inlining clones the codec chain rather than rebuilding around a single inner codec, so toggling inlining no longer collapses a multi-codec chain to its first entry.

Decoding dispatches on identity, not on position or trial

This part was redesigned in response to review from @milenkovicm and @gabotechs — the earlier version of this PR tried each codec in turn until one returned Ok, which is unsound. Every payload an installed codec writes is now wrapped in an envelope naming the codec that produced it, laid out as DFPYCHN | version: u8 | id_len: u32 (LE) | id | blob. Decoding reads the id and consults exactly one codec, so a codec is never offered bytes it did not write, and its error surfaces verbatim rather than being masked by whichever codec was tried last.

Nothing is asked of the codec itself. An extension library implements LogicalExtensionCodec / PhysicalExtensionCodec exactly as it would for a session that installs only its own, and receives its payload byte for byte; the envelope is applied and stripped by datafusion-python and never reaches it. In particular a codec does not have to recognise and reject foreign payloads, which is not something a codec can reliably do: protobuf carries no type identity, so a prost message decodes cleanly from an unrelated message's bytes whenever their leading field numbers and wire types line up, and the natural implementation, MyMessage::decode(buf), has no prefix to check and cannot decline.

datafusion_proto::physical_plan::ComposedPhysicalExtensionCodec was suggested and is not reused, for three reasons:

  1. It keys dispatch on encoder_position, which is sound only when both ends assemble the same codec list in the same order. That holds for Ballista (a compile-time constant) and datafusion-distributed (own codec pinned at index 0, user codecs rebuilt from the same startup code on every node). It does not hold here: a chain is assembled by user Python, and Expr.to_bytes(ctx1) / Expr.from_bytes(ctx2) puts two independently configured sessions on either end of one payload, so an index names a different codec in the decoder as soon as install order differs.
  2. It exists only for the physical layer. There is no ComposedLogicalExtensionCodec, and this change needs both layers.
  3. encode_protobuf always wraps the result in DataEncoderTuple, so a codec that writes zero bytes still produces a non-empty payload. That sets fun_definition and permanently skips the FunctionRegistry lookup the decoder does first, breaking DataFusion's encode-by-name path.

Codec identity

An identity is derived automatically, in this order:

  1. An explicit codec_id= argument to the install call.
  2. __datafusion_codec_id__ on the exporting object. SessionContext declares one carrying its session id, because the class-derived fallback below would name every session at once.
  3. Otherwise the exporting class's module.QualName, which is the library's own import path and already stable across processes. This is the common case and asks nothing of existing extension libraries.
  4. For a bare PyCapsule there is nothing stable to read, since every capsule reports the same type. It gets a random id minted at install time, so a payload it writes decodes within the installing session's lineage and fails with a pointed error anywhere else. Pass codec_id= when such plans must cross sessions.

Installing two codecs under one id raises ValueError rather than shadowing the first, because a payload naming that id would otherwise resolve to whichever entry came first and only the caller knows whether the two write the same wire format. SessionContext.logical_extension_codec_ids() and physical_extension_codec_ids() list what is installed, which is also what a decode failure names.

Two payloads deliberately stay unframed

The terminal codec — Default{Logical,Physical}ExtensionCodec unless a Rust caller supplied another — writes bare, so a session with no extension codecs installed serializes byte-identically to a build without codec chaining.

An encode that writes nothing also stays empty, because try_encode_udf returning Ok with an empty buffer is DataFusion's encode-by-name signal. Framing it would set fun_definition and skip the registry lookup permanently. That empty-buffer path is the one place every installed codec is still consulted in turn, since there are no bytes to carry an identity; it is not the hazard the envelope removes, because the question asked is "do you own the function named x", which is name-scoped, and two codecs disagreeing requires them to claim the same function name — already a collision in the function registry.

Documentation

docs/source/contributor-guide/ffi.md gains a "Composable codecs" section, scoped to what an extension author or a caller wiring several libraries together can act on: that nothing is asked of the codec itself, that defensive prefix checks against another library's payloads are unnecessary, how identity is derived, when to pass codec_id=, that registration order between libraries does not affect decoding, that a codec may encode a function by name alone, the caution that composing whole sessions couples the imported codecs to the source session's lifetime, and a two-library registration recipe. It replaces the paragraph stating that installing a codec replaces the prior one. The design rationale behind those rules — the envelope layout, why trial decoding is unsound, why the bare-capsule identity is random, and the two payloads that stay unframed — lives in crates/core/src/codec.rs, which the section points to, rather than in the published guide. docs/source/user-guide/upgrade-guides.md gains a migration section, and both example READMEs are updated.

Test coverage

The chain behavior was originally covered by two #[cfg(test)] modules in crates/core/src/codec.rs. Those tests never ran: no workflow invokes cargo test, and the only Rust checks are cargo fmt --check and cargo clippy --no-deps --all-targets. --all-targets compiles test code, so the tests could not rot into a non-compiling state, but a behavioral regression would not have failed the build. Adding a cargo test job is also not a one-line change, because crates/core/Cargo.toml enables pyo3/extension-module unconditionally and the test binary therefore fails to link against Py_* on Linux. The coverage was moved to pytest instead, matching this repository's practice of treating the user-facing Python surface as the primary focus.

  • Both Rust test modules are removed. Five of the eighteen cases were already covered by existing pytest cases, and one (strip_errors_on_too_old_version) asserted nothing because WIRE_VERSION_MIN_SUPPORTED equals WIRE_VERSION_CURRENT.
  • python/tests/test_pickle_expr.py gains coverage for the wire-header diagnostics — an unsupported wire-format version and a Python major-version mismatch — by patching the header in place inside the encoded protobuf. The patches preserve length so the outer message stays parseable and the bytes reach the codec.
  • Three cases were dropped rather than ported. They truncate the wire header, which changes the payload length and breaks the enclosing protobuf framing, so they cannot reach the header check from Python.
  • The FFI example suite gains coverage for the chain itself, exercised across a real FFI boundary: encode ordering, that a later install cannot hijack an earlier codec's objects, that decoding reaches the codec that encoded and survives a different install order in the decoding session, the error naming a codec that is not installed, rejection of a duplicate id, the per-session and per-install identities, that a class-derived id is what goes on the wire, and that a pinned __datafusion_codec_id__ survives a class rename.
  • NameOnlyUdfCodec in the FFI example pins the by-name path: it owns functions fully described by their names, encodes no bytes, and rebuilds each function from the name on decode. It is the guard against framing empty payloads, which would break by-name round trips with nothing else in the suite noticing.
  • One inlining test is retained in the FFI example suite even though Add FFI query planner support #1677 added equivalent coverage in test_pickle_expr.py. That version installs a codec exported from another SessionContext, which is itself a Python-aware codec with inlining enabled; this one installs a codec that delegates UDF encoding to DataFusion's default codec, which is the realistic extension-library case and the only one that pins the strict outer codec's own behavior.
  • AGENTS.md records the Python-first testing preference and the fact that CI does not run Rust tests, so the tradeoff does not have to be rediscovered.

Are there any user-facing changes?

Behavior change. Installing an extension codec composes with previously installed codecs instead of replacing them. Code that relied on replacement semantics (installing a codec to remove a prior one) is affected; all other usage keeps working and no longer loses earlier codecs. There is no way to remove an installed codec.

Wire-format change. Once an extension codec is installed, payloads it writes carry the identity envelope. A session with no extension codecs installed produces the same bytes as before, as do functions encoded by name. Plans serialized by an earlier release and stored for later use should be regenerated if they were produced by a session with an extension codec installed.

New API surface, all additive:

  • codec_id= on with_logical_extension_codec / with_physical_extension_codec.
  • SessionContext.logical_extension_codec_ids() and physical_extension_codec_ids().
  • SessionContext.__datafusion_codec_id__, and recognition of __datafusion_codec_id__ on any object being installed as a codec.
  • Installing two codecs under one id now raises ValueError.

Carries the api change label, with a before/after section added to docs/source/user-guide/upgrade-guides.md.

MyLogicalExtensionCodec in examples/datafusion-ffi-example gains an optional provider_prefix argument that overrides the byte prefix it stamps on encoded table providers, which is what lets the tests install two instances owning disjoint slices of the wire format. It defaults to the previous constant, so existing call sites are unchanged, and the example README notes that real plugin libraries should hard-code a prefix unique to the library rather than accept one from the caller. The example also gains NameOnlyUdfCodec / NameOnlyFunction for the by-name path.

@ntjohnson1 ntjohnson1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm is it useful to be able to see for a given call which codec triggers? I see you can check the counts before/after with something like codec.table_provider_encode_calls() but I wonder if this does/should show up in an explain plan or something.

Comment thread crates/core/src/codec.rs
}
}

pub fn inner(&self) -> &Arc<dyn LogicalExtensionCodec> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was pub before. Do people care about inspecting the codecs directly?

Comment thread crates/core/src/codec.rs
}
}

pub fn inner(&self) -> &Arc<dyn PhysicalExtensionCodec> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar note about directly referencing codec chain

Comment thread docs/source/contributor-guide/ffi.md Outdated

# 1. Codecs from both libraries. Order between libraries does not matter.
ctx = ctx.with_logical_extension_codec(lib_a.codec())
ctx = ctx.with_logical_extension_codec(lib_b.codec())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is the resulting stage at this point [default, lib_a, lib_b]? I could read it or ask claude if some assumed based codec for standard datafusion python gets installed always as a fallback. Mostly I'm curious if it's a reasonable workflow for someone to set things up to only get [lib_a, lib_b] and if anything unsupported by their custom codecs barfs.

The example codecs do not inspect the callback `TaskContext`. A production codec that depends on session configuration or registered functions must ensure its exported FFI codec is bound to, and retains, the appropriate host `TaskContextProvider`.

The current Python API installs one external logical codec and one external physical codec. It does not yet compose codecs from several independent plugin owners. This example therefore makes the provider library the sole external codec owner; the planner uses built-in physical nodes and receives the provider codecs from the host.
Extension codecs compose: each `with_logical_extension_codec` / `with_physical_extension_codec` call prepends the codec to the session's codec chain, with the most recently installed codec consulted first and DataFusion's default codec as the terminal fallback. A codec signals "not mine" by returning an error, so several independent plugin libraries can install codecs on the same session as long as each only answers for payloads it owns (frame them with a distinct byte prefix). In this example the provider library is the only codec owner; the planner uses built-in physical nodes and receives the provider codecs from the host.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok this line says DF default codec is the terminal fallback but I think the constructor takes a codec in rust at least so it could potentially be updated there but maybe not in the exposed python surface.

@milenkovicm milenkovicm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have a look at datafusion_proto::physical_plan::ComposedPhysicalExtensionCodec which should handle this case. try decode can produce wrong results in some cases and its hard to debug when it does

Comment thread crates/core/src/codec.rs Outdated
/// corrupt-token error from the codec that owns the payload family).
fn chain_try<C: ?Sized, R>(chain: &[Arc<C>], what: &str, f: impl Fn(&C) -> Result<R>) -> Result<R> {
let mut errors: Vec<datafusion::error::DataFusionError> = Vec::new();
for codec in chain {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not safe, you can get things decoded by accident. please dont ask how i know and how much time i spent debuging it

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☝️ this is important, some more context in apache/datafusion#16980 and apache/datafusion#16986.

Because of how protobuf decoding works, it's very easy to be in situations where the same protobuf payload can decode to multiple prost structs, specially in cases where the structs are specially simple.

@timsaucer timsaucer Sep 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You were both right, and I've reworked the dispatch rather than keeping trial decoding. Thanks for the #16980 / #16986 pointers — the CSV-decoded-as-Parquet case is exactly the failure mode, and it's not one a codec can defend against from inside, since MyMessage::decode(buf) has no prefix to check and cannot decline.

Every payload an installed codec writes is now wrapped in an envelope naming its author: DFPYCHN | version: u8 | id_len: u32 (LE) | id | blob. Decoding reads the id and consults exactly one codec. Nothing is ever offered to a codec that did not write it, so structurally similar prost messages can't cross libraries. Codecs themselves are unchanged — they receive their payload byte for byte and never see the envelope.

On reusing ComposedPhysicalExtensionCodec: I looked at it closely and it doesn't fit here, for three reasons.

  1. It keys on encoder_position, which is sound when both ends build the same list in the same order. That's true for the consumers it was written for — Ballista's list is a compile-time constant, and datafusion-distributed pins its own codec at index 0 and appends user codecs rebuilt from the same startup code on every node. It isn't true here: the chain is assembled by user Python, and Expr.to_bytes(ctx1) / Expr.from_bytes(ctx2) puts two independently configured sessions on either end of one payload. An index names a different codec in the decoder as soon as install order differs. test_decode_survives_a_different_install_order is the case.
  2. It only exists for the physical layer. There's no ComposedLogicalExtensionCodec, and this needs both — table providers and UDFs go through the logical codec.
  3. encode_protobuf always wraps in DataEncoderTuple, so a codec that writes zero bytes still emits a non-empty payload. That sets fun_definition and permanently skips the FunctionRegistry lookup the decoder does first, which breaks DataFusion's encode-by-name path. I need that path to keep working — NameOnlyUdfCodec in the FFI example owns functions that are fully described by their names and encodes nothing at all.

The reasoning is recorded in the code at crates/core/src/codec.rs (see ChainEntry and chain_decode, which cite both upstream issues) so the next person doesn't re-derive it, and the "Composable codecs" section of docs/source/contributor-guide/ffi.md covers it for extension authors. The empty-payload case is the one place codecs are still consulted in turn, because there are no bytes to carry an identity; that question is "do you own the function named x", which is name-scoped, and two codecs disagreeing means they've already collided in the function registry.

Comment thread crates/core/src/codec.rs
/// chance to emit a richer payload. If no codec writes bytes but at
/// least one returned `Ok`, the overall result is `Ok` with nothing
/// written (encode by name).
fn chain_encode<C: ?Sized>(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might work, but still can be tricky

Base automatically changed from feat/ffi-query-planner-core to main August 28, 2026 20:21
timsaucer and others added 3 commits August 30, 2026 18:28
Installing a logical or physical extension codec now prepends it to a
codec chain instead of replacing the prior codec. The most recently
installed codec is consulted first, falling through codec by codec to
the default codec. This lets multiple independent extension libraries
install codecs on the same session, and removes the codec registration
ordering requirement between libraries.

Chain dispatch treats a codec error as "not mine". Encoding runs each
codec against a scratch buffer so failed attempts leave no partial
bytes, and treats Ok-with-no-bytes (encode by name) as no opinion so
later codecs still get a chance. When every codec fails, the errors
are aggregated so the owning codec's diagnostic is not masked by the
default codec's generic error.

Also preserves the python_udf_inlining setting when installing a
codec; previously it was silently reset to enabled.

Documents the remaining planner constraint: a session holds one query
planner, layering is explicit via fallback capsules, and codecs must
be installed before exporting or chaining planners because a planner
capsule captures the codecs at export time.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Rust tests added for the composable codec work never ran: CI invokes
`cargo fmt` and `cargo clippy --all-targets` but no `cargo test`, so the
tests compiled and were never executed. Rather than add a `cargo test`
job — which would also require feature-gating `pyo3/extension-module`,
since the test binary cannot link on Linux while it is unconditional —
move the coverage to pytest, matching this repository's practice of
treating the user-facing Python surface as the first line of defense.

Remove both `#[cfg(test)]` modules from crates/core/src/codec.rs and
replace them as follows:

- Four wire-header round-trip tests and the Python-minor-mismatch test
  were already covered by existing cases in test_pickle_expr.py.
- `strip_errors_on_too_old_version` asserted nothing: it returns early
  because WIRE_VERSION_MIN_SUPPORTED equals WIRE_VERSION_CURRENT.
- The unsupported-wire-version and Python-major-mismatch cases move to
  test_pickle_expr.py, patching the header in place inside the encoded
  protobuf. The patches preserve length so the outer message stays
  parseable and the bytes reach the codec.
- The three truncated-header cases are dropped. Truncation changes the
  payload length and breaks the protobuf framing, so they fail before
  reaching the header check and cannot be expressed from Python.
- The codec-chain tests move to the FFI example suite, which exercises
  the same chain through the real FFI boundary.

MyLogicalExtensionCodec gains an optional token overriding the byte
prefix it stamps on encoded table providers. Two instances with distinct
tokens own disjoint slices of the wire format, which is what makes chain
ordering and fall-through observable from Python.

The ported inlining test asserts encode and decode behavior rather than
the `python_udf_inlining()` getter the Rust test checked. This is a
stronger assertion: the getter is preserved even when a composed
Python-aware codec re-inlines a UDF that the outer strict codec declined
to inline, so the original test could not have caught that path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
New coverage should land as a doctest example or a pytest case. Agents have
been adding Rust tests that CI never executes: no workflow invokes
`cargo test`, and `cargo clippy --all-targets` only compiles the test code.
Write down that constraint, along with the reason a `cargo test` job is not a
trivial addition, so the tradeoff does not have to be rediscovered.

Also point at the FFI example suites, which are easy to overlook when judging
whether behavior is reachable from Python.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@timsaucer
timsaucer force-pushed the feat/ffi-composable-codecs branch from b32667e to c1ffd81 Compare August 31, 2026 11:05

@gabotechs gabotechs left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth following @milenkovicm's suggestion and reuse the ComposedPhysicalExtensionCodec.

Comment thread crates/core/src/codec.rs Outdated
/// corrupt-token error from the codec that owns the payload family).
fn chain_try<C: ?Sized, R>(chain: &[Arc<C>], what: &str, f: impl Fn(&C) -> Result<R>) -> Result<R> {
let mut errors: Vec<datafusion::error::DataFusionError> = Vec::new();
for codec in chain {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☝️ this is important, some more context in apache/datafusion#16980 and apache/datafusion#16986.

Because of how protobuf decoding works, it's very easy to be in situations where the same protobuf payload can decode to multiple prost structs, specially in cases where the structs are specially simple.

timsaucer and others added 4 commits August 31, 2026 16:23
Decoding walked the codec chain and took the first codec that returned
Ok. That is unsound, and upstream has already been bitten by it:
apache/datafusion#16980 records ComposedPhysicalExtensionCodec decoding
an encoded parquet file as csv because the payload happened to parse
with an earlier codec, and #16986 fixed it by recording the encoder.
Protobuf carries no type identity, so two codecs whose leading field
numbers and wire types line up decode each other's payloads cleanly.
A byte-prefix convention does not help: the natural implementation is
`Message::decode(buf)`, which has no prefix to check and cannot decline.

Payloads written by a chained codec now carry an envelope naming the
codec that wrote them, and decoding consults exactly that codec. The
envelope is applied and stripped inside PythonLogicalCodec /
PythonPhysicalCodec, so third-party codecs are unmodified and never see
it -- they receive the bare bytes they wrote.

Keyed on a stable identity rather than chain position. Position is
sound for Ballista and datafusion-distributed because their codec lists
are pinned -- a compile-time constant in one, a fixed entry plus
appended user codecs rebuilt from the same startup code in the other.
A datafusion-python chain is assembled by user Python across sessions
that share no struct, and a library shipping a codec cannot know its own
index, so position would silently name the wrong codec whenever two
sessions install in different orders.

Identity is derived and asks nothing of existing libraries: an explicit
codec_id, else __datafusion_codec_id__, else the exporting class's
module and qualified name. A bare PyCapsule exposes nothing stable --
every capsule reports the same type -- so it gets a session-local id and
a pointed error if its payloads reach an unrelated session. Installing
two codecs under one id is rejected at install time rather than
resolving to whichever entry came first.

Codecs now append rather than prepend, so encoding is claimed by the
first codec installed that wants the object. Installing a library can
only claim objects nothing else claimed; it can never take over an
existing library's objects, and it never renumbers ids that older
payloads reference.

Two payloads are deliberately left unframed. The terminal codec writes
bare, so a session with no extension codecs is byte-identical to a build
without chaining. And an encode that writes nothing stays empty: an
empty fun_definition is DataFusion's encode-by-name signal, and framing
it would set the field, permanently skipping the registry lookup the
decoder does first and breaking codecs that reconstruct a function from
its name alone (from_proto.rs, the `None => ctx.udf(..).or_else(..)`
arm). That arm is also why an empty buffer still consults every codec:
there are no bytes to tag. It is not the hazard tagging removes -- the
question asked is "do you own the function named x", which is
name-scoped, and a disagreement needs two libraries claiming one name.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds NameOnlyUdfCodec to the FFI example: a codec owning functions that
are fully described by their names, so try_encode_udf writes nothing and
try_decode_udf rebuilds the function from the name with no registry
entry. DataFusion supports that shape directly -- an empty
fun_definition sends the decoder to the FunctionRegistry first and the
codec second (from_proto.rs, the `None => ctx.udf(..).or_else(..)` arm).

Nothing covered that arm before, and it is the path most at risk from a
plausible change: wrapping every chained encode in the identity envelope
would make an empty payload non-empty, set fun_definition, and skip the
registry lookup permanently. That breaks ordinary by-name round trips as
well as codecs like this one, and no other test would notice. The
decoding session here deliberately never registers the function, so only
the codec can supply it.

Exposes logical_extension_codec_ids() and physical_extension_codec_ids()
on SessionContext. The ids are the dispatch keys a payload names, so
listing them answers "which library owns this plan" and "can this
session decode it" -- and they are what a decode failure reports.

Documentation rewritten around identity rather than ordering:

- ffi.md states that codecs need no changes, why rejecting foreign
  payloads is not something a codec can reliably do, the identity
  resolution ladder, and the two payloads left unframed with the reason
  for each.
- Both example READMEs drop the "most recently installed is consulted
  first" language, which described dispatch that no longer exists.
- upgrade-guides.md gains a section for the behaviour change: codecs
  compose rather than replace, no codec-side change is required, and the
  wire format changes only for sessions that install one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The skill already triggers on "any FFI_* export that asks for ... an
extension codec", but had nothing to say about how a chain decides which
codec handles a payload -- the part of this area most likely to be got
wrong, and the part with a rationale that is not visible from the code.

Rule 8 records it: dispatch by identity rather than by trying codecs
until one returns Ok, with the upstream incident that settles the
question (apache/datafusion#16980, a Parquet payload decoded as CSV) and
the protobuf reason a byte-prefix convention cannot fix it.

It also records why we key on identity where Ballista,
datafusion-distributed, and upstream all key on chain position. Their
lists cannot disagree -- one is a compile-time constant, the other a
pinned entry plus user codecs rebuilt from the same startup code. Ours is
assembled by user Python, and to_bytes/from_bytes puts two independently
built sessions on either end of one payload. Copying the upstream design
here reintroduces the same silent mis-decode from the other direction,
which is exactly the kind of thing an agent reading only upstream would
do.

And it records the two payloads that are never framed, because framing
the empty one is a plausible tidy-up that breaks by-name decoding
silently. Points at NameOnlyUdfCodec as the guard.

Extends the trigger to cover codec.rs dispatch, which the old wording
did not obviously reach, and documents the optional
__datafusion_codec_id__ hook on the two codec Protocols -- where Rule 5
says such things belong.

Reviewed the other four skills: check-upstream, make-pythonic,
audit-skill-md, and the user-facing datafusion_python skill are all
unaffected, none of them touching codec dispatch.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rule 8 restated, less precisely, what the docstrings in codec.rs already
say: identity dispatch, why not position, the two unframed payloads,
append-and-first-claim-wins. A skill earns its place by carrying what
you cannot learn from the file you are about to edit -- Rule 1 greps a
family across files, Rule 3 governs code in an extension author's own
repo, Rule 5 is a four-file process checklist. "How the dispatch in
codec.rs works" is what codec.rs is for, and duplicating it there just
creates a second copy to keep in sync.

Two pieces were worth keeping, and neither is a rule.

The apache/datafusion#16980 citation moves to `chain_decode`, next to
the code someone would be editing when tempted to delete the envelope
and walk the chain instead. It is the concrete evidence that makes the
warning land, and it was the one thing the skill had that the code did
not.

Rule 5 gains a clause: changing what a codec puts on the wire is as
breaking as changing a getter signature, and easier to miss because
nothing fails to compile. That is genuinely cross-file process, which is
what the skill is for.

Also enriches ChainEntry's docstring with the datafusion-distributed
half of the "why not position" argument, which previously named only
Ballista and upstream. Both consumers matter: the point is that their
codec lists structurally cannot disagree and ours can.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
timsaucer and others added 3 commits September 1, 2026 08:31
A codec installed from a bare PyCapsule has nothing stable to derive an
identity from, so it was tagged `anon:{chain_length}`. That is a position,
not an identity: every session numbers from the same end, so two unrelated
sessions that each install one capsule both mint `anon:0`. A payload written
by the first was then handed to the second session's codec — the positional
dispatch this chain design exists to avoid, reintroduced in the one case
that has no derivable id.

The failure was quiet. A codec offered bytes it does not recognise falls
through to its own inner default codec, so the error came back as
`LogicalExtensionCodec is not provided` and named neither codec, instead of
the intended "encoded by extension codec X, which is not installed on this
session" with the `codec_id=` hint.

Mint a random id per install instead, reusing the `Uuid` idiom already in
`context.rs`. The chain clones the id along with the codec, so a payload
still decodes anywhere in the installing session's lineage; everywhere else
it now fails with the pointed error. `derive_codec_id` no longer needs the
chain length, so that parameter goes away.

Also fixes two docs that described the pre-identity-dispatch design: the
`codec.rs` module doc claimed codecs are consulted most-recently-installed
first (it is install order, and decoding does not walk the chain at all) and
asked downstream codecs to reject foreign payloads (an encode-side contract
only — a payload only ever reaches the codec whose id it carries), and a
physical codec test docstring said a second install prepends.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…class

Installing one context's codec stack on another session derived the
identity from the class, and every `SessionContext` shares one class. So
every session reported `datafusion.context.SessionContext`: two of them
could not coexist on one target, and a payload written through one
resolved to the other on decode, failing as
`LogicalExtensionCodec is not provided` from the stranger's own inner
default codec. Same shape as the positional `anon:{n}` collision, but via
an id that looks portable, so the error carried no hint either.

`SessionContext` now declares `__datafusion_codec_id__` carrying its
session id. That goes through the existing resolution arm for an object
pinning its own identity, so one implementation covers both the Python
wrapper and the internal object and `derive_codec_id` keeps its four
documented arms. Handles derived from one session report the same id, so
installing two of them on one target is refused — their payloads would be
indistinguishable on decode.

Framing is unchanged. A session-exported codec is opaque across the FFI
boundary, so the outer session cannot enumerate what is inside it and the
envelope naming its own entry is the only handle it has; nesting costs
about 50 bytes per hop and only arises when composing sessions, which a
library exporting its own codec class never does. What that route needed
was a usable identity, not fewer frames.

Documents the lifetime coupling that comes with it: imported codecs
resolve their task context against the source session and stop working
when it is dropped, so this composes sessions rather than copying codecs
out of one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A codec that declares no identity is named `module.QualName`, and that
string goes on the wire in front of every payload it writes. Nothing
asserted it, so renaming `MyLogicalExtensionCodec` or moving its module
would have silently changed the wire format with a green suite.

Pins it twice: against the literal, so a rename has to come here and be
acknowledged, and against `__module__`/`__qualname__`, so the literal
cannot drift away from what the code actually derives.

Also covers the reason `__datafusion_codec_id__` exists, which had no
test either. A plan encoded by a codec under its old class name decodes
on a session that only knows the new one, and the payload carries the
pinned id rather than either class name. Under the class-derived default
those are two different ids and the plan is undecodable.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@timsaucer

Copy link
Copy Markdown
Member Author

@ntjohnson1 on your question about seeing which codec triggers for a given call — partly answered now, though not in EXPLAIN.

SessionContext.logical_extension_codec_ids() and physical_extension_codec_ids() list what's installed, in install order, and those ids are exactly what a payload carries — so a decode failure names the codec that wrote the bytes and lists what the session actually has, e.g. a table provider was encoded by extension codec 'lib_a.Codec', which is not installed on this session (installed: lib_c.Codec).

What's still missing is your actual question: per-call attribution, "which codec handled this node in this plan". Encoding decides that by walking the chain until one claims the object, and nothing records the winner. Surfacing it in EXPLAIN would mean threading it through plan formatting, which I'd rather not fold into this PR. Happy to file it as a follow-up if you think it's worth having.

Your other three comments are addressed as well: the inner() accessor is replaced by codecs() / codec_ids() / terminal() on the Rust side and logical_extension_codec_ids() / physical_extension_codec_ids() from Python, and both the contributor guide and the example README now state that the terminal fallback is Default{Logical,Physical}ExtensionCodec unless a Rust caller supplies another to Python{Logical,Physical}Codec::new, which is not reachable from the Python surface — so a chain of [lib_a, lib_b] with no default behind it isn't something you can set up from Python.

The codec documentation added in this PR drifted into narrating the
implementation. An extension author cannot act on the name of a private
Rust type, and a caller installing a library's codec does not need the
envelope layout or the argument for why trial decoding is unsound.

Removed from `ffi.md`: the paragraph naming `Python{Logical,Physical}Codec`
as the thing that wraps payloads, the protobuf-ambiguity argument with the
upstream postmortem, the note on why the bare-capsule id is random, and the
two-unframed-payloads section, which opened by addressing whoever is
"changing this code". What each reader can act on stays: implement your
codec as though yours is the only one, do not write defensive prefix checks,
how identity is derived, when to pass `codec_id=`, that install order does
not affect decoding, and that a codec may encode a function by name alone.
All of the removed rationale already lives in `crates/core/src/codec.rs`,
which the section now points to for anyone changing the framing.

Trimmed `with_logical_extension_codec` to what a caller acts on, with a
`:ref:` to the FFI guide for the rest, and added the `ValueError` on a
duplicate id, which the longer version buried. `with_physical_extension_codec`
now delegates in one sentence instead of restating the same paragraphs, which
had already begun to drift.

Reworded the internal vocabulary out of the upgrade guide and
`logical_extension_codec_ids`: "terminal codec", "writes unframed", and
"identity envelope" name internals a reader cannot see. Dropped the
explanation of why a session id rather than a class name identifies a
context installed as a codec, keeping the constraint that follows from it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants