feat: change datafusion-proto to use TaskContext rather thanSessionContext for physical plan serialization

[milenkovicm]

Which issue does this PR close?

• Closes SessionContext propagation in physical plan proto functions #17596 .

Rationale for this change

As described in #17596 replacing SessionContext with TaskContext and aligning PhysicalExtensionCodec::try_decode with other interface changes.

more details at ballista pr apache/datafusion-ballista#1320

What changes are included in this PR?

• changed public good few interfaces in datafusion_proto::physical_plan replacing SessionContext with TaskContext
• removed runtime: &RuntimeEnv, from arguments as RuntimeEnv can be retrieved from TaskContext

Are these changes tested?

current tests

Are there any user-facing changes?

yes, interface in good few public methods changed

Note: if we agree that change make sense I'll refactor methods which have TaskContext and RuntimeEnv such as:

    fn try_into_projection_physical_plan(
        &self,
        projection: &protobuf::ProjectionExecNode,
        ctx: &TaskContext,
        runtime: &RuntimeEnv,
        extension_codec: &dyn PhysicalExtensionCodec,
    ) -> Result<Arc<dyn ExecutionPlan>> {

as task context hasn runtime env as well

[milenkovicm]

cc @Omega359 slight change of #16970

[Omega359]

I don't know enough about the requirements of proto code to be able to review this PR wrt whether TaskContext is suitable or not.

[milenkovicm]

All right @Omega359, thanks! I was just adding you as I was looking at the git log; it looked to me that you've made the change of parameter.

[alamb]

@lewiszlw could you help review this PR?

[alamb]

@crepererum , @gabotechs and @Dandandan @avantgardnerio I think you use datafusion-proto heavily too -- do you have any thoughts about this API change?

[milenkovicm]

just adding ballista pr which triggered this change apache/datafusion-ballista#1320

[gabotechs]

👍 the changes look reasonable. I don't think we've struggled with the same challenges as Ballista when upgrading to v50, but passing a TaskContext instead of a SessionContext doesn't sound like an extra burden, and the APIs look cleaner as now the RuntimeEnv can be omitted.

Also, a red LOC PR is always welcome!

[milenkovicm]

as mentioned in #17650 (comment)

Looking at the FunctionRegisty usage, if we ignore proto submodule, its not really used as parameter anywhere. and with #17596 question is to be asked do we need it at all ?

[lewiszlw]

I'm using ballista-variant in my work, so I'm ok with this change.
Could you @milenkovicm try to remove dependency datafusion from datafusion-proto? This will be very helpful for some third-party crates in reducing compile time.

[milenkovicm]

@lewiszlw removing direct datafusion dependency may make sense, I'd have to have a look into package structure, but i'm afraid it may need some datafusion module reorganisation. Lets take that as follow up

[alamb]

@lewiszlw removing direct datafusion dependency may make sense, I'd have to have a look into package structure, but i'm afraid it may need some datafusion module reorganisation. Lets take that as follow up

Filed

• remove datafusion dependency from datafusion-proto #17713

[crepererum]

I think you use datafusion-proto heavily too -- do you have any thoughts about this API change?

I don't really, but the change looks good to me 👍

[alamb]

Makes sense to me -- thank you @milenkovicm

Maybe we could add a brief note to the upgrade guide in https://github.com/apache/datafusion/blob/main/docs/source/library-user-guide/upgrading.md#datafusion-5100 to show the difference

(Basically users will have to change

-            let plan2 = physical_plan_from_bytes(&bytes, &ctx)?;
+            let plan2 = physical_plan_from_bytes(&bytes, &ctx.task_ctx())?;

[alamb]

It makes sense to only pass a subset (TaskContext) here 👍

[milenkovicm]

Thanks @alamb will update

[milenkovicm]

upgrade guide updated

[alamb]

🚀