GH-38798: [Integration] Enable C Data Interface integration testing on Rust#38799
GH-38798: [Integration] Enable C Data Interface integration testing on Rust#38799pitrou merged 2 commits intoapache:mainfrom
Conversation
|
|
There was a problem hiding this comment.
Do you want to add your name or an issue ID to this or is it fine to keep it as is in these scripts?
There was a problem hiding this comment.
I think we can open an issue, though this depends on Rust exposing appropriate primitives:
apache/arrow-rs#5080 (comment)
There was a problem hiding this comment.
Cand self.ffi.string(..) fail and if that happens is it safe to pass rs_error to arrow_rs_free_error in the finally block?
There was a problem hiding this comment.
That's unrelated. rs_error is valid regardless of whether self.ffi.string fails or not (I suppose it can raise MemoryError if it fails allocating the Python bytestring, though that's highly unlikely).
There was a problem hiding this comment.
This is the function allocating the Rust error string btw:
https://github.com/apache/arrow-rs/blob/df69ef57d055453c399fa925ad315d19211d7ab2/arrow-integration-testing/src/lib.rs#L246-L252
github-actions
bot
added
awaiting committer review
pitrou
force-pushed
the
rust-cdata-integration
branch
from
dde5927 to
f0ce845
Compare
|
Thanks for the review @felipecrv ! I'm going to merge now. |
|
@tustvold Hopefully this will work fine on the arrow-rs repo. |
|
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 9a36c42. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them. |
|
Looks like it might not be quite working - https://github.com/apache/arrow-rs/actions/runs/6954459872/job/18921320582#step:10:1 |
|
Thank you that seems to have done the trick 👍 |
…ting on Rust (apache#38799) ### Rationale for this change Arrow Rust has added entrypoints for C Data Interface integration testing, so this can now be enabled on our side: apache/arrow-rs#5080 ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * Closes: apache#38798 Authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>
3 participants
Rationale for this change
Arrow Rust has added entrypoints for C Data Interface integration testing, so this can now be enabled on our side:
apache/arrow-rs#5080
Are these changes tested?
Yes.
Are there any user-facing changes?
No.