andygrove commented on code in PR #2619:
URL:
https://github.com/apache/datafusion-ballista/pull/2619#discussion_r4406551344
##########
python/src/ffi_planner_workaround.rs:
##########
@@ -182,11 +189,23 @@ unsafe extern "C" fn create_physical_plan(
}
}
+/// Family prefixes of inline Python UDF payloads written by
+/// [`PythonLogicalCodec`], mirroring datafusion-python's `pub(crate)`
constants.
+const PY_SCALAR_UDF_FAMILY: &[u8] = b"DFPYUDF";
Review Comment:
Since these mirror `pub(crate)` constants in datafusion-python, would you
mind opening an issue there to expose them? Then we can link it here and drop
the copies once it lands.
##########
python/README.md:
##########
@@ -81,7 +81,9 @@ Known limitations and inefficiencies of the current approach:
- A `SessionConfig` passed as `config` is used locally only. Use
`cluster_config` for settings that
should reach the cluster.
- Anything requiring custom
`datafusion_proto::logical_plan::LogicalExtensionCodec`.
-- No support for `UDF` as DataFusion Python does not serialise them.
+- Python UDFs (scalar, aggregate and window) are serialised with the plan
using `cloudpickle`, so
Review Comment:
Could we add a short note here about trust? The scheduler and executors
started from Python now run `cloudpickle.loads` on whatever UDF payloads a
client sends, so anyone who can reach the scheduler port can run arbitrary
Python on the cluster. That's the same model as PySpark, but I think it's worth
spelling out so people don't expose a cluster to untrusted clients.
It might also be worth saying this only works on clusters started from
Python (`BallistaScheduler` / `BallistaExecutor`). The stock Rust binaries and
Docker images won't be able to decode these UDFs.
##########
python/src/cluster.rs:
##########
@@ -42,7 +45,15 @@ impl PyScheduler {
#[pyo3(signature = (bind_host=None, bind_port=None))]
#[new]
pub fn new(_py: Python, bind_host: Option<String>, bind_port: Option<u16>)
-> Self {
- let mut config = SchedulerConfig::default();
+ let mut config = SchedulerConfig {
+ override_logical_codec: Some(Arc::new(
Review Comment:
Not for this PR, but datafusion-python has `with_python_udf_inlining(false)`
on its codec. A flag on `BallistaScheduler` and `BallistaExecutor` to turn
decoding off could be a nice follow-up for people who don't need Python UDFs
and want the codec to refuse pickled payloads.
--
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]
