feat: give PhysicalExtensionCodec the full decode context by namanjain24-sudo · Pull Request #25318 · apache/datafusion

namanjain24-sudo · 2026-09-15T08:21:46Z

Which issue does this PR close?

Closes PhysicalExtensionCodec should take a PhysicalPlanDecodeContext instead of a TaskContext #25089.

Rationale for this change

A custom ExecutionPlan whose codec decodes its own expressions fails to deserialize when one of those expressions is a ScalarSubqueryExpr. It fails even when the plan sits inside the ScalarSubqueryExec that owns the results:

Internal("ScalarSubqueryExpr can only be deserialized as part of a surrounding ScalarSubqueryExec")

The decoder does track the results of the enclosing ScalarSubqueryExec in PhysicalPlanDecodeContext. But PhysicalExtensionCodec::try_decode only receives ctx.task_ctx(), so a codec has to build a fresh PhysicalPlanDecodeContext::new(..) to decode its expressions, and that fresh context has no results.

What changes are included in this PR?

This takes the non-breaking option from the issue:

PhysicalExtensionCodec::try_decode_with_ctx receives the full PhysicalPlanDecodeContext. Its default implementation calls try_decode(ctx.task_ctx()), so existing codecs compile and behave as before.
Extension plans are now decoded through try_decode_with_ctx.
ComposedPhysicalExtensionCodec forwards try_decode_with_ctx to the codec that encoded the node. Without this, composing codecs would drop the context again.
On the FFI side, FFI_PhysicalExtensionCodec gains a try_decode_with_ctx function pointer that also forwards the caller's active scalar-subquery results scope (if any) across the boundary through a new FFI_ScalarSubqueryResults handle, so a ScalarSubqueryExpr decoded by a foreign codec shares the same populated results as the host's ScalarSubqueryExec. A codec that only implements the new method therefore also works through FFI. For codecs that only implement try_decode, nothing changes.

I went with the new method rather than changing try_decode's signature because of the API health policy. The breaking change is simpler if reviewers prefer it, and I'm happy to switch.

This touches the same function as the draft #24631, which adds a per-type registry in front of the codec. That PR's registry path already passes decoders the full context (through ConverterPlanDecoder), but its codec fallback still calls try_decode(ctx.task_ctx()), which is the line this PR changes. Whichever lands second needs a one-line rebase.

What is the testing strategy for this PR?

New tests in datafusion/proto/tests/cases/plans/scalar_subquery.rs use an extension plan whose codec decodes its own expressions through try_decode_with_ctx. Its try_decode returns an error, so a caller that drops the context fails the test:

a ScalarSubqueryExpr inside the extension plan shares the enclosing ScalarSubqueryExec's results, with both DefaultPhysicalProtoConverter and DeduplicatingProtoConverter
the same through ComposedPhysicalExtensionCodec
nested ScalarSubqueryExecs: each expression binds to its own scope and not to the other
without an enclosing ScalarSubqueryExec, decoding still fails with the existing error

A new FFI test checks that a codec that only decodes through try_decode_with_ctx works behind FFI_PhysicalExtensionCodec. A second, cross-library integration test (datafusion/ffi/tests/ffi_physical_extension_codec.rs, gated by the integration-tests feature) builds that codec inside a separately dlopen'd copy of the datafusion-ffi cdylib, so the scalar-subquery results handle crosses a genuine FFI boundary and exercises ForeignScalarSubqueryResultsBackend for real, rather than only the in-process mock_foreign_marker_id path the unit test uses.

I checked that each part of the change is needed:

The first test fails on main with the error from the issue.
With the call site reverted to try_decode, all four new proto tests fail.
With ComposedPhysicalExtensionCodec::try_decode_with_ctx removed, only the composed-codec test fails.
With the FFI wrapper reverted, the new FFI test fails.

Existing suites pass: datafusion-proto (17 lib, 265 integration, 4 doc tests) and datafusion-ffi (123 lib tests, plus the new cross-library test under --features integration-tests). ./dev/rust_lint.sh is clean.

Are there any user-facing changes?

This has two separate kinds of API impact:

Rust API (non-breaking): PhysicalExtensionCodec::try_decode_with_ctx is a new provided method. Existing implementations keep compiling and behave the same.
FFI ABI (breaking): FFI_PhysicalExtensionCodec is #[repr(C)] and gains a try_decode_with_ctx function-pointer field, appended after the existing fields so a producer compiled against the old layout still sees every field it knows about at its original offset. The struct's size still changes, so this is an ABI break: a producer and a consumer built against different layouts of this struct must not be mixed — both sides need to be rebuilt against the same DataFusion version. This should carry the api change label; I don't have permission to add labels on this repo, so flagging it here for a maintainer to apply.

The same two-sided split applies to the FFI_ScalarSubqueryResults handle added for the scalar-subquery results forwarding described above.

PhysicalExtensionCodec::try_decode only receives the TaskContext, so a codec that decodes its own nested expressions has to build a fresh PhysicalPlanDecodeContext. That context has no scalar subquery results, so a ScalarSubqueryExpr inside an extension plan fails to deserialize even under the ScalarSubqueryExec that owns the results. Add try_decode_with_ctx, which receives the full PhysicalPlanDecodeContext and by default calls try_decode. Decode extension plans through it, forward it in ComposedPhysicalExtensionCodec, and call it with a root context on the FFI provider side.

codecov-commenter · 2026-09-18T03:19:31Z

Codecov Report

❌ Patch coverage is 76.00000% with 120 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.85%. Comparing base (b645de0) to head (febd806).
⚠️ Report is 167 commits behind head on main.

Files with missing lines	Patch %	Lines
...tafusion/ffi/src/proto/physical_extension_codec.rs	71.53%	65 Missing and 15 partials ⚠️
...atafusion/ffi/src/proto/scalar_subquery_results.rs	80.57%	12 Missing and 15 partials ⚠️
datafusion/expr/src/physical_planning_context.rs	77.96%	12 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #25318    +/-   ##
========================================
  Coverage   82.85%   82.85%            
========================================
  Files        1151     1152     +1     
  Lines      456012   456485   +473     
  Branches   456012   456485   +473     
========================================
+ Hits       377824   378222   +398     
- Misses      55235    55274    +39     
- Partials    22953    22989    +36

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

namanjain24-sudo · 2026-09-28T08:34:59Z

Bumping this — still open for review whenever someone has bandwidth.

kosiew

@namanjain24-sudo,

Thanks for working on this. The context-aware decode path looks good for local codecs, but the FFI path still loses the active scalar subquery scope. I left one blocking comment and one small test-naming suggestion.

…ndary ForeignPhysicalExtensionCodec now forwards the active ScalarSubqueryResults scope through an ABI-safe FFI_ScalarSubqueryResults handle instead of dropping it, so a ScalarSubqueryExpr decoded by a genuinely foreign codec shares the same results as the surrounding ScalarSubqueryExec. ScalarSubqueryResults gains an alternate backend representation (ScalarSubqueryResultsBackend) to support this proxy, with ScalarValues crossing the boundary as prost-encoded bytes like the rest of the codec's wire format. Adds a forced-foreign end-to-end ScalarSubqueryExec roundtrip test, and clarifies the legacy-decode test's name/intent.

namanjain24-sudo · 2026-10-02T16:58:37Z

@kosiew addressed: ForeignPhysicalExtensionCodec now forwards the active scalar subquery results scope through an ABI-safe FFI_ScalarSubqueryResults handle (values cross as prost-encoded bytes), and added a forced-foreign end-to-end ScalarSubqueryExec roundtrip test. Also renamed the legacy-decode test per your suggestion.

- Move the new FFI_PhysicalExtensionCodec::try_decode_with_ctx field to the end of the repr(C) struct instead of inserting it in the middle, so every pre-existing field keeps its original offset. - Add RefUnwindSafe as a supertrait bound on ScalarSubqueryResultsBackend so ScalarSubqueryResults keeps implementing UnwindSafe/RefUnwindSafe, with a regression test pinning it.

namanjain24-sudo · 2026-10-02T17:40:34Z

Fixed both: moved try_decode_with_ctx to the end of the repr(C) struct so no existing field shifts, and added RefUnwindSafe as a supertrait bound on ScalarSubqueryResultsBackend so ScalarSubqueryResults keeps its auto traits (pinned with a test).

CI's clippy (ci/scripts/rust_clippy.sh, not --all-features) flagged encode_scalar_value for taking ScalarValue by value while only borrowing it. Takes &ScalarValue instead.

namanjain24-sudo · 2026-10-04T13:51:33Z

CI clippy caught needless_pass_by_value on encode_scalar_value (it only borrowed the ScalarValue) — fixed, and verified with ./ci/scripts/rust_clippy.sh locally (clean across the whole workspace).

kosiew

@namanjain24-sudo,

Thanks for working through the earlier feedback. The decode-context forwarding and related cleanup look improved, but there is still a type-preservation issue in the new FFI scalar-subquery results bridge that needs to be fixed before this is safe to merge.

…uery bridge datafusion_proto::protobuf::ScalarValue has no dedicated variant for every DataType: a non-null Float16 is encoded as Float32Value, so a foreign ScalarSubqueryExpr reading it back got a Float32 instead. Wrap the encoded bytes with the original DataType (as an ArrowType) and cast back to it on decode, so the value crossing FFI always matches the type it was declared with. Adds forced-foreign non-null Float16 get/set regression coverage.

kosiew

@namanjain24-sudo,

Thanks for the updates. The Float16 transport issue is fixed, and the context forwarding is in place. I still have one FFI integration coverage gap that I think should be addressed before merging, plus an ABI metadata/documentation issue.

…bridge The existing unit test only forces the codec foreign through mock_foreign_marker_id, so the results handle still takes the local-bypass fast path and never exercises ForeignScalarSubqueryResultsBackend. Adds an integration-tests-gated test that decodes inside a separately dlopen'd copy of the cdylib, so the handle genuinely crosses the boundary. Shares the scalar-subquery fixtures between the unit test and the new integration test via a fixtures module gated on cfg(any(test, feature = "integration-tests")). Also flags the FFI ABI break from the appended try_decode_with_ctx field for a maintainer to label, since the struct's size changes even though existing try_decode-only implementations keep compiling.

kosiew

@namanjain24-sudo,

Thanks for the follow-up changes. The scope forwarding, Float16 type preservation, and ABI documentation have improved.

The new cross-library test confirms that the remote results backend can read host-populated values, but it still bypasses the production context-aware FFI codec callback. Please extend the test to exercise that callback with the active remote results handle, while retaining the host-value assertion.

…ss-library test The cross-library integration test decoded through a bespoke test-only entry point instead of the real FFI_PhysicalExtensionCodec callback a foreign codec actually goes through, so it never exercised try_decode_with_ctx_fn_wrapper or ForeignPhysicalExtensionCodec itself. Now it builds the codec inside the dlopen'd cdylib, converts it to Arc<dyn PhysicalExtensionCodec> (confirming it is really foreign), and calls try_decode_with_ctx on that Arc directly - the same call any caller of a foreign codec makes. The decoded plan comes back as an opaque ForeignExecutionPlan, so the host-value assertion now goes through execute() instead of a downcast: ScalarSubqueryExprExec gets a real execute() that evaluates its expression and emits the result as a RecordBatch, giving the host-populated value a way to cross back out through the FFI_ExecutionPlan/FFI_RecordBatchStream machinery every ExecutionPlan already has, without a generic FFI_PhysicalExpr wrapper. ScalarSubqueryExprExec gains a dedicated single-column PlanProperties (no longer delegating to its child's) and a constructor that computes it, used by the fixture's own decode path and both tests.

kosiew

@namanjain24-sudo,

Thanks for addressing the earlier feedback. The context-aware extension decoding now preserves the active scalar-subquery results scope, including composed and foreign codecs.

The additional Float16 regression tests cover type preservation across the FFI boundary, and the cross-library test verifies that a foreign plan can access host-populated results. The PR also documents the breaking FFI layout change and the need to rebuild both sides.

The changes look good to me. Approved!

…e-context-sync # Conflicts: # datafusion/proto/src/physical_plan/mod.rs

alamb · 2026-10-16T14:08:26Z

@thinkharderdev -- can you please review this PR as you requested / suggested the original feature?

alamb · 2026-10-24T09:58:31Z

Let's put this as drat until @thinkharderdev has a chance to review and confirm this is what he had in mind. Thank you @kosiew for the review

# Conflicts: # datafusion/ffi/integration-tests/tests/cases/ffi_physical_extension_codec.rs

…s crate main moved the FFI cross-library tests out of `datafusion/ffi/tests` and into the `datafusion-ffi-integration-tests` crate, where each case is a module under `tests/cases` that `tests/ffi_integration.rs` gates behind the `integration-tests` feature. The merge carried this file to the new directory but left it shaped for the old layout. Drop the per-file `#[cfg(feature = "integration-tests")]`, since `mod cases` is already gated, register the module in `tests/cases/mod.rs`, and name the file after its subject like its siblings. The test needs `ScalarSubqueryExpr`, so add `datafusion-physical-expr` to the crate's dev-dependencies.

namanjain24-sudo · 2026-10-26T12:34:06Z

Synced with main. The FFI cross-library tests moved into their own crate (#26564) while this sat, so the new test went to datafusion/ffi/integration-tests/tests/cases/ with its siblings — otherwise it would have landed in a directory that no longer exists. No change to the feature itself; still waiting on @thinkharderdev.

namanjain24-sudo · 2026-10-26T20:38:58Z

Still no word from @thinkharderdev, so this stays in draft as you suggested — no rush from my side.

If there's someone else who could confirm the API shape is what was intended, happy to be pointed at them; otherwise I'll leave it parked.

alamb · 2026-10-27T17:34:47Z

Thanks for the flexibility @namanjain24-sudo

This is a case where I want to make sure that if we add an API it is actually usable by users / isn't going to just bitrot / be unused.

I appreciate you sticking with it

namanjain24-sudo · 2026-11-04T18:19:46Z

Added the 56.0.0 upgrade-guide entry for the api change label.

@thinkharderdev — ping when you have a moment: this is the decode context from #25089, and @kosiew has approved it. @alamb if someone else is a better reviewer, happy to redirect.

… version check

thinkharderdev · 2026-11-06T15:30:17Z

Apologies, I somehow missed this but will review today

Upstream Issue: apache#25089 Decoding `ScalarSubqueryExpr` inside an extension `ExecutionPlan` node fails because we don't have the full context.

thinkharderdev

I'm out of the loop on the FFI stuff but it looks like it has been reviewed already. The other changes lgtm though. Thanks!

…e-context

namanjain24-sudo · 2026-11-07T12:55:13Z

Synced with main. The license header check had gone red: main switched to the short SPDX header in #27049, so this PR's two new files carry that form now.

alamb · 2026-11-07T13:32:08Z

Thanks @namanjain24-sudo @thinkharderdev and @kosiew

alamb

Hi @namanjain24-sudo -- thank you for this PR and thank you for the reviews @thinkharderdev and @kosiew

I went through some of it and I think this PR is much too verbose for what it is doing. There are many seemingly AI generated comments that restate the changes in the PR or re-describe the code for no additional gain. I also have a few questions on the design

Can you please go through (yourself) and clean up the comments?

Also, I left some additional questions -- could you please help me understand this a bit more? There are several design decisions that seem like they are pretty narrow and not well thought through (like ScalarValueEnvelope)

alamb · 2026-11-07T13:33:21Z


 See [issue #24626](https://github.com/apache/datafusion/issues/24626) for details.

+### `PhysicalExtensionCodec` can decode with the full decode context


actually since we cut the brach, this should now go into the 57 upgrade guide

alamb · 2026-11-07T13:34:03Z


+### `PhysicalExtensionCodec` can decode with the full decode context
+
+`PhysicalExtensionCodec::try_decode` received a `TaskContext`, which carries no


I din't think we have to reiterate the problem here -- we need to just say it didn't have enough context and then refer to the issue for more details

alamb · 2026-11-07T13:34:31Z

+- `ComposedPhysicalExtensionCodec` forwards `try_decode_with_ctx` to the codec
+  that wrote the payload, so composing codecs preserves the context.
+
+**FFI users must rebuild both sides.** `FFI_PhysicalExtensionCodec` gained a


is this specific to this change? It seem generic to all FFI

alamb · 2026-11-07T13:35:49Z

+}
+```
+
+**Who is affected:**


Can you please trim this down from AI speak

For example this first sentence doesn't need to restate how the code works, it can just say "no changes are required"

The second can say "if you need the additional context, use the new function"

The detail about ComposedPhysical... is an implementation detail that isn't relevant for implmenters, is it?

alamb · 2026-11-07T13:36:14Z

+`datafusion_ffi::version()` reports changes with it: a provider and host that
+compare it, which is what that function is for, see the mismatch instead.
+
+`ScalarSubqueryResults` is no longer backed only by in-process state. It can be


is this important to state? This just reiterates how things are implemented. I think we could remove most/all of this last paragraph

alamb · 2026-11-07T13:41:32Z

+        DefaultPhysicalProtoConverter, PhysicalExtensionCodec, PhysicalPlanDecodeContext,
+    };
+
+    /// The decode context's active scalar-subquery results scope must reach


Please cut this comment down to explain the purpose of the test, rahter than restating in english how it is implemented

alamb · 2026-11-07T13:42:30Z

+/// external integration-test binary, which links this crate as an ordinary
+/// dependency rather than compiling it with `cfg(test)`, can still reach
+/// these types through `datafusion_ffi::proto::physical_extension_codec::fixtures`.
+#[cfg(any(test, feature = "integration-tests"))]


Why is this in the main ffi crate (and not in the test binary?)? This doesn't make sense to me

alamb · 2026-11-07T13:43:15Z

+    /// always decodes with a root context (no scope), so the embedded
+    /// `ScalarSubqueryExpr` fails to deserialize.
+    ///
+    /// This only mocks the foreign path in-process (see


similar, please trim this comment to describe what this is testing, not restate how it is implemented

Also, why is this test needed if we already have a integration test?

alamb · 2026-11-07T13:43:58Z

+/// A stable struct for sharing the shared results container of a
+/// `ScalarSubqueryExec` across FFI boundaries.
+///
+/// Unlike most FFI wrappers in this crate, which stand in for a whole


Why is it important that this is different? Please clarify this comment

And remove details about its implementation -- instead focus on its relevance

alamb · 2026-11-07T13:44:51Z

+/// The wire form a [`ScalarValue`] crosses the FFI boundary in.
+///
+/// `datafusion_proto::protobuf::ScalarValue` alone is not a type-preserving
+/// transport: it has no dedicated variant for every `DataType` (`Float16`,


THis seems like a rationale for expanding the coverage of datafusion_proto::protobuf::ScalarValue, ot adding a whole new wrapper

alamb · 2026-11-07T13:46:17Z

Also the comments say "give the full decode context" but this seems mostly to be about adding an API to pass scalar subquery results around 🤔

… the upgrade entry to 57

namanjain24-sudo · 2026-11-07T17:20:24Z

Thanks for the detailed review. The comments were helpful. I've gone through each one and the PR is now 210 lines smaller.

The enum: Removed. ScalarSubqueryResults now just holds an Arc<dyn ScalarSubqueryResultsBackend>, with the in-memory slots implementing the same trait as any other backend. The "remote" distinction is gone too.

The in-crate forced_foreign test: Dropped. You're right that it was duplicating the cross-library test. That cross-library test runs in cargo test (macos-aarch64), which is skipped for fork PRs but runs in the merge queue.

Fixtures in the FFI crate: These need to stay in the crate because the test loads this crate's cdylib and asks it for the codec. The cdylib needs to be able to build the fixtures itself; types defined only in the test binary aren't available inside it. I've narrowed the gate from any(test, feature = "integration-tests") to just the feature.

The upgrade note: Moved to a new 57.0.0.md. It now simply says the context wasn't sufficient and points to #25089 for the details. The "who is affected" section is also down to the two lines you suggested. I removed the ComposedPhysicalExtensionCodec detail, the FFI rebuild paragraph (since that's generic to FFI layout changes), and the closing paragraph about the results container.

The remaining comments: Trimmed them down to explain the purpose of each thing: the FFI field, the cross-library test, and FFI_ScalarSubqueryResults. For the last one, I also removed the "unlike most FFI wrappers" comparison rather than adding more explanation that doesn't really help the reader.

There are two things I'd still like your input on:

ScalarValueEnvelope: Your diagnosis is right. ScalarValue::Float16 is currently written into float32_value, so a round trip widens the type. Expanding protobuf::ScalarValue seems like the right fix, but that would mean changing a shared wire format. Would you prefer that I make that change in this PR, or file it separately and remove the envelope once it lands?

The framing: I agree that the PR's framing could be clearer. The trait change itself is 42 changed lines in datafusion/proto/src/physical_plan/mod.rs. The FFI transport needed to carry the context across is another 848 lines, plus 315 lines of tests. Would it make more sense to land the trait change separately and bring the FFI part in a follow-up?

alamb

thanks -- this is getting closer but still has a lot of unecessary complexity -- please re-read the whole thing before asking for a re-review and either call out or clean up anything you don't understand

alamb · 2026-11-08T16:04:16Z


+/// An extension plan and codec for the cross-library integration test.
+///
+/// These live in this crate because the test `dlopen`s this crate's cdylib and


I don't understand this

Why can't it be in datafusion/ffi/integration-tests/tests/cases/physical_extension_codec.rs ?

This seems a lot of code to be testing the availablity of subquery contexts over FFI

alamb · 2026-11-08T16:04:53Z

+}
+
+#[cfg(test)]
+mod tests {


what do these tests cover that the integration tests do not?

alamb · 2026-11-08T16:06:24Z

    FFI_ExecutionPlan::new(plan, None)
 }

+/// Builds a [`FFI_PhysicalExtensionCodec`] wrapping


Have you read these comments carefully? It seems like they are restating the details of how the function works -- before asking for my review again, please carefully read all the code and comments your LLM has written and determine if it is needed or not

…ation test covers

namanjain24-sudo · 2026-11-10T07:04:40Z

Thanks - that was a fair call. I went back through the whole diff rather than just the three spots, and removed 55 lines of comments plus the code below. Net 175 lines out, 20 in.

The comments. You were right that they were restating the code. The block you pointed at on datafusion_ffi_test_create_scalar_subquery_expr_exec_codec went from 31 lines to 7 - it had also gone stale, still referring to a unit test I had deleted. Three inline comments in the cross-library test were saying what the test's own doc comment already said, or narrating the code directly underneath them; those are gone. What is left is the reasoning that is not in the code: why execute evaluates the expression directly instead of pulling a batch from its child, and why try_decode_fn_wrapper builds a root context.

I also stopped exposing the fixtures' internals - ScalarSubqueryExprExecProto and two struct fields were pub for no reason.

The two tests in scalar_subquery_results.rs. Both gone. One covered ScalarValueEnvelope, which is also gone - I took your earlier point and dropped it, so a Float16 now widens to Float32 across the boundary like it does anywhere else protobuf::ScalarValue is used. That file is down from 294 lines to 177.

One honest gap: the other test also exercised set and clear from the foreign side, and the integration test does neither. In the real flow the host populates the results and the far side only reads, so I do not think they are worth their own test, but say the word and I will put it back.

The fixtures' location. They cannot live in integration-tests/tests/cases/physical_extension_codec.rs - that is the test binary, and the test loads a cdylib. But I was wrong in my last reply about which cdylib: find_cdylib looks for libdatafusion_ffi_integration_tests, not this crate's. So they could live in that crate's src/lib.rs, which is what gets built into the cdylib.

The cost is that datafusion-ffi-integration-tests has exactly one entry in [dependencies] today; the fixtures would need seven of its dev-dependencies promoted, plus prost added. Keeping them here follows datafusion_ffi_test_create_exec_with_byte_metrics, which sits in datafusion_ffi::tests and reaches the cdylib the same way. Happy to move them if you would rather the test code not sit in the published crate - it is your call and I did not want to guess.

On the volume. The fixtures module is 195 lines, and about 100 of those are the plan itself. Its ExecutionPlan impl is 58 of them, and six of the seven methods it implements are required by the trait. The part that could genuinely go is the plan altogether - the codec could assert inside the cdylib and return a trivial plan, which would cut those ~100 lines at the cost of a weaker assertion.

And the structural version of the same question, which I asked last time and is still open: the trait change itself is 42 lines in datafusion/proto/src/physical_plan/mod.rs. I am happy to land that alone and bring the FFI transport as a separate PR, if that is an easier thing to review.

github-actions Bot added proto Related to proto crate ffi Changes to the ffi crate labels Sep 15, 2026

namanjain24-sudo force-pushed the extension-codec-decode-context branch from a384c28 to cf5431e Compare September 18, 2026 02:58

kosiew requested changes Oct 2, 2026

View reviewed changes

Comment thread datafusion/ffi/src/proto/physical_extension_codec.rs Resolved

Comment thread datafusion/ffi/src/proto/physical_extension_codec.rs Resolved

github-actions Bot added logical-expr Logical plan and expressions auto detected api change Auto detected API change labels Oct 2, 2026

github-actions Bot removed the auto detected api change Auto detected API change label Oct 2, 2026

fix: clippy needless_pass_by_value in encode_scalar_value

dd38c1c

CI's clippy (ci/scripts/rust_clippy.sh, not --all-features) flagged encode_scalar_value for taking ScalarValue by value while only borrowing it. Takes &ScalarValue instead.

kosiew requested changes Oct 6, 2026

View reviewed changes

Comment thread datafusion/ffi/src/proto/scalar_subquery_results.rs Resolved

kosiew requested changes Oct 7, 2026

View reviewed changes

Comment thread datafusion/ffi/src/proto/physical_extension_codec.rs Resolved

Comment thread datafusion/ffi/src/proto/physical_extension_codec.rs Resolved

kosiew requested changes Oct 9, 2026

View reviewed changes

Comment thread datafusion/ffi/src/tests/mod.rs Resolved

namanjain24-sudo and others added 2 commits October 9, 2026 10:42

Merge branch 'main' into extension-codec-decode-context

09a16ed

kosiew added the api change Changes the API exposed to users of the crate label Oct 12, 2026

kosiew approved these changes Oct 12, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into extension-codec-decod…

febd806

…e-context-sync # Conflicts: # datafusion/proto/src/physical_plan/mod.rs

alamb marked this pull request as draft October 24, 2026 09:58

Merge remote-tracking branch 'origin/main' into ecdc-sync4

40e7bdd

# Conflicts: # datafusion/ffi/integration-tests/tests/cases/ffi_physical_extension_codec.rs

namanjain24-sudo added 2 commits October 25, 2026 20:51

Merge remote-tracking branch 'origin/main' into ecdc-sync4

8a90fe5

docs: add the 56.0.0 upgrade guide entry for the decode context

9a4d36e

github-actions Bot added the documentation Improvements or additions to documentation label Nov 4, 2026

docs: make the decode-context example self-contained and name the FFI…

0ee0d3f

… version check

thinkharderdev approved these changes Nov 6, 2026

View reviewed changes

namanjain24-sudo added 2 commits November 7, 2026 18:23

Merge remote-tracking branch 'origin/main' into extension-codec-decod…

bc9a880

…e-context

chore: move this PR's new files to the short Apache license header

0989f0c

alamb marked this pull request as ready for review November 7, 2026 13:31

alamb added this pull request to the merge queue Nov 7, 2026

alamb removed this pull request from the merge queue due to a manual request Nov 7, 2026

alamb requested changes Nov 7, 2026

View reviewed changes

namanjain24-sudo added 2 commits November 7, 2026 20:54

refactor: address review - drop the results enum, trim comments, move…

095f8b1

… the upgrade entry to 57

docs: trim the upgrade note to the wording asked for in review

ece3cce

alamb reviewed Nov 8, 2026

View reviewed changes

namanjain24-sudo added 3 commits November 10, 2026 02:08

refactor: drop the ScalarValue envelope and the unit tests the integr…

f03608c

…ation test covers

refactor: correct the fixtures comment and stop exposing their internals

217300a

test: drop comments the test's doc comment already covers

2ddb9d1


		See [issue #24626](https://github.com/apache/datafusion/issues/24626) for details.

		### `PhysicalExtensionCodec` can decode with the full decode context


		### `PhysicalExtensionCodec` can decode with the full decode context

		`PhysicalExtensionCodec::try_decode` received a `TaskContext`, which carries no

Conversation

namanjain24-sudo commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

What is the testing strategy for this PR?

Are there any user-facing changes?

Uh oh!

codecov-commenter commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

namanjain24-sudo commented Sep 28, 2026

Uh oh!

kosiew left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

namanjain24-sudo commented Oct 2, 2026

Uh oh!

namanjain24-sudo commented Oct 2, 2026

Uh oh!

namanjain24-sudo commented Oct 4, 2026

Uh oh!

kosiew left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kosiew left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kosiew left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kosiew left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Oct 16, 2026

Uh oh!

alamb commented Oct 24, 2026

Uh oh!

namanjain24-sudo commented Oct 26, 2026

Uh oh!

namanjain24-sudo commented Oct 26, 2026

Uh oh!

alamb commented Oct 27, 2026

Uh oh!

namanjain24-sudo commented Nov 4, 2026

Uh oh!

thinkharderdev commented Nov 6, 2026

Uh oh!

thinkharderdev left a comment

Choose a reason for hiding this comment

Uh oh!

namanjain24-sudo commented Nov 7, 2026

Uh oh!

alamb commented Nov 7, 2026

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

namanjain24-sudo commented Sep 15, 2026 •

edited

Loading

codecov-commenter commented Sep 18, 2026 •

edited

Loading

kosiew left a comment •

edited

Loading