timsaucer commented on code in PR #21240:
URL: https://github.com/apache/datafusion/pull/21240#discussion_r3069723739
##########
datafusion/proto/src/physical_plan/mod.rs:
##########
@@ -3792,7 +3894,11 @@ struct DataEncoderTuple {
pub blob: Vec<u8>,
}
-pub struct DefaultPhysicalProtoConverter;
+#[derive(Default)]
+pub struct DefaultPhysicalProtoConverter {
+ scalar_subquery_results: RefCell<Option<ScalarSubqueryResults>>,
Review Comment:
I put up [this PR targeting you
branch](https://github.com/neilconway/datafusion/pull/1) as an explanation of
what I mean.
The problem I have with adding state data to `DefaultPhysicalProtoConverter`
is that now any time we have a custom proto converter that doesn't call the
default, we will not be able to process these scalar subquery results.
Instead I think we just have to plumb this data member through the
deserialization process. I haven't taken a super deep look into exactly how
this ends up getting used to see if there's another way to take advantage. The
method I used in the PR was basically to add a struct that contains all of the
parts we pass through deserialization and add the `scalar_subquery_results` to
it.
In regards to switching from `FunctionRegistry` -> `TaskContext` that's a
great change. It was done part way in recent releases for the physical side but
not on the logical side. It makes perfect sense to do it the way you have on
the logical side.
--
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]
