You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/06/09 17:48:05 UTC
[GitHub] [arrow-rs] kszucs opened a new pull request #439: [WIP] FFI bridge for Schema, Field and DataType
kszucs opened a new pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439
# Which issue does this PR close?
<!---
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
# Rationale for this change
<!---
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!---
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!---
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] kszucs commented on pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#issuecomment-872896494
Thanks everyone! Merging it then.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter edited a comment on pull request #439: [WIP] FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#issuecomment-857974778
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#439](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a2fbd66) into [master](https://codecov.io/gh/apache/arrow-rs/commit/18c804a8f80ada6577d16a6f0fab4b2e262e314d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (18c804a) will **increase** coverage by `0.07%`.
> The diff coverage is `79.50%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #439 +/- ##
==========================================
+ Coverage 82.64% 82.72% +0.07%
==========================================
Files 162 164 +2
Lines 44542 44808 +266
==========================================
+ Hits 36813 37067 +254
- Misses 7729 7741 +12
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/datatypes/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9tb2QucnM=) | `100.00% <ø> (ø)` | |
| [arrow/src/datatypes/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9mZmkucnM=) | `73.03% <73.03%> (ø)` | |
| [arrow/src/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2ZmaS5ycw==) | `86.33% <96.96%> (+3.48%)` | :arrow_up: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9tb2QucnM=) | `86.15% <0.00%> (-3.04%)` | :arrow_down: |
| [arrow/src/compute/kernels/sort.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9zb3J0LnJz) | `94.97% <0.00%> (-0.16%)` | :arrow_down: |
| [arrow/src/compute/kernels/concat.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jb25jYXQucnM=) | `100.00% <0.00%> (ø)` | |
| [arrow/src/compute/kernels/length.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9sZW5ndGgucnM=) | `100.00% <0.00%> (ø)` | |
| [arrow/src/compute/kernels/window.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy93aW5kb3cucnM=) | `100.00% <0.00%> (ø)` | |
| [arrow/src/compute/kernels/partition.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9wYXJ0aXRpb24ucnM=) | `99.19% <0.00%> (ø)` | |
| ... and [9 more](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [18c804a...a2fbd66](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#issuecomment-872563601
@kszucs I took the liberty of rebasing this PR and fixing clippy -- it should be good to go now once the CI passes
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] kszucs commented on pull request #439: [WIP] FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#issuecomment-860523380
Thanks for the review, I'm going to add more tests.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] kszucs edited a comment on pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
kszucs edited a comment on pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#issuecomment-865163720
@jorgecarleitao created the relevant follow-up tickets and cleaned up the PR.
@pitrou didn't notice any outstanding problems, so I think this should be ready to go now.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] kszucs merged pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
kszucs merged pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] jorgecarleitao commented on pull request #439: [WIP] FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#issuecomment-858270139
At the time I had limited knowledge of what I was doing. I think that we could refactor this so that the arrays are shared and the datatypes are copied, since we do not really need to share metadata.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] kszucs edited a comment on pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
kszucs edited a comment on pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#issuecomment-865163720
@jorgecarleitao created the relevant follow-up tickets and cleaned up the PR.
@pitrou didn't notice any outstanding problems, so I think this should be ready to go now.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alippai commented on pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
alippai commented on pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#issuecomment-864401873
> @jorgecarleitao what do you think about implementing `arrow <-> pyarrow` interoperability directly in the `arrow-rs` repository and use that from the datafusion bindings?
@ritchie46 you might be interested in this, as I saw https://github.com/pola-rs/polars/tree/master/py-polars/src/arrow_interop
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] kszucs commented on a change in pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#discussion_r655509305
##########
File path: arrow/src/ffi.rs
##########
@@ -122,66 +132,61 @@ unsafe extern "C" fn release_schema(schema: *mut FFI_ArrowSchema) {
let schema = &mut *schema;
// take ownership back to release it.
- CString::from_raw(schema.format as *mut std::os::raw::c_char);
- CString::from_raw(schema.name as *mut std::os::raw::c_char);
- let private = Box::from_raw(schema.private_data as *mut SchemaPrivateData);
- for child in private.children_ptr.iter() {
- let _ = Box::from_raw(*child);
+ CString::from_raw(schema.format as *mut c_char);
+ if !schema.name.is_null() {
+ CString::from_raw(schema.name as *mut c_char);
+ }
+ if !schema.private_data.is_null() {
+ let private_data = Box::from_raw(schema.private_data as *mut SchemaPrivateData);
+ for child in private_data.children.iter() {
+ drop(Box::from_raw(*child))
+ }
+ drop(private_data);
}
schema.release = None;
}
impl FFI_ArrowSchema {
/// create a new [`Ffi_ArrowSchema`]. This fails if the fields' [`DataType`] is not supported.
- fn try_new(field: Field) -> Result<FFI_ArrowSchema> {
- let format = to_format(field.data_type())?;
- let name = field.name().clone();
-
- // allocate (and hold) the children
- let children_vec = match field.data_type() {
- DataType::List(field) => {
- vec![Box::new(FFI_ArrowSchema::try_new(field.as_ref().clone())?)]
- }
- DataType::LargeList(field) => {
- vec![Box::new(FFI_ArrowSchema::try_new(field.as_ref().clone())?)]
- }
- DataType::Struct(fields) => fields
- .iter()
- .map(|field| Ok(Box::new(FFI_ArrowSchema::try_new(field.clone())?)))
- .collect::<Result<Vec<_>>>()?,
- _ => vec![],
- };
- // note: this cannot be done along with the above because the above is fallible and this op leaks.
- let children_ptr = children_vec
+ pub fn try_new(format: &str, children: Vec<FFI_ArrowSchema>) -> Result<Self> {
+ let mut this = Self::empty();
+
+ // note: this op leaks.
+ let mut children_ptr = children
.into_iter()
+ .map(Box::new)
.map(Box::into_raw)
.collect::<Box<_>>();
- let n_children = children_ptr.len() as i64;
- let flags = field.is_nullable() as i64 * 2;
+ this.format = CString::new(format).unwrap().into_raw();
+ this.release = Some(release_schema);
+ this.n_children = children_ptr.len() as i64;
+ this.children = children_ptr.as_mut_ptr();
- let mut private = Box::new(SchemaPrivateData {
- field,
- children_ptr,
+ let private_data = Box::new(SchemaPrivateData {
+ children: children_ptr,
});
+ this.private_data = Box::into_raw(private_data) as *mut c_void;
- // <https://arrow.apache.org/docs/format/CDataInterface.html#c.ArrowSchema>
- Ok(FFI_ArrowSchema {
- format: CString::new(format).unwrap().into_raw(),
- name: CString::new(name).unwrap().into_raw(),
- metadata: std::ptr::null_mut(),
- flags,
- n_children,
- children: private.children_ptr.as_mut_ptr(),
- dictionary: std::ptr::null_mut(),
- release: Some(release_schema),
- private_data: Box::into_raw(private) as *mut ::std::os::raw::c_void,
- })
+ Ok(this)
+ }
+
+ pub fn with_name(mut self, name: &str) -> Result<Self> {
+ self.name = CString::new(name).unwrap().into_raw();
+ Ok(self)
+ }
+
+ pub fn with_flags(mut self, flags: Flags) -> Result<Self> {
+ self.flags = flags.bits();
+ Ok(self)
}
+ // pub fn with_dictionary() {}
+ // pub fn with_metadata() {}
Review comment:
Metadata: https://github.com/apache/arrow-rs/issues/478
Dictionary: https://github.com/apache/arrow-rs/issues/479
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] kszucs commented on a change in pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#discussion_r655512763
##########
File path: arrow/src/ffi.rs
##########
@@ -122,66 +132,61 @@ unsafe extern "C" fn release_schema(schema: *mut FFI_ArrowSchema) {
let schema = &mut *schema;
// take ownership back to release it.
- CString::from_raw(schema.format as *mut std::os::raw::c_char);
- CString::from_raw(schema.name as *mut std::os::raw::c_char);
- let private = Box::from_raw(schema.private_data as *mut SchemaPrivateData);
- for child in private.children_ptr.iter() {
- let _ = Box::from_raw(*child);
+ CString::from_raw(schema.format as *mut c_char);
+ if !schema.name.is_null() {
+ CString::from_raw(schema.name as *mut c_char);
+ }
+ if !schema.private_data.is_null() {
+ let private_data = Box::from_raw(schema.private_data as *mut SchemaPrivateData);
+ for child in private_data.children.iter() {
+ drop(Box::from_raw(*child))
+ }
+ drop(private_data);
}
schema.release = None;
}
impl FFI_ArrowSchema {
/// create a new [`Ffi_ArrowSchema`]. This fails if the fields' [`DataType`] is not supported.
- fn try_new(field: Field) -> Result<FFI_ArrowSchema> {
- let format = to_format(field.data_type())?;
- let name = field.name().clone();
-
- // allocate (and hold) the children
- let children_vec = match field.data_type() {
- DataType::List(field) => {
- vec![Box::new(FFI_ArrowSchema::try_new(field.as_ref().clone())?)]
- }
- DataType::LargeList(field) => {
- vec![Box::new(FFI_ArrowSchema::try_new(field.as_ref().clone())?)]
- }
- DataType::Struct(fields) => fields
- .iter()
- .map(|field| Ok(Box::new(FFI_ArrowSchema::try_new(field.clone())?)))
- .collect::<Result<Vec<_>>>()?,
- _ => vec![],
- };
- // note: this cannot be done along with the above because the above is fallible and this op leaks.
- let children_ptr = children_vec
+ pub fn try_new(format: &str, children: Vec<FFI_ArrowSchema>) -> Result<Self> {
+ let mut this = Self::empty();
+
+ // note: this op leaks.
Review comment:
I just kept the original comment from below, going to remove it.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] kszucs commented on a change in pull request #439: [WIP] FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#discussion_r650767379
##########
File path: arrow/src/ffi.rs
##########
@@ -208,15 +213,25 @@ impl FFI_ArrowSchema {
pub fn name(&self) -> &str {
assert!(!self.name.is_null());
// safe because the lifetime of `self.name` equals `self`
- unsafe { CStr::from_ptr(self.name) }.to_str().unwrap()
+ unsafe { CStr::from_ptr(self.name) }
+ .to_str()
+ .expect("The external API has a non-utf8 as name")
+ }
+
+ pub fn flags(&self) -> Option<Flags> {
+ Flags::from_bits(self.flags)
}
pub fn child(&self, index: usize) -> &Self {
assert!(index < self.n_children as usize);
- assert!(!self.name.is_null());
+ // assert!(!self.name.is_null());
Review comment:
Why do we check the name here? Both schema and field can have children but only the latter one should have a name.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter edited a comment on pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#issuecomment-857974778
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#439](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7acd3a5) into [master](https://codecov.io/gh/apache/arrow-rs/commit/ef8887609017680d94b2a35f9889aa10cf3b3de8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ef88876) will **decrease** coverage by `0.05%`.
> The diff coverage is `63.19%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #439 +/- ##
==========================================
- Coverage 82.76% 82.71% -0.06%
==========================================
Files 165 166 +1
Lines 45724 45853 +129
==========================================
+ Hits 37844 37927 +83
- Misses 7880 7926 +46
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow-pyarrow-integration-testing/src/lib.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3ctcHlhcnJvdy1pbnRlZ3JhdGlvbi10ZXN0aW5nL3NyYy9saWIucnM=) | `0.00% <0.00%> (ø)` | |
| [arrow/src/datatypes/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9tb2QucnM=) | `100.00% <ø> (ø)` | |
| [arrow/src/datatypes/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9mZmkucnM=) | `72.86% <72.86%> (ø)` | |
| [arrow/src/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2ZmaS5ycw==) | `87.07% <92.50%> (+8.35%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ef88876...7acd3a5](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter edited a comment on pull request #439: [WIP] FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#issuecomment-857974778
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#439](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (57fbd16) into [master](https://codecov.io/gh/apache/arrow-rs/commit/18c804a8f80ada6577d16a6f0fab4b2e262e314d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (18c804a) will **increase** coverage by `0.07%`.
> The diff coverage is `78.62%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #439 +/- ##
==========================================
+ Coverage 82.64% 82.71% +0.07%
==========================================
Files 162 164 +2
Lines 44542 44817 +275
==========================================
+ Hits 36813 37072 +259
- Misses 7729 7745 +16
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/datatypes/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9tb2QucnM=) | `100.00% <ø> (ø)` | |
| [arrow/src/datatypes/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9mZmkucnM=) | `73.11% <73.11%> (ø)` | |
| [arrow/src/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2ZmaS5ycw==) | `85.95% <92.10%> (+3.10%)` | :arrow_up: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9tb2QucnM=) | `86.15% <0.00%> (-3.04%)` | :arrow_down: |
| [arrow/src/compute/kernels/sort.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9zb3J0LnJz) | `94.97% <0.00%> (-0.16%)` | :arrow_down: |
| [arrow/src/compute/kernels/concat.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jb25jYXQucnM=) | `100.00% <0.00%> (ø)` | |
| [arrow/src/compute/kernels/length.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9sZW5ndGgucnM=) | `100.00% <0.00%> (ø)` | |
| [arrow/src/compute/kernels/window.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy93aW5kb3cucnM=) | `100.00% <0.00%> (ø)` | |
| [arrow/src/compute/kernels/partition.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9wYXJ0aXRpb24ucnM=) | `99.19% <0.00%> (ø)` | |
| ... and [8 more](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [18c804a...57fbd16](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter edited a comment on pull request #439: [WIP] FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#issuecomment-857974778
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#439](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (60b5398) into [master](https://codecov.io/gh/apache/arrow-rs/commit/18c804a8f80ada6577d16a6f0fab4b2e262e314d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (18c804a) will **decrease** coverage by `0.03%`.
> The diff coverage is `75.00%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #439 +/- ##
==========================================
- Coverage 82.64% 82.61% -0.04%
==========================================
Files 162 165 +3
Lines 44542 45621 +1079
==========================================
+ Hits 36813 37691 +878
- Misses 7729 7930 +201
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow-pyarrow-integration-testing/src/lib.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3ctcHlhcnJvdy1pbnRlZ3JhdGlvbi10ZXN0aW5nL3NyYy9saWIucnM=) | `0.00% <0.00%> (ø)` | |
| [arrow/src/array/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2ZmaS5ycw==) | `100.00% <ø> (ø)` | |
| [arrow/src/datatypes/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9tb2QucnM=) | `100.00% <ø> (ø)` | |
| [arrow/src/datatypes/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9mZmkucnM=) | `86.18% <86.18%> (ø)` | |
| [arrow/src/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2ZmaS5ycw==) | `85.95% <92.50%> (+3.10%)` | :arrow_up: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9tb2QucnM=) | `86.06% <0.00%> (-3.12%)` | :arrow_down: |
| [arrow/src/compute/kernels/sort.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9zb3J0LnJz) | `94.11% <0.00%> (-1.02%)` | :arrow_down: |
| [parquet/src/util/test\_common/page\_util.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvdXRpbC90ZXN0X2NvbW1vbi9wYWdlX3V0aWwucnM=) | `91.00% <0.00%> (-0.67%)` | :arrow_down: |
| [parquet/src/arrow/record\_reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvcmVjb3JkX3JlYWRlci5ycw==) | `93.44% <0.00%> (-0.54%)` | :arrow_down: |
| [parquet/src/column/reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvY29sdW1uL3JlYWRlci5ycw==) | `74.36% <0.00%> (-0.38%)` | :arrow_down: |
| ... and [26 more](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [18c804a...60b5398](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] pitrou commented on a change in pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#discussion_r653875228
##########
File path: arrow/src/ffi.rs
##########
@@ -122,66 +132,61 @@ unsafe extern "C" fn release_schema(schema: *mut FFI_ArrowSchema) {
let schema = &mut *schema;
// take ownership back to release it.
- CString::from_raw(schema.format as *mut std::os::raw::c_char);
- CString::from_raw(schema.name as *mut std::os::raw::c_char);
- let private = Box::from_raw(schema.private_data as *mut SchemaPrivateData);
- for child in private.children_ptr.iter() {
- let _ = Box::from_raw(*child);
+ CString::from_raw(schema.format as *mut c_char);
+ if !schema.name.is_null() {
+ CString::from_raw(schema.name as *mut c_char);
+ }
+ if !schema.private_data.is_null() {
+ let private_data = Box::from_raw(schema.private_data as *mut SchemaPrivateData);
+ for child in private_data.children.iter() {
+ drop(Box::from_raw(*child))
Review comment:
Does `drop` automatically call the release callback on `child`? It seems it does, according to `impl Drop for FFI_ArrowSchema`, but I may be misunderstanding.
##########
File path: arrow/src/ffi.rs
##########
@@ -122,66 +132,61 @@ unsafe extern "C" fn release_schema(schema: *mut FFI_ArrowSchema) {
let schema = &mut *schema;
// take ownership back to release it.
- CString::from_raw(schema.format as *mut std::os::raw::c_char);
- CString::from_raw(schema.name as *mut std::os::raw::c_char);
- let private = Box::from_raw(schema.private_data as *mut SchemaPrivateData);
- for child in private.children_ptr.iter() {
- let _ = Box::from_raw(*child);
+ CString::from_raw(schema.format as *mut c_char);
+ if !schema.name.is_null() {
+ CString::from_raw(schema.name as *mut c_char);
+ }
+ if !schema.private_data.is_null() {
+ let private_data = Box::from_raw(schema.private_data as *mut SchemaPrivateData);
+ for child in private_data.children.iter() {
+ drop(Box::from_raw(*child))
+ }
+ drop(private_data);
}
schema.release = None;
}
impl FFI_ArrowSchema {
/// create a new [`Ffi_ArrowSchema`]. This fails if the fields' [`DataType`] is not supported.
- fn try_new(field: Field) -> Result<FFI_ArrowSchema> {
- let format = to_format(field.data_type())?;
- let name = field.name().clone();
-
- // allocate (and hold) the children
- let children_vec = match field.data_type() {
- DataType::List(field) => {
- vec![Box::new(FFI_ArrowSchema::try_new(field.as_ref().clone())?)]
- }
- DataType::LargeList(field) => {
- vec![Box::new(FFI_ArrowSchema::try_new(field.as_ref().clone())?)]
- }
- DataType::Struct(fields) => fields
- .iter()
- .map(|field| Ok(Box::new(FFI_ArrowSchema::try_new(field.clone())?)))
- .collect::<Result<Vec<_>>>()?,
- _ => vec![],
- };
- // note: this cannot be done along with the above because the above is fallible and this op leaks.
- let children_ptr = children_vec
+ pub fn try_new(format: &str, children: Vec<FFI_ArrowSchema>) -> Result<Self> {
+ let mut this = Self::empty();
+
+ // note: this op leaks.
Review comment:
Does it? It seems that you call `from_raw` on each item in `release_schema`, so that should be ok?
##########
File path: arrow/src/datatypes/ffi.rs
##########
@@ -0,0 +1,287 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::convert::TryFrom;
+
+use crate::{
+ datatypes::{DataType, Field, Schema, TimeUnit},
+ error::{ArrowError, Result},
+ ffi::{FFI_ArrowSchema, Flags},
+};
+
+impl TryFrom<&FFI_ArrowSchema> for DataType {
+ type Error = ArrowError;
+
+ /// See https://arrow.apache.org/docs/format/CDataInterface.html#data-type-description-format-strings
+ fn try_from(c_schema: &FFI_ArrowSchema) -> Result<Self> {
+ let dtype = match c_schema.format() {
+ "n" => DataType::Null,
+ "b" => DataType::Boolean,
+ "c" => DataType::Int8,
+ "C" => DataType::UInt8,
+ "s" => DataType::Int16,
+ "S" => DataType::UInt16,
+ "i" => DataType::Int32,
+ "I" => DataType::UInt32,
+ "l" => DataType::Int64,
+ "L" => DataType::UInt64,
+ "e" => DataType::Float16,
+ "f" => DataType::Float32,
+ "g" => DataType::Float64,
+ "z" => DataType::Binary,
+ "Z" => DataType::LargeBinary,
+ "u" => DataType::Utf8,
+ "U" => DataType::LargeUtf8,
+ "tdD" => DataType::Date32,
+ "tdm" => DataType::Date64,
+ "tts" => DataType::Time32(TimeUnit::Second),
+ "ttm" => DataType::Time32(TimeUnit::Millisecond),
+ "ttu" => DataType::Time64(TimeUnit::Microsecond),
+ "ttn" => DataType::Time64(TimeUnit::Nanosecond),
+ "+l" => {
+ let c_child = c_schema.child(0);
+ DataType::List(Box::new(Field::try_from(c_child)?))
+ }
+ "+L" => {
+ let c_child = c_schema.child(0);
+ DataType::LargeList(Box::new(Field::try_from(c_child)?))
+ }
+ "+s" => {
+ let fields = c_schema.children().map(Field::try_from);
+ DataType::Struct(fields.collect::<Result<Vec<_>>>()?)
+ }
+ other => {
+ return Err(ArrowError::CDataInterface(format!(
+ "The datatype \"{:?}\" is still not supported in Rust implementation",
+ other
+ )))
+ }
+ };
+ Ok(dtype)
+ }
+}
+
+impl TryFrom<&FFI_ArrowSchema> for Field {
+ type Error = ArrowError;
+
+ fn try_from(c_schema: &FFI_ArrowSchema) -> Result<Self> {
+ let dtype = DataType::try_from(c_schema)?;
+ let field = Field::new(c_schema.name(), dtype, c_schema.nullable());
+ Ok(field)
Review comment:
Is there a TODO for metadata?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter edited a comment on pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#issuecomment-857974778
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#439](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (095d060) into [master](https://codecov.io/gh/apache/arrow-rs/commit/97232097870a01dd6d9d6ec16c1b80a84519a561?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9723209) will **decrease** coverage by `0.04%`.
> The diff coverage is `63.19%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #439 +/- ##
==========================================
- Coverage 82.62% 82.57% -0.05%
==========================================
Files 165 166 +1
Lines 45664 45793 +129
==========================================
+ Hits 37728 37812 +84
- Misses 7936 7981 +45
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow-pyarrow-integration-testing/src/lib.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3ctcHlhcnJvdy1pbnRlZ3JhdGlvbi10ZXN0aW5nL3NyYy9saWIucnM=) | `0.00% <0.00%> (ø)` | |
| [arrow/src/datatypes/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9tb2QucnM=) | `100.00% <ø> (ø)` | |
| [arrow/src/datatypes/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9mZmkucnM=) | `72.86% <72.86%> (ø)` | |
| [arrow/src/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2ZmaS5ycw==) | `87.07% <92.50%> (+8.35%)` | :arrow_up: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZW5jb2RpbmdzL2VuY29kaW5nLnJz) | `95.04% <0.00%> (+0.19%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [9723209...095d060](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] kszucs commented on a change in pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#discussion_r655484390
##########
File path: arrow-pyarrow-integration-testing/tests/test_sql.py
##########
@@ -16,84 +16,195 @@
# specific language governing permissions and limitations
# under the License.
-import unittest
-
-import pyarrow
-import arrow_pyarrow_integration_testing
-
-
-class TestCase(unittest.TestCase):
- def test_primitive_python(self):
- """
- Python -> Rust -> Python
- """
- old_allocated = pyarrow.total_allocated_bytes()
- a = pyarrow.array([1, 2, 3])
- b = arrow_pyarrow_integration_testing.double(a)
- self.assertEqual(b, pyarrow.array([2, 4, 6]))
- del a
- del b
- # No leak of C++ memory
- self.assertEqual(old_allocated, pyarrow.total_allocated_bytes())
-
- def test_primitive_rust(self):
- """
- Rust -> Python -> Rust
- """
- old_allocated = pyarrow.total_allocated_bytes()
-
- def double(array):
- array = array.to_pylist()
- return pyarrow.array([x * 2 if x is not None else None for x in array])
-
- is_correct = arrow_pyarrow_integration_testing.double_py(double)
- self.assertTrue(is_correct)
- # No leak of C++ memory
- self.assertEqual(old_allocated, pyarrow.total_allocated_bytes())
-
- def test_string_python(self):
- """
- Python -> Rust -> Python
- """
- old_allocated = pyarrow.total_allocated_bytes()
- a = pyarrow.array(["a", None, "ccc"])
- b = arrow_pyarrow_integration_testing.substring(a, 1)
- self.assertEqual(b, pyarrow.array(["", None, "cc"]))
- del a
- del b
- # No leak of C++ memory
- self.assertEqual(old_allocated, pyarrow.total_allocated_bytes())
-
- def test_time32_python(self):
- """
- Python -> Rust -> Python
- """
- old_allocated = pyarrow.total_allocated_bytes()
- a = pyarrow.array([None, 1, 2], pyarrow.time32('s'))
- b = arrow_pyarrow_integration_testing.concatenate(a)
- expected = pyarrow.array([None, 1, 2] + [None, 1, 2], pyarrow.time32('s'))
- self.assertEqual(b, expected)
- del a
- del b
- del expected
- # No leak of C++ memory
- self.assertEqual(old_allocated, pyarrow.total_allocated_bytes())
-
- def test_list_array(self):
- """
- Python -> Rust -> Python
- """
- old_allocated = pyarrow.total_allocated_bytes()
- a = pyarrow.array([[], None, [1, 2], [4, 5, 6]], pyarrow.list_(pyarrow.int64()))
- b = arrow_pyarrow_integration_testing.round_trip(a)
-
- b.validate(full=True)
- assert a.to_pylist() == b.to_pylist()
- assert a.type == b.type
- del a
- del b
- # No leak of C++ memory
- self.assertEqual(old_allocated, pyarrow.total_allocated_bytes())
+import contextlib
+import string
+import pytest
+import pyarrow as pa
+from arrow_pyarrow_integration_testing import PyDataType, PyField, PySchema
+import arrow_pyarrow_integration_testing as rust
+
+@contextlib.contextmanager
+def no_pyarrow_leak():
+ # No leak of C++ memory
+ old_allocation = pa.total_allocated_bytes()
+ try:
+ yield
+ finally:
+ assert pa.total_allocated_bytes() == old_allocation
+
+
+@pytest.fixture(autouse=True)
+def assert_pyarrow_leak():
+ # automatically applied to all test cases
+ with no_pyarrow_leak():
+ yield
+
+
+_supported_pyarrow_types = [
+ pa.null(),
+ pa.bool_(),
+ pa.int32(),
+ pa.time32("s"),
+ pa.time64("us"),
+ pa.date32(),
+ pa.float16(),
+ pa.float32(),
+ pa.float64(),
+ pa.string(),
+ pa.binary(),
+ pa.large_string(),
+ pa.large_binary(),
+ pa.list_(pa.int32()),
+ pa.large_list(pa.uint16()),
+ pa.struct(
+ [
+ pa.field("a", pa.int32()),
+ pa.field("b", pa.int8()),
+ pa.field("c", pa.string()),
+ ]
+ ),
+ pa.struct(
+ [
+ pa.field("a", pa.int32(), nullable=False),
+ pa.field("b", pa.int8(), nullable=False),
+ pa.field("c", pa.string()),
+ ]
+ ),
+]
+
+_unsupported_pyarrow_types = [
Review comment:
https://github.com/apache/arrow-rs/issues/477
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter edited a comment on pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#issuecomment-857974778
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#439](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6768471) into [master](https://codecov.io/gh/apache/arrow-rs/commit/9f56afb2d2347310184706f7d5e46af583557bea?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9f56afb) will **decrease** coverage by `0.05%`.
> The diff coverage is `70.12%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #439 +/- ##
==========================================
- Coverage 82.65% 82.60% -0.06%
==========================================
Files 165 166 +1
Lines 45556 45654 +98
==========================================
+ Hits 37655 37711 +56
- Misses 7901 7943 +42
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow-pyarrow-integration-testing/src/lib.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3ctcHlhcnJvdy1pbnRlZ3JhdGlvbi10ZXN0aW5nL3NyYy9saWIucnM=) | `0.00% <0.00%> (ø)` | |
| [arrow/src/datatypes/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9tb2QucnM=) | `100.00% <ø> (ø)` | |
| [arrow/src/datatypes/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9mZmkucnM=) | `86.84% <86.84%> (ø)` | |
| [arrow/src/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2ZmaS5ycw==) | `85.95% <92.50%> (+3.10%)` | :arrow_up: |
| [arrow/src/array/array\_struct.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5X3N0cnVjdC5ycw==) | `87.84% <0.00%> (-0.59%)` | :arrow_down: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZW5jb2RpbmdzL2VuY29kaW5nLnJz) | `94.85% <0.00%> (-0.20%)` | :arrow_down: |
| [arrow/src/array/builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIucnM=) | `85.78% <0.00%> (-0.13%)` | :arrow_down: |
| [arrow/src/ipc/reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2lwYy9yZWFkZXIucnM=) | `84.30% <0.00%> (-0.04%)` | :arrow_down: |
| [parquet/src/record/api.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvcmVjb3JkL2FwaS5ycw==) | `92.48% <0.00%> (-0.03%)` | :arrow_down: |
| [arrow/src/array/ord.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L29yZC5ycw==) | `63.33% <0.00%> (ø)` | |
| ... and [6 more](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [9f56afb...6768471](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] kszucs commented on a change in pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#discussion_r655509158
##########
File path: arrow/src/datatypes/ffi.rs
##########
@@ -0,0 +1,287 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::convert::TryFrom;
+
+use crate::{
+ datatypes::{DataType, Field, Schema, TimeUnit},
+ error::{ArrowError, Result},
+ ffi::{FFI_ArrowSchema, Flags},
+};
+
+impl TryFrom<&FFI_ArrowSchema> for DataType {
+ type Error = ArrowError;
+
+ /// See https://arrow.apache.org/docs/format/CDataInterface.html#data-type-description-format-strings
+ fn try_from(c_schema: &FFI_ArrowSchema) -> Result<Self> {
+ let dtype = match c_schema.format() {
+ "n" => DataType::Null,
+ "b" => DataType::Boolean,
+ "c" => DataType::Int8,
+ "C" => DataType::UInt8,
+ "s" => DataType::Int16,
+ "S" => DataType::UInt16,
+ "i" => DataType::Int32,
+ "I" => DataType::UInt32,
+ "l" => DataType::Int64,
+ "L" => DataType::UInt64,
+ "e" => DataType::Float16,
+ "f" => DataType::Float32,
+ "g" => DataType::Float64,
+ "z" => DataType::Binary,
+ "Z" => DataType::LargeBinary,
+ "u" => DataType::Utf8,
+ "U" => DataType::LargeUtf8,
+ "tdD" => DataType::Date32,
+ "tdm" => DataType::Date64,
+ "tts" => DataType::Time32(TimeUnit::Second),
+ "ttm" => DataType::Time32(TimeUnit::Millisecond),
+ "ttu" => DataType::Time64(TimeUnit::Microsecond),
+ "ttn" => DataType::Time64(TimeUnit::Nanosecond),
+ "+l" => {
+ let c_child = c_schema.child(0);
+ DataType::List(Box::new(Field::try_from(c_child)?))
+ }
+ "+L" => {
+ let c_child = c_schema.child(0);
+ DataType::LargeList(Box::new(Field::try_from(c_child)?))
+ }
+ "+s" => {
+ let fields = c_schema.children().map(Field::try_from);
+ DataType::Struct(fields.collect::<Result<Vec<_>>>()?)
+ }
+ other => {
+ return Err(ArrowError::CDataInterface(format!(
+ "The datatype \"{:?}\" is still not supported in Rust implementation",
+ other
+ )))
+ }
+ };
+ Ok(dtype)
+ }
+}
+
+impl TryFrom<&FFI_ArrowSchema> for Field {
+ type Error = ArrowError;
+
+ fn try_from(c_schema: &FFI_ArrowSchema) -> Result<Self> {
+ let dtype = DataType::try_from(c_schema)?;
+ let field = Field::new(c_schema.name(), dtype, c_schema.nullable());
+ Ok(field)
Review comment:
https://github.com/apache/arrow-rs/issues/478
##########
File path: arrow/src/ffi.rs
##########
@@ -122,66 +132,61 @@ unsafe extern "C" fn release_schema(schema: *mut FFI_ArrowSchema) {
let schema = &mut *schema;
// take ownership back to release it.
- CString::from_raw(schema.format as *mut std::os::raw::c_char);
- CString::from_raw(schema.name as *mut std::os::raw::c_char);
- let private = Box::from_raw(schema.private_data as *mut SchemaPrivateData);
- for child in private.children_ptr.iter() {
- let _ = Box::from_raw(*child);
+ CString::from_raw(schema.format as *mut c_char);
+ if !schema.name.is_null() {
+ CString::from_raw(schema.name as *mut c_char);
+ }
+ if !schema.private_data.is_null() {
+ let private_data = Box::from_raw(schema.private_data as *mut SchemaPrivateData);
+ for child in private_data.children.iter() {
+ drop(Box::from_raw(*child))
+ }
+ drop(private_data);
}
schema.release = None;
}
impl FFI_ArrowSchema {
/// create a new [`Ffi_ArrowSchema`]. This fails if the fields' [`DataType`] is not supported.
- fn try_new(field: Field) -> Result<FFI_ArrowSchema> {
- let format = to_format(field.data_type())?;
- let name = field.name().clone();
-
- // allocate (and hold) the children
- let children_vec = match field.data_type() {
- DataType::List(field) => {
- vec![Box::new(FFI_ArrowSchema::try_new(field.as_ref().clone())?)]
- }
- DataType::LargeList(field) => {
- vec![Box::new(FFI_ArrowSchema::try_new(field.as_ref().clone())?)]
- }
- DataType::Struct(fields) => fields
- .iter()
- .map(|field| Ok(Box::new(FFI_ArrowSchema::try_new(field.clone())?)))
- .collect::<Result<Vec<_>>>()?,
- _ => vec![],
- };
- // note: this cannot be done along with the above because the above is fallible and this op leaks.
- let children_ptr = children_vec
+ pub fn try_new(format: &str, children: Vec<FFI_ArrowSchema>) -> Result<Self> {
+ let mut this = Self::empty();
+
+ // note: this op leaks.
+ let mut children_ptr = children
.into_iter()
+ .map(Box::new)
.map(Box::into_raw)
.collect::<Box<_>>();
- let n_children = children_ptr.len() as i64;
- let flags = field.is_nullable() as i64 * 2;
+ this.format = CString::new(format).unwrap().into_raw();
+ this.release = Some(release_schema);
+ this.n_children = children_ptr.len() as i64;
+ this.children = children_ptr.as_mut_ptr();
- let mut private = Box::new(SchemaPrivateData {
- field,
- children_ptr,
+ let private_data = Box::new(SchemaPrivateData {
+ children: children_ptr,
});
+ this.private_data = Box::into_raw(private_data) as *mut c_void;
- // <https://arrow.apache.org/docs/format/CDataInterface.html#c.ArrowSchema>
- Ok(FFI_ArrowSchema {
- format: CString::new(format).unwrap().into_raw(),
- name: CString::new(name).unwrap().into_raw(),
- metadata: std::ptr::null_mut(),
- flags,
- n_children,
- children: private.children_ptr.as_mut_ptr(),
- dictionary: std::ptr::null_mut(),
- release: Some(release_schema),
- private_data: Box::into_raw(private) as *mut ::std::os::raw::c_void,
- })
+ Ok(this)
+ }
+
+ pub fn with_name(mut self, name: &str) -> Result<Self> {
+ self.name = CString::new(name).unwrap().into_raw();
+ Ok(self)
+ }
+
+ pub fn with_flags(mut self, flags: Flags) -> Result<Self> {
+ self.flags = flags.bits();
+ Ok(self)
}
+ // pub fn with_dictionary() {}
+ // pub fn with_metadata() {}
Review comment:
https://github.com/apache/arrow-rs/issues/478
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] jorgecarleitao commented on a change in pull request #439: [WIP] FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#discussion_r650633827
##########
File path: arrow/src/ffi.rs
##########
@@ -208,15 +213,25 @@ impl FFI_ArrowSchema {
pub fn name(&self) -> &str {
assert!(!self.name.is_null());
// safe because the lifetime of `self.name` equals `self`
- unsafe { CStr::from_ptr(self.name) }.to_str().unwrap()
+ unsafe { CStr::from_ptr(self.name) }
+ .to_str()
+ .expect("The external API has a non-utf8 as name")
+ }
+
+ pub fn flags(&self) -> Option<Flags> {
+ Flags::from_bits(self.flags)
}
pub fn child(&self, index: usize) -> &Self {
assert!(index < self.n_children as usize);
- assert!(!self.name.is_null());
+ // assert!(!self.name.is_null());
Review comment:
why was this commented?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] kszucs commented on a change in pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#discussion_r655484390
##########
File path: arrow-pyarrow-integration-testing/tests/test_sql.py
##########
@@ -16,84 +16,195 @@
# specific language governing permissions and limitations
# under the License.
-import unittest
-
-import pyarrow
-import arrow_pyarrow_integration_testing
-
-
-class TestCase(unittest.TestCase):
- def test_primitive_python(self):
- """
- Python -> Rust -> Python
- """
- old_allocated = pyarrow.total_allocated_bytes()
- a = pyarrow.array([1, 2, 3])
- b = arrow_pyarrow_integration_testing.double(a)
- self.assertEqual(b, pyarrow.array([2, 4, 6]))
- del a
- del b
- # No leak of C++ memory
- self.assertEqual(old_allocated, pyarrow.total_allocated_bytes())
-
- def test_primitive_rust(self):
- """
- Rust -> Python -> Rust
- """
- old_allocated = pyarrow.total_allocated_bytes()
-
- def double(array):
- array = array.to_pylist()
- return pyarrow.array([x * 2 if x is not None else None for x in array])
-
- is_correct = arrow_pyarrow_integration_testing.double_py(double)
- self.assertTrue(is_correct)
- # No leak of C++ memory
- self.assertEqual(old_allocated, pyarrow.total_allocated_bytes())
-
- def test_string_python(self):
- """
- Python -> Rust -> Python
- """
- old_allocated = pyarrow.total_allocated_bytes()
- a = pyarrow.array(["a", None, "ccc"])
- b = arrow_pyarrow_integration_testing.substring(a, 1)
- self.assertEqual(b, pyarrow.array(["", None, "cc"]))
- del a
- del b
- # No leak of C++ memory
- self.assertEqual(old_allocated, pyarrow.total_allocated_bytes())
-
- def test_time32_python(self):
- """
- Python -> Rust -> Python
- """
- old_allocated = pyarrow.total_allocated_bytes()
- a = pyarrow.array([None, 1, 2], pyarrow.time32('s'))
- b = arrow_pyarrow_integration_testing.concatenate(a)
- expected = pyarrow.array([None, 1, 2] + [None, 1, 2], pyarrow.time32('s'))
- self.assertEqual(b, expected)
- del a
- del b
- del expected
- # No leak of C++ memory
- self.assertEqual(old_allocated, pyarrow.total_allocated_bytes())
-
- def test_list_array(self):
- """
- Python -> Rust -> Python
- """
- old_allocated = pyarrow.total_allocated_bytes()
- a = pyarrow.array([[], None, [1, 2], [4, 5, 6]], pyarrow.list_(pyarrow.int64()))
- b = arrow_pyarrow_integration_testing.round_trip(a)
-
- b.validate(full=True)
- assert a.to_pylist() == b.to_pylist()
- assert a.type == b.type
- del a
- del b
- # No leak of C++ memory
- self.assertEqual(old_allocated, pyarrow.total_allocated_bytes())
+import contextlib
+import string
+import pytest
+import pyarrow as pa
+from arrow_pyarrow_integration_testing import PyDataType, PyField, PySchema
+import arrow_pyarrow_integration_testing as rust
+
+@contextlib.contextmanager
+def no_pyarrow_leak():
+ # No leak of C++ memory
+ old_allocation = pa.total_allocated_bytes()
+ try:
+ yield
+ finally:
+ assert pa.total_allocated_bytes() == old_allocation
+
+
+@pytest.fixture(autouse=True)
+def assert_pyarrow_leak():
+ # automatically applied to all test cases
+ with no_pyarrow_leak():
+ yield
+
+
+_supported_pyarrow_types = [
+ pa.null(),
+ pa.bool_(),
+ pa.int32(),
+ pa.time32("s"),
+ pa.time64("us"),
+ pa.date32(),
+ pa.float16(),
+ pa.float32(),
+ pa.float64(),
+ pa.string(),
+ pa.binary(),
+ pa.large_string(),
+ pa.large_binary(),
+ pa.list_(pa.int32()),
+ pa.large_list(pa.uint16()),
+ pa.struct(
+ [
+ pa.field("a", pa.int32()),
+ pa.field("b", pa.int8()),
+ pa.field("c", pa.string()),
+ ]
+ ),
+ pa.struct(
+ [
+ pa.field("a", pa.int32(), nullable=False),
+ pa.field("b", pa.int8(), nullable=False),
+ pa.field("c", pa.string()),
+ ]
+ ),
+]
+
+_unsupported_pyarrow_types = [
Review comment:
https://github.com/apache/arrow-rs/issues/477
##########
File path: arrow/src/datatypes/ffi.rs
##########
@@ -0,0 +1,287 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::convert::TryFrom;
+
+use crate::{
+ datatypes::{DataType, Field, Schema, TimeUnit},
+ error::{ArrowError, Result},
+ ffi::{FFI_ArrowSchema, Flags},
+};
+
+impl TryFrom<&FFI_ArrowSchema> for DataType {
+ type Error = ArrowError;
+
+ /// See https://arrow.apache.org/docs/format/CDataInterface.html#data-type-description-format-strings
+ fn try_from(c_schema: &FFI_ArrowSchema) -> Result<Self> {
+ let dtype = match c_schema.format() {
+ "n" => DataType::Null,
+ "b" => DataType::Boolean,
+ "c" => DataType::Int8,
+ "C" => DataType::UInt8,
+ "s" => DataType::Int16,
+ "S" => DataType::UInt16,
+ "i" => DataType::Int32,
+ "I" => DataType::UInt32,
+ "l" => DataType::Int64,
+ "L" => DataType::UInt64,
+ "e" => DataType::Float16,
+ "f" => DataType::Float32,
+ "g" => DataType::Float64,
+ "z" => DataType::Binary,
+ "Z" => DataType::LargeBinary,
+ "u" => DataType::Utf8,
+ "U" => DataType::LargeUtf8,
+ "tdD" => DataType::Date32,
+ "tdm" => DataType::Date64,
+ "tts" => DataType::Time32(TimeUnit::Second),
+ "ttm" => DataType::Time32(TimeUnit::Millisecond),
+ "ttu" => DataType::Time64(TimeUnit::Microsecond),
+ "ttn" => DataType::Time64(TimeUnit::Nanosecond),
+ "+l" => {
+ let c_child = c_schema.child(0);
+ DataType::List(Box::new(Field::try_from(c_child)?))
+ }
+ "+L" => {
+ let c_child = c_schema.child(0);
+ DataType::LargeList(Box::new(Field::try_from(c_child)?))
+ }
+ "+s" => {
+ let fields = c_schema.children().map(Field::try_from);
+ DataType::Struct(fields.collect::<Result<Vec<_>>>()?)
+ }
+ other => {
+ return Err(ArrowError::CDataInterface(format!(
+ "The datatype \"{:?}\" is still not supported in Rust implementation",
+ other
+ )))
+ }
+ };
+ Ok(dtype)
+ }
+}
+
+impl TryFrom<&FFI_ArrowSchema> for Field {
+ type Error = ArrowError;
+
+ fn try_from(c_schema: &FFI_ArrowSchema) -> Result<Self> {
+ let dtype = DataType::try_from(c_schema)?;
+ let field = Field::new(c_schema.name(), dtype, c_schema.nullable());
+ Ok(field)
Review comment:
https://github.com/apache/arrow-rs/issues/478
##########
File path: arrow/src/ffi.rs
##########
@@ -122,66 +132,61 @@ unsafe extern "C" fn release_schema(schema: *mut FFI_ArrowSchema) {
let schema = &mut *schema;
// take ownership back to release it.
- CString::from_raw(schema.format as *mut std::os::raw::c_char);
- CString::from_raw(schema.name as *mut std::os::raw::c_char);
- let private = Box::from_raw(schema.private_data as *mut SchemaPrivateData);
- for child in private.children_ptr.iter() {
- let _ = Box::from_raw(*child);
+ CString::from_raw(schema.format as *mut c_char);
+ if !schema.name.is_null() {
+ CString::from_raw(schema.name as *mut c_char);
+ }
+ if !schema.private_data.is_null() {
+ let private_data = Box::from_raw(schema.private_data as *mut SchemaPrivateData);
+ for child in private_data.children.iter() {
+ drop(Box::from_raw(*child))
+ }
+ drop(private_data);
}
schema.release = None;
}
impl FFI_ArrowSchema {
/// create a new [`Ffi_ArrowSchema`]. This fails if the fields' [`DataType`] is not supported.
- fn try_new(field: Field) -> Result<FFI_ArrowSchema> {
- let format = to_format(field.data_type())?;
- let name = field.name().clone();
-
- // allocate (and hold) the children
- let children_vec = match field.data_type() {
- DataType::List(field) => {
- vec![Box::new(FFI_ArrowSchema::try_new(field.as_ref().clone())?)]
- }
- DataType::LargeList(field) => {
- vec![Box::new(FFI_ArrowSchema::try_new(field.as_ref().clone())?)]
- }
- DataType::Struct(fields) => fields
- .iter()
- .map(|field| Ok(Box::new(FFI_ArrowSchema::try_new(field.clone())?)))
- .collect::<Result<Vec<_>>>()?,
- _ => vec![],
- };
- // note: this cannot be done along with the above because the above is fallible and this op leaks.
- let children_ptr = children_vec
+ pub fn try_new(format: &str, children: Vec<FFI_ArrowSchema>) -> Result<Self> {
+ let mut this = Self::empty();
+
+ // note: this op leaks.
+ let mut children_ptr = children
.into_iter()
+ .map(Box::new)
.map(Box::into_raw)
.collect::<Box<_>>();
- let n_children = children_ptr.len() as i64;
- let flags = field.is_nullable() as i64 * 2;
+ this.format = CString::new(format).unwrap().into_raw();
+ this.release = Some(release_schema);
+ this.n_children = children_ptr.len() as i64;
+ this.children = children_ptr.as_mut_ptr();
- let mut private = Box::new(SchemaPrivateData {
- field,
- children_ptr,
+ let private_data = Box::new(SchemaPrivateData {
+ children: children_ptr,
});
+ this.private_data = Box::into_raw(private_data) as *mut c_void;
- // <https://arrow.apache.org/docs/format/CDataInterface.html#c.ArrowSchema>
- Ok(FFI_ArrowSchema {
- format: CString::new(format).unwrap().into_raw(),
- name: CString::new(name).unwrap().into_raw(),
- metadata: std::ptr::null_mut(),
- flags,
- n_children,
- children: private.children_ptr.as_mut_ptr(),
- dictionary: std::ptr::null_mut(),
- release: Some(release_schema),
- private_data: Box::into_raw(private) as *mut ::std::os::raw::c_void,
- })
+ Ok(this)
+ }
+
+ pub fn with_name(mut self, name: &str) -> Result<Self> {
+ self.name = CString::new(name).unwrap().into_raw();
+ Ok(self)
+ }
+
+ pub fn with_flags(mut self, flags: Flags) -> Result<Self> {
+ self.flags = flags.bits();
+ Ok(self)
}
+ // pub fn with_dictionary() {}
+ // pub fn with_metadata() {}
Review comment:
https://github.com/apache/arrow-rs/issues/478
##########
File path: arrow/src/ffi.rs
##########
@@ -122,66 +132,61 @@ unsafe extern "C" fn release_schema(schema: *mut FFI_ArrowSchema) {
let schema = &mut *schema;
// take ownership back to release it.
- CString::from_raw(schema.format as *mut std::os::raw::c_char);
- CString::from_raw(schema.name as *mut std::os::raw::c_char);
- let private = Box::from_raw(schema.private_data as *mut SchemaPrivateData);
- for child in private.children_ptr.iter() {
- let _ = Box::from_raw(*child);
+ CString::from_raw(schema.format as *mut c_char);
+ if !schema.name.is_null() {
+ CString::from_raw(schema.name as *mut c_char);
+ }
+ if !schema.private_data.is_null() {
+ let private_data = Box::from_raw(schema.private_data as *mut SchemaPrivateData);
+ for child in private_data.children.iter() {
+ drop(Box::from_raw(*child))
Review comment:
That's my understanding as well.
##########
File path: arrow/src/ffi.rs
##########
@@ -122,66 +132,61 @@ unsafe extern "C" fn release_schema(schema: *mut FFI_ArrowSchema) {
let schema = &mut *schema;
// take ownership back to release it.
- CString::from_raw(schema.format as *mut std::os::raw::c_char);
- CString::from_raw(schema.name as *mut std::os::raw::c_char);
- let private = Box::from_raw(schema.private_data as *mut SchemaPrivateData);
- for child in private.children_ptr.iter() {
- let _ = Box::from_raw(*child);
+ CString::from_raw(schema.format as *mut c_char);
+ if !schema.name.is_null() {
+ CString::from_raw(schema.name as *mut c_char);
+ }
+ if !schema.private_data.is_null() {
+ let private_data = Box::from_raw(schema.private_data as *mut SchemaPrivateData);
+ for child in private_data.children.iter() {
+ drop(Box::from_raw(*child))
+ }
+ drop(private_data);
}
schema.release = None;
}
impl FFI_ArrowSchema {
/// create a new [`Ffi_ArrowSchema`]. This fails if the fields' [`DataType`] is not supported.
- fn try_new(field: Field) -> Result<FFI_ArrowSchema> {
- let format = to_format(field.data_type())?;
- let name = field.name().clone();
-
- // allocate (and hold) the children
- let children_vec = match field.data_type() {
- DataType::List(field) => {
- vec![Box::new(FFI_ArrowSchema::try_new(field.as_ref().clone())?)]
- }
- DataType::LargeList(field) => {
- vec![Box::new(FFI_ArrowSchema::try_new(field.as_ref().clone())?)]
- }
- DataType::Struct(fields) => fields
- .iter()
- .map(|field| Ok(Box::new(FFI_ArrowSchema::try_new(field.clone())?)))
- .collect::<Result<Vec<_>>>()?,
- _ => vec![],
- };
- // note: this cannot be done along with the above because the above is fallible and this op leaks.
- let children_ptr = children_vec
+ pub fn try_new(format: &str, children: Vec<FFI_ArrowSchema>) -> Result<Self> {
+ let mut this = Self::empty();
+
+ // note: this op leaks.
Review comment:
I just kept the original comment from below, going to remove it.
##########
File path: arrow/src/ffi.rs
##########
@@ -122,66 +132,61 @@ unsafe extern "C" fn release_schema(schema: *mut FFI_ArrowSchema) {
let schema = &mut *schema;
// take ownership back to release it.
- CString::from_raw(schema.format as *mut std::os::raw::c_char);
- CString::from_raw(schema.name as *mut std::os::raw::c_char);
- let private = Box::from_raw(schema.private_data as *mut SchemaPrivateData);
- for child in private.children_ptr.iter() {
- let _ = Box::from_raw(*child);
+ CString::from_raw(schema.format as *mut c_char);
+ if !schema.name.is_null() {
+ CString::from_raw(schema.name as *mut c_char);
+ }
+ if !schema.private_data.is_null() {
+ let private_data = Box::from_raw(schema.private_data as *mut SchemaPrivateData);
+ for child in private_data.children.iter() {
+ drop(Box::from_raw(*child))
+ }
+ drop(private_data);
}
schema.release = None;
}
impl FFI_ArrowSchema {
/// create a new [`Ffi_ArrowSchema`]. This fails if the fields' [`DataType`] is not supported.
- fn try_new(field: Field) -> Result<FFI_ArrowSchema> {
- let format = to_format(field.data_type())?;
- let name = field.name().clone();
-
- // allocate (and hold) the children
- let children_vec = match field.data_type() {
- DataType::List(field) => {
- vec![Box::new(FFI_ArrowSchema::try_new(field.as_ref().clone())?)]
- }
- DataType::LargeList(field) => {
- vec![Box::new(FFI_ArrowSchema::try_new(field.as_ref().clone())?)]
- }
- DataType::Struct(fields) => fields
- .iter()
- .map(|field| Ok(Box::new(FFI_ArrowSchema::try_new(field.clone())?)))
- .collect::<Result<Vec<_>>>()?,
- _ => vec![],
- };
- // note: this cannot be done along with the above because the above is fallible and this op leaks.
- let children_ptr = children_vec
+ pub fn try_new(format: &str, children: Vec<FFI_ArrowSchema>) -> Result<Self> {
+ let mut this = Self::empty();
+
+ // note: this op leaks.
+ let mut children_ptr = children
.into_iter()
+ .map(Box::new)
.map(Box::into_raw)
.collect::<Box<_>>();
- let n_children = children_ptr.len() as i64;
- let flags = field.is_nullable() as i64 * 2;
+ this.format = CString::new(format).unwrap().into_raw();
+ this.release = Some(release_schema);
+ this.n_children = children_ptr.len() as i64;
+ this.children = children_ptr.as_mut_ptr();
- let mut private = Box::new(SchemaPrivateData {
- field,
- children_ptr,
+ let private_data = Box::new(SchemaPrivateData {
+ children: children_ptr,
});
+ this.private_data = Box::into_raw(private_data) as *mut c_void;
- // <https://arrow.apache.org/docs/format/CDataInterface.html#c.ArrowSchema>
- Ok(FFI_ArrowSchema {
- format: CString::new(format).unwrap().into_raw(),
- name: CString::new(name).unwrap().into_raw(),
- metadata: std::ptr::null_mut(),
- flags,
- n_children,
- children: private.children_ptr.as_mut_ptr(),
- dictionary: std::ptr::null_mut(),
- release: Some(release_schema),
- private_data: Box::into_raw(private) as *mut ::std::os::raw::c_void,
- })
+ Ok(this)
+ }
+
+ pub fn with_name(mut self, name: &str) -> Result<Self> {
+ self.name = CString::new(name).unwrap().into_raw();
+ Ok(self)
+ }
+
+ pub fn with_flags(mut self, flags: Flags) -> Result<Self> {
+ self.flags = flags.bits();
+ Ok(self)
}
+ // pub fn with_dictionary() {}
+ // pub fn with_metadata() {}
Review comment:
Metadata: https://github.com/apache/arrow-rs/issues/478
Dictionary: https://github.com/apache/arrow-rs/issues/479
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter commented on pull request #439: [WIP] FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#issuecomment-857974778
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#439](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (404991d) into [master](https://codecov.io/gh/apache/arrow-rs/commit/18c804a8f80ada6577d16a6f0fab4b2e262e314d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (18c804a) will **decrease** coverage by `0.01%`.
> The diff coverage is `84.61%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #439 +/- ##
==========================================
- Coverage 82.64% 82.63% -0.02%
==========================================
Files 162 163 +1
Lines 44542 44683 +141
==========================================
+ Hits 36813 36924 +111
- Misses 7729 7759 +30
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/datatypes/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9tb2QucnM=) | `100.00% <ø> (ø)` | |
| [arrow/src/datatypes/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9mZmkucnM=) | `83.33% <83.33%> (ø)` | |
| [arrow/src/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2ZmaS5ycw==) | `81.77% <100.00%> (-1.09%)` | :arrow_down: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9tb2QucnM=) | `86.15% <0.00%> (-3.04%)` | :arrow_down: |
| [arrow/src/compute/kernels/sort.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9zb3J0LnJz) | `94.88% <0.00%> (-0.24%)` | :arrow_down: |
| [arrow/src/compute/kernels/concat.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jb25jYXQucnM=) | `100.00% <0.00%> (ø)` | |
| [arrow/src/compute/kernels/window.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy93aW5kb3cucnM=) | `100.00% <0.00%> (ø)` | |
| [arrow/src/compute/kernels/boolean.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9ib29sZWFuLnJz) | `96.76% <0.00%> (+0.04%)` | :arrow_up: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZW5jb2RpbmdzL2VuY29kaW5nLnJz) | `95.04% <0.00%> (+0.19%)` | :arrow_up: |
| ... and [4 more](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [18c804a...404991d](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] kszucs commented on pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#issuecomment-865163720
@jorgecarleitao created the relevant follow-up tickets and cleaned up the PR.
@pitrou didn't notice any outstanding problem, so I think this should be ready to go now.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] kszucs commented on a change in pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#discussion_r655509754
##########
File path: arrow/src/ffi.rs
##########
@@ -122,66 +132,61 @@ unsafe extern "C" fn release_schema(schema: *mut FFI_ArrowSchema) {
let schema = &mut *schema;
// take ownership back to release it.
- CString::from_raw(schema.format as *mut std::os::raw::c_char);
- CString::from_raw(schema.name as *mut std::os::raw::c_char);
- let private = Box::from_raw(schema.private_data as *mut SchemaPrivateData);
- for child in private.children_ptr.iter() {
- let _ = Box::from_raw(*child);
+ CString::from_raw(schema.format as *mut c_char);
+ if !schema.name.is_null() {
+ CString::from_raw(schema.name as *mut c_char);
+ }
+ if !schema.private_data.is_null() {
+ let private_data = Box::from_raw(schema.private_data as *mut SchemaPrivateData);
+ for child in private_data.children.iter() {
+ drop(Box::from_raw(*child))
Review comment:
That's my understanding as well.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter edited a comment on pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#issuecomment-857974778
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#439](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6768471) into [master](https://codecov.io/gh/apache/arrow-rs/commit/9f56afb2d2347310184706f7d5e46af583557bea?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9f56afb) will **decrease** coverage by `0.05%`.
> The diff coverage is `70.12%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #439 +/- ##
==========================================
- Coverage 82.65% 82.60% -0.06%
==========================================
Files 165 166 +1
Lines 45556 45654 +98
==========================================
+ Hits 37655 37711 +56
- Misses 7901 7943 +42
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow-pyarrow-integration-testing/src/lib.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3ctcHlhcnJvdy1pbnRlZ3JhdGlvbi10ZXN0aW5nL3NyYy9saWIucnM=) | `0.00% <0.00%> (ø)` | |
| [arrow/src/datatypes/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9tb2QucnM=) | `100.00% <ø> (ø)` | |
| [arrow/src/datatypes/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9mZmkucnM=) | `86.84% <86.84%> (ø)` | |
| [arrow/src/ffi.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2ZmaS5ycw==) | `85.95% <92.50%> (+3.10%)` | :arrow_up: |
| [arrow/src/array/array\_struct.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5X3N0cnVjdC5ycw==) | `87.84% <0.00%> (-0.59%)` | :arrow_down: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZW5jb2RpbmdzL2VuY29kaW5nLnJz) | `94.85% <0.00%> (-0.20%)` | :arrow_down: |
| [arrow/src/array/builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIucnM=) | `85.78% <0.00%> (-0.13%)` | :arrow_down: |
| [arrow/src/ipc/reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2lwYy9yZWFkZXIucnM=) | `84.30% <0.00%> (-0.04%)` | :arrow_down: |
| [parquet/src/record/api.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvcmVjb3JkL2FwaS5ycw==) | `92.48% <0.00%> (-0.03%)` | :arrow_down: |
| [arrow/src/array/ord.rs](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L29yZC5ycw==) | `63.33% <0.00%> (ø)` | |
| ... and [6 more](https://codecov.io/gh/apache/arrow-rs/pull/439/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [9f56afb...6768471](https://codecov.io/gh/apache/arrow-rs/pull/439?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] kszucs commented on pull request #439: [WIP] FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#issuecomment-857908771
@jorgecarleitao what is the rational behind wrapping `(FFI_ArrowArray, FFI_ArrowSchema)` with `ArrowArray` and the children with `ArrowArrayChild` ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] kszucs commented on pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#issuecomment-865163720
@jorgecarleitao created the relevant follow-up tickets and cleaned up the PR.
@pitrou didn't notice any outstanding problem, so I think this should be ready to go now.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] jorgecarleitao commented on pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#issuecomment-864577241
> @jorgecarleitao what do you think about implementing `arrow <-> pyarrow` interoperability directly in the `arrow-rs` repository and use that from the datafusion bindings?
I think that it is a good idea.
I am not sure how to execute on it though, as I never built a Rust library out of pyo3. E.g. DataFusion requires some conversions that are currently called from Rust (not Python). Is it enough for it to depend on "arrow-rs-python"?
So, I would say let's try it and see how this goes in terms of packaging and dependency management.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] kszucs commented on pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#issuecomment-866932981
@jorgecarleitao @alamb resolved conflicts with https://github.com/apache/arrow-rs/commit/86722748a950f3e7d1603aeddebdfa85f7ecbdf9
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] kszucs commented on pull request #439: Python FFI bridge for Schema, Field and DataType
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #439:
URL: https://github.com/apache/arrow-rs/pull/439#issuecomment-872302103
@jorgecarleitao may I go ahead and merge it?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org