pyarrow: Wrap capsule extraction errors into an explicit error by Tpt · Pull Request #9660 · apache/arrow-rs

Tpt · 2026-04-03T15:44:01Z

Remove validate_pycapsule: pointer_checked already raises an exception if the capsule type is wrong, and we wrap it into a nicer error

Also relies on a trait to associate types with their capsules names

Because we wrap the exception (equivalent of raise X from Y), the original exception is still there

Follow up of #9639 #9594

Tpt · 2026-04-16T17:02:00Z

@alamb Thank you for the rebase. This MR is blocked by a segfault. I have not understood why yet and put this MR on hold to go back to it with a fresh mind. I will try to debug that in the next few days. Sorry for having it been stalled for so long.

Update: got it working, there was a use-after-free error.

Remove validate_pycapsule: pointer_checked already raises an exception if the capsule type is wrong, and we wrap it into a nicer error

Tpt · 2026-06-17T05:34:52Z

@timsaucer I would love to get some review on this MR. It cleans up both code and error messages

timsaucer

It's a nice cleanup. Adding a trait that just contains a constant seems like it adds complexity with minimal value. Otherwise I'm good with the changes.

timsaucer · 2026-06-17T12:29:53Z

+trait PyCapsuleType {
+    const NAME: &CStr;
+}
+
+impl PyCapsuleType for FFI_ArrowSchema {
+    const NAME: &CStr = c"arrow_schema";
+}
+
+impl PyCapsuleType for FFI_ArrowArray {
+    const NAME: &CStr = c"arrow_array";
+}
+
+impl PyCapsuleType for FFI_ArrowArrayStream {
+    const NAME: &CStr = c"arrow_array_stream";
+}


This seems like overkill. Why not just keep these as constants like we had before and pass them as needed?

My motivation for the trait is to have a single place to associate a type with its capsule name (the impl block) and to avoid the possible mistake of using a wrong capsule name at an extract_capsule call site. This also allows to keep the capsule name concern outside of the main code logic.

No other motivation outside of this small "fool-proofing". Glad to remove it if you prefer.

I'm not a committer on this repo, so I'd leave it to them for final say. My recommendation would be to keep it simpler.

Other than that, it's a very nice cleanup and I may steal this idea over in datafusion-python.

Nice! I have also ideas for follow ups (use string interning...). Very happy to help also on the datafusion-python side.

I agree with @timsaucer -- keeping the code simpler would be my preference but this looks ok too

Trait removed: 2ab7223 (fun fact, the number of LOC has not changed)

alamb

Looks like a nice improvement to me -- thanks @Tpt

I defer to @timsaucer (or @kylebarron ) for the specifics

alamb · 2026-06-17T14:55:04Z

+trait PyCapsuleType {
+    const NAME: &CStr;
+}
+
+impl PyCapsuleType for FFI_ArrowSchema {
+    const NAME: &CStr = c"arrow_schema";
+}
+
+impl PyCapsuleType for FFI_ArrowArray {
+    const NAME: &CStr = c"arrow_array";
+}
+
+impl PyCapsuleType for FFI_ArrowArrayStream {
+    const NAME: &CStr = c"arrow_array_stream";
+}


I agree with @timsaucer -- keeping the code simpler would be my preference but this looks ok too

kylebarron · 2026-06-17T18:06:50Z

        assert!(err.is_instance_of::<PyTypeError>(py));
        assert_eq!(
            err.to_string(),
-            "TypeError: Expected __arrow_c_array__ to return a tuple of (schema, array) capsules."


Not sure if it's necessary here because these are integration tests, not user-facing, but I think the old language was more explicit

Indeed. I have reverted the error message change: 1f72b24

kylebarron · 2026-06-17T18:14:34Z

Sorry I can't tell what the difference is with this PR. What's the user-facing difference between before and after?

Tpt · 2026-06-17T19:09:56Z

Sorry I can't tell what the difference is with this PR. What's the user-facing difference between before and after?

Sorry, I should have made it more explicit. 56218cd add a test to highlight the change.

Before the change, when calling a method that must return a capsule but does not actually return one:

TypeError: 'int' object is not an instance of 'PyCapsule'

After:

TypeError: Expected arrow_c_schema to return a capsule.

alamb · 2026-06-22T18:14:23Z

Thank you @Tpt @timsaucer and @kylebarron

Tpt force-pushed the tpt/pyarrow-capsule branch 2 times, most recently from aa910ff to 61391e3 Compare April 4, 2026 17:21

Tpt force-pushed the tpt/pyarrow-capsule branch from f0b61da to 7ecba54 Compare April 16, 2026 19:28

Tpt marked this pull request as ready for review April 16, 2026 19:29

Tpt force-pushed the tpt/pyarrow-capsule branch from b1880b6 to 240fd32 Compare April 17, 2026 09:21

Tpt added 3 commits June 16, 2026 17:01

pyarrow: Wrap capsule extraction errors into an explicit error

9497c5c

Remove validate_pycapsule: pointer_checked already raises an exception if the capsule type is wrong, and we wrap it into a nicer error

Fix use after free error

c1a2860

Fix error message for RecordBatch from_pyarrow_bound

6c51b43

Tpt force-pushed the tpt/pyarrow-capsule branch from 240fd32 to 6c51b43 Compare June 16, 2026 15:03

Factorize checking if capsule method exists

e730f3c

timsaucer reviewed Jun 17, 2026

View reviewed changes

alamb approved these changes Jun 17, 2026

View reviewed changes

Remove PyCapsuleType trait

2ab7223

alamb approved these changes Jun 17, 2026

View reviewed changes

kylebarron reviewed Jun 17, 2026

View reviewed changes

Tpt added 2 commits June 17, 2026 20:59

Keep tuple error message

1f72b24

Add an error test

56218cd

Tpt force-pushed the tpt/pyarrow-capsule branch from c95726c to 56218cd Compare June 17, 2026 19:07

rustfmt

b4231e0

kylebarron approved these changes Jun 18, 2026

View reviewed changes

alamb merged commit 45f008a into apache:main Jun 22, 2026
14 checks passed

Uh oh!

Conversation

Tpt commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tpt commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tpt commented Jun 17, 2026

Uh oh!

timsaucer left a comment

Choose a reason for hiding this comment

Uh oh!

timsaucer Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

Tpt Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timsaucer Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

timsaucer Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

Tpt Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

alamb Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

Tpt Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

kylebarron Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

Tpt Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kylebarron commented Jun 17, 2026

Uh oh!

Tpt commented Jun 17, 2026

Uh oh!

Uh oh!

alamb commented Jun 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Tpt commented Apr 3, 2026 •

edited

Loading

Tpt commented Apr 16, 2026 •

edited

Loading

Tpt Jun 17, 2026 •

edited

Loading

Tpt Jun 17, 2026 •

edited

Loading

Tpt Jun 17, 2026 •

edited

Loading