GH-43487: [Python] Sanitize Python reference handling in UDF implementation#43557
GH-43487: [Python] Sanitize Python reference handling in UDF implementation#43557pitrou merged 1 commit intoapache:mainfrom
Conversation
|
|
|
@github-actions crossbow submit -g python -g wheel |
This comment was marked as outdated.
This comment was marked as outdated.
|
@rtpsw You're one of the people who made significant changes to the UDF implementation, can you perhaps take a look? |
|
@lysnikolaou Are you able to test this PR to see if this solves your issue? |
|
@pitrou Thanks for having a look at this. Confirmed that the whole test suite passes when run under a debug build of 3.13 with this PR. |
|
Thanks @lysnikolaou . By the way, do you build a debug mode Python yourself? Or is there a Conda package available somewhere? |
I'm either building by myself or using |
Apologies, I currently don't have the resources to handle this. |
…plementation 1. Remove spurious increfs (the function object is already incref'ed at an upper level) 2. Add unit test with an ephemeral Python function object 3. Streamline and improve Python reference handling
c22df05 to
ac26d0d
Compare
|
@github-actions crossbow submit -g python -g wheel |
|
Revision: ac26d0d Submitted crossbow builds: ursacomputing/crossbow @ actions-522226e29b
|
|
Our new test-conda-python-3.12-cpython-debug CI job now passes. |
|
@pitrou I can help review the best I can but please give me a little bit of time. I will try to get to this tomorrow. |
jorisvandenbossche
left a comment
There was a problem hiding this comment.
I am not very familiar with the UDF code, but I can follow the changes and that looks good to me
icexelloss
left a comment
There was a problem hiding this comment.
IIUC this correctly, the new changes will only increase the refcount once UDF is registered. Since now there is a shared_ptr of OwnedRef in the registered function, it will not decrease ref until the function is unregistered or the registry goes away.
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 1f24799. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 23 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Uh oh!
There was an error while loading. Please reload this page.