kevinjqliu commented on code in PR #1426:
URL:
https://github.com/apache/datafusion-python/pull/1426#discussion_r2934419505
##########
examples/datafusion-ffi-example/src/utils.rs:
##########
@@ -35,30 +34,10 @@ pub(crate) fn ffi_logical_codec_from_pycapsule(
};
let capsule = capsule.cast::<PyCapsule>()?;
- validate_pycapsule(capsule, "datafusion_logical_extension_codec")?;
-
let data: NonNull<FFI_LogicalExtensionCodec> = capsule
- .pointer_checked(Some(c_str!("datafusion_logical_extension_codec")))?
+ .pointer_checked(Some(c"datafusion_logical_extension_codec"))?
.cast();
let codec = unsafe { data.as_ref() };
Ok(codec.clone())
}
-
-pub(crate) fn validate_pycapsule(capsule: &Bound<PyCapsule>, name: &str) ->
PyResult<()> {
- let capsule_name = capsule.name()?;
- if capsule_name.is_none() {
- return Err(PyValueError::new_err(format!(
- "Expected {name} PyCapsule to have name set."
- )));
- }
-
- let capsule_name = unsafe { capsule_name.unwrap().as_cstr().to_str()? };
- if capsule_name != name {
- return Err(PyValueError::new_err(format!(
- "Expected name '{name}' in PyCapsule, instead got '{capsule_name}'"
- )));
- }
Review Comment:
Thanks for pointing out the error messages from
[`PyCapsule_GetPointer`](https://github.com/python/cpython/blob/3c38feb2a21aacdb009eea8baa2a6a3daf4b4932/Objects/capsule.c#L98-L113)!
They do seem fairly similar 😄
I still think it could be helpful to expose `validate_pycapsule` as a public
function for implementers. While `PyCapsule_GetPointer` errors will mostly be
encountered by developers implementing the “magic” methods, having a helper
with clearer validation and error messages could make debugging easier. Since
https://github.com/apache/datafusion-python/pull/1414/files#diff-6dc4fc730855f068c83d61b03b141832727bb30bc71788f763761ef8cd2bde7eR154
introduces a new `datafusion-python-util` crate that includes this helper,
exposing it there could provide implementers with a simple way to validate
capsules before extracting pointers.
--
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]
