(datafusion-python) branch feat/ffi-composable-codecs updated: docs: state the one case where a codec sees bytes it may not own

[ti...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

timsaucer pushed a commit to branch feat/ffi-composable-codecs
in repository https://gitbox.apache.org/repos/asf/datafusion-python.git


The following commit(s) were added to refs/heads/feat/ffi-composable-codecs by this push:
     new 29e360fe docs: state the one case where a codec sees bytes it may not own
29e360fe is described below

commit 29e360fe4347f71b0929de3e3719966fe7805378
Author: Tim Saucer <ti...@gmail.com>
AuthorDate: Tue Sep 1 12:25:14 2026 -0400

    docs: state the one case where a codec sees bytes it may not own
    
    The FFI guide told codec authors not to guard against another library's
    payloads, on the grounds that such a check "simply never fires". That was
    wrong twice over. The check executes on every decode, and its reject branch
    is reachable: an empty payload carries no identity to route on, so
    `chain_resolve_by_name` offers it to every installed codec in turn, and a
    codec asked about a function it does not own rejects — correctly.
    
    That exception is worth documenting on its own terms, because an author who
    trusts the "you only ever see your own payload" guarantee can index into an
    empty `buf` and panic. `try_decode_udf` and its aggregate and window
    siblings can be called with an empty buffer and another library's `name`, so
    the guide now says to decide from `name` and not to assume `buf` is
    non-empty. `name_only_codec.rs` in the FFI example already does exactly
    that, so the guide and the worked example agree.
    
    The advice against defensive checks is gone rather than reworded. An author
    who already writes one keeps it and nothing breaks; an author who does not
    needs nothing. What remains is the portability case: a codec that also ships
    to hosts dispatching by position or by trial may still want its own guard,
    and keeping one costs nothing here.
    
    Two claims in codec.rs conflated a marker check with relying on decode
    failure. A marker an encoder writes and a decoder checks is reliable — it is
    what the envelope itself does. What cannot be relied on is
    `MyMessage::decode(buf)` declining someone else's bytes, and what cannot be
    required is third-party codecs all honouring such a convention. Both are now
    stated as what they are.
    
    Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
 crates/core/src/codec.rs             | 21 +++++++++++++--------
 docs/source/contributor-guide/ffi.md | 13 +++++++++----
 2 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/crates/core/src/codec.rs b/crates/core/src/codec.rs
index fd32fae7..f0b7e300 100644
--- a/crates/core/src/codec.rs
+++ b/crates/core/src/codec.rs
@@ -90,8 +90,11 @@
 //! ahead of every codec installed after it.
 //!
 //! Rejecting foreign *payloads* is not asked of a codec, because a
-//! codec cannot reliably do it: a payload is only ever handed to the
-//! codec whose id it carries. See [`PythonLogicalCodec`].
+//! payload is only ever handed to the codec whose id it carries. A
+//! codec is free to check for a marker of its own anyway — that costs
+//! nothing here and is worth keeping for hosts that dispatch by
+//! position or by trial — but dispatch never depends on it. See
+//! [`PythonLogicalCodec`].
 
 use std::sync::Arc;
 
@@ -384,12 +387,14 @@ fn read_chained_payload(buf: &[u8]) -> Result<Option<(&str, &[u8])>> {
 /// is unsound: 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 an all-defaults
-/// message encodes to zero bytes that decode as anything. Asking codecs
-/// to check a byte prefix does not fix it either, because the natural
-/// implementation is `MyMessage::decode(buf)`, which has no prefix to
-/// check and cannot decline. Upstream shipped that design and reverted
-/// it after a Parquet payload decoded as CSV — apache/datafusion#16980,
-/// fixed in #16986.
+/// message encodes to zero bytes that decode as anything. Requiring
+/// each codec to check a marker of its own does not fix it either: the
+/// codecs come from libraries this crate does not control, the natural
+/// implementation is `MyMessage::decode(buf)`, which has no marker to
+/// check and cannot decline, and one library skipping the convention is
+/// enough to lose someone else's payload. Upstream shipped that design
+/// and reverted it after a Parquet payload decoded as CSV —
+/// apache/datafusion#16980, fixed in #16986.
 fn chain_decode<C: ?Sized, R>(
     chain: &[ChainEntry<C>],
     terminal: &Arc<C>,
diff --git a/docs/source/contributor-guide/ffi.md b/docs/source/contributor-guide/ffi.md
index 4cfe8eb1..566a3b03 100644
--- a/docs/source/contributor-guide/ffi.md
+++ b/docs/source/contributor-guide/ffi.md
@@ -261,10 +261,9 @@ wrote it and strips the tag before handing the bytes back, so your codec receive
 byte for byte, the payload it wrote, and is never offered a payload another codec
 wrote.
 
-Do not add defensive prefix checks to guard against another library's payloads.
-They are unnecessary here, and a codec cannot do it reliably in any case: a `prost`
-message decodes cleanly from an unrelated message's bytes whenever their leading
-field numbers and wire types line up.
+A codec that also ships to hosts which dispatch differently may still want its own
+guard against foreign payloads. Keeping one is fine; it is simply not needed for the
+datafusion-python path.
 
 Codec identity is derived automatically and is stable across processes:
 
@@ -333,6 +332,12 @@ buffer is read as "no opinion" and passes the object to the next codec.
 `NameOnlyUdfCodec` in the FFI example is the worked case. Anything no installed
 codec claims falls through to `Default{Logical,Physical}ExtensionCodec`.
 
+This is the one case where your decoder is consulted about something you may not
+own, because an empty payload carries no identity to route on. `try_decode_udf` and
+its aggregate and window siblings can therefore be called with an empty `buf` and a
+`name` belonging to another library. Decide from `name` and return an error if it is
+not yours; do not assume `buf` is non-empty.
+
 The framing itself — the envelope layout, the identity dispatch, and the two cases
 that stay unframed — is internal to datafusion-python and documented in
 `crates/core/src/codec.rs` for anyone changing it.


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org