alamb commented on code in PR #8009:
URL: https://github.com/apache/arrow-rs/pull/8009#discussion_r2237842657
##########
.github/workflows/arrow.yml:
##########
@@ -68,7 +68,10 @@ jobs:
- name: Test arrow-schema
run: cargo test -p arrow-schema --all-features
- name: Test arrow-array
- run: cargo test -p arrow-array --all-features
+ run: |
+ cargo test -p arrow-array --all-features
+ # Disable feature `force_validate`
+ cargo test -p arrow-array
Review Comment:
I suggest we explicitly run with the ffi feature too
```suggestion
cargo test -p arrow-array --features=ffi
```
##########
arrow-array/src/ffi.rs:
##########
@@ -408,7 +408,17 @@ impl ImportedArrowArray<'_> {
.map(|index| {
let len = self.buffer_len(index, variadic_buffer_lens,
&self.data_type)?;
match unsafe { create_buffer(self.owner.clone(), self.array,
index, len) } {
- Some(buf) => Ok(buf),
+ Some(buf) => {
+ // External libraries may use a dangling pointer for a
buffer with length 0.
+ // We respect the array length specified in the C Data
Interface. Actually,
Review Comment:
I don't understand this comment. I guess in my mind "if the length is
incorrect we can't make a good buffer" is "obvious" but maybe it is not for
other readers
##########
arrow-array/src/ffi.rs:
##########
@@ -1660,6 +1676,25 @@ mod tests_from_ffi {
}
}
+ #[test]
+ #[cfg(not(feature = "force_validate"))]
Review Comment:
I don't understand why this can't work with `force_validate`
I ran the test without this line and it seems to work fine:
```diff
diff --git a/arrow-array/src/ffi.rs b/arrow-array/src/ffi.rs
index 2ee2fd379e..5b05bcd81d 100644
--- a/arrow-array/src/ffi.rs
+++ b/arrow-array/src/ffi.rs
@@ -621,7 +621,6 @@ mod tests_to_then_from_ffi {
}
#[test]
- #[cfg(not(feature = "force_validate"))]
fn test_decimal_round_trip() -> Result<()> {
// create an array natively
let original_array = [Some(12345_i128), Some(-12345_i128), None]
@@ -1523,7 +1522,6 @@ mod tests_from_ffi {
}
#[test]
- #[cfg(not(feature = "force_validate"))]
fn test_empty_string_with_non_zero_offset() -> Result<()> {
use super::ImportedArrowArray;
use arrow_buffer::{MutableBuffer, OffsetBuffer};
@@ -1677,7 +1675,6 @@ mod tests_from_ffi {
}
#[test]
- #[cfg(not(feature = "force_validate"))]
fn test_utf8_view_ffi_from_dangling_pointer() {
let empty =
GenericByteViewBuilder::<StringViewType>::new().finish();
let buffers = empty.data_buffers().to_vec();
```
```shell
cargo test -p arrow-array --features=ffi
```
test result: ok. 185 passed; 0 failed; 1 ignored; 0 measured; 0 filtered
out; finished in 24.13s
```
##########
arrow-array/src/ffi.rs:
##########
@@ -1660,6 +1676,25 @@ mod tests_from_ffi {
}
}
+ #[test]
+ #[cfg(not(feature = "force_validate"))]
Review Comment:
I don't understand why this can't work with `force_validate`
I ran the test without this line and it seems to work fine:
```diff
diff --git a/arrow-array/src/ffi.rs b/arrow-array/src/ffi.rs
index 2ee2fd379e..5b05bcd81d 100644
--- a/arrow-array/src/ffi.rs
+++ b/arrow-array/src/ffi.rs
@@ -621,7 +621,6 @@ mod tests_to_then_from_ffi {
}
#[test]
- #[cfg(not(feature = "force_validate"))]
fn test_decimal_round_trip() -> Result<()> {
// create an array natively
let original_array = [Some(12345_i128), Some(-12345_i128), None]
@@ -1523,7 +1522,6 @@ mod tests_from_ffi {
}
#[test]
- #[cfg(not(feature = "force_validate"))]
fn test_empty_string_with_non_zero_offset() -> Result<()> {
use super::ImportedArrowArray;
use arrow_buffer::{MutableBuffer, OffsetBuffer};
@@ -1677,7 +1675,6 @@ mod tests_from_ffi {
}
#[test]
- #[cfg(not(feature = "force_validate"))]
fn test_utf8_view_ffi_from_dangling_pointer() {
let empty =
GenericByteViewBuilder::<StringViewType>::new().finish();
let buffers = empty.data_buffers().to_vec();
```
```shell
cargo test -p arrow-array --features=ffi
```
```
test result: ok. 185 passed; 0 failed; 1 ignored; 0 measured; 0 filtered
out; finished in 24.13s
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
