namanjain24-sudo opened a new pull request, #25318:
URL: https://github.com/apache/datafusion/pull/25318
## Which issue does this PR close?
- Closes #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, the context cannot cross the boundary, so the provider
decodes with a root context through `try_decode_with_ctx`. 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](https://github.com/apache/datafusion/blob/main/docs/source/contributor-guide/api-health.md).
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 `ScalarSubqueryExec`s: 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`.
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` (120). `./dev/rust_lint.sh` is clean.
## Are there any user-facing changes?
A new provided method on `PhysicalExtensionCodec`. It is not a breaking
change: existing implementations keep compiling and behave the same.
--
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]
