wjones127 commented on code in PR #5070:
URL: https://github.com/apache/arrow-rs/pull/5070#discussion_r1391386468
##########
arrow-data/src/ffi.rs:
##########
@@ -282,6 +286,27 @@ impl FFI_ArrowArray {
// If dictionary is not null should be valid for reads of `Self`
unsafe { self.dictionary.as_ref() }
}
+
+ /// Create a copy of an existing `FFI_ArrowArray`
+ ///
+ /// As required by the C Data Interface specification, this sets the
`release` member of `Self`
+ /// to `None`, but without calling the release callback.
+ pub fn copy(&mut self) -> Self {
+ let new = Self {
+ length: self.length,
+ null_count: self.null_count,
+ offset: self.offset,
+ n_buffers: self.n_buffers,
+ n_children: self.n_children,
+ buffers: self.buffers,
+ children: self.children,
+ dictionary: self.dictionary,
+ release: self.release,
+ private_data: self.private_data,
+ };
+ self.release = None;
+ new
Review Comment:
Do we still use this?
Is this any better than just `std::mem::replace(schema,
FFI_ArrowSchema::empty())`?
##########
arrow/src/pyarrow.rs:
##########
@@ -118,8 +118,39 @@ fn validate_class(expected: &str, value: &PyAny) ->
PyResult<()> {
Ok(())
}
+fn validate_pycapsule(capsule: &PyCapsule, name: &str) -> PyResult<()> {
+ let capsule_name = capsule.name()?;
+ if capsule_name.is_none() {
+ return Err(PyValueError::new_err(
+ "Expected schema PyCapsule to have name set.",
+ ));
+ }
+
+ let capsule_name = capsule_name.unwrap().to_str()?;
+ if capsule_name != name {
+ return Err(PyValueError::new_err(format!(
+ "Expected name '{}' in PyCapsule.",
+ name,
Review Comment:
Always nice to have the actual value whenever possible.
```suggestion
"Expected name '{}' in PyCapsule, instead got '{}'",
name, capsule_name
```
##########
arrow/src/pyarrow.rs:
##########
Review Comment:
Potential refactor for later, but it would be nice to move the Capsule
imports to their own functions that aren't named as if to by PyArrow-specific.
Right now this module is named for PyArrow, but the import logic will apply to
other Arrow implementations as well.
##########
arrow-schema/src/ffi.rs:
##########
@@ -351,6 +355,9 @@ impl Drop for FFI_ArrowSchema {
}
}
+unsafe impl Send for FFI_ArrowSchema {}
+unsafe impl Sync for FFI_ArrowSchema {}
Review Comment:
Are we able to just add `Send`. I'm more confident that is true than `Sync`.
--
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]
