timsaucer commented on code in PR #1678:
URL:
https://github.com/apache/datafusion-python/pull/1678#discussion_r3905881375
##########
crates/core/src/codec.rs:
##########
@@ -223,32 +224,129 @@ fn strip_wire_header<'a>(
Ok(Some(&buf[py_minor_idx + 1..]))
}
+/// Run `f` against each codec in `chain`, returning the first `Ok`.
+///
+/// A codec signals "not mine" by returning an error, so the chain
+/// keeps trying until a codec succeeds. When every codec fails and the
+/// chain has more than one entry, the errors are aggregated into a
+/// single message — returning only the last error would surface the
+/// terminal `Default*ExtensionCodec` "not provided" message and mask
+/// the more specific diagnostic from an installed codec (e.g. a
+/// 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 {
Review Comment:
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.
Coverage is in
`examples/datafusion-ffi-example/python/tests/_test_logical_extension_codec.py`,
across a real FFI boundary:
`test_decode_dispatches_to_the_codec_that_encoded`,
`test_decode_survives_a_different_install_order`,
`test_decode_names_the_codec_that_is_not_installed`,
`test_installing_a_codec_cannot_hijack_an_earlier_codecs_objects`, and
`test_name_only_codec_round_trips_without_a_payload`. The PR description has
been rewritten to describe this design — the earlier text described the
trial-decode version you reviewed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
