GH-43487: [Python] Sanitize Python reference handling in UDF implementation

[pitrou]

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

• GitHub Issue: [Python] SEGFAULT in test_udf_via_substrait when run under CPython debug build #43487

[github-actions]

⚠️ GitHub issue #43487 has been automatically assigned in GitHub to PR creator.

[pitrou]

@github-actions crossbow submit -g python -g wheel

[pitrou]

@rtpsw You're one of the people who made significant changes to the UDF implementation, can you perhaps take a look?

[pitrou]

@lysnikolaou Are you able to test this PR to see if this solves your issue?

[lysnikolaou]

@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.

[pitrou]

Thanks @lysnikolaou . By the way, do you build a debug mode Python yourself? Or is there a Conda package available somewhere?

[lysnikolaou]

By the way, do you build a debug mode Python yourself?

I'm either building by myself or using pyenv.

[rtpsw]

@rtpsw You're one of the people who made significant changes to the UDF implementation, can you perhaps take a look?

Apologies, I currently don't have the resources to handle this.

[pitrou]

@github-actions crossbow submit -g python -g wheel

[github-actions]

Revision: ac26d0d

Submitted crossbow builds: ursacomputing/crossbow @ actions-522226e29b

Task	Status
example-python-minimal-build-fedora-conda
example-python-minimal-build-ubuntu-venv
test-conda-python-3.10
test-conda-python-3.10-cython2
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest-numpy-1.26
test-conda-python-3.10-pandas-latest-numpy-latest
test-conda-python-3.10-pandas-nightly-numpy-nightly
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-upstream_devel-numpy-nightly
test-conda-python-3.11-spark-master
test-conda-python-3.12
test-conda-python-3.12-cpython-debug
test-conda-python-3.8
test-conda-python-3.8-pandas-1.0-numpy-1.19
test-conda-python-3.9
test-conda-python-3.9-pandas-latest-numpy-latest
test-conda-python-emscripten
test-cuda-python
test-debian-12-python-3-amd64
test-debian-12-python-3-i386
test-fedora-39-python-3
test-ubuntu-20.04-python-3
test-ubuntu-22.04-python-3
wheel-macos-big-sur-cp310-arm64
wheel-macos-big-sur-cp311-arm64
wheel-macos-big-sur-cp312-arm64
wheel-macos-big-sur-cp38-arm64
wheel-macos-big-sur-cp39-arm64
wheel-macos-catalina-cp310-amd64
wheel-macos-catalina-cp311-amd64
wheel-macos-catalina-cp312-amd64
wheel-macos-catalina-cp38-amd64
wheel-macos-catalina-cp39-amd64
wheel-manylinux-2-28-cp310-amd64
wheel-manylinux-2-28-cp310-arm64
wheel-manylinux-2-28-cp311-amd64
wheel-manylinux-2-28-cp311-arm64
wheel-manylinux-2-28-cp312-amd64
wheel-manylinux-2-28-cp312-arm64
wheel-manylinux-2-28-cp38-amd64
wheel-manylinux-2-28-cp38-arm64
wheel-manylinux-2-28-cp39-amd64
wheel-manylinux-2-28-cp39-arm64
wheel-manylinux-2014-cp310-amd64
wheel-manylinux-2014-cp310-arm64
wheel-manylinux-2014-cp311-amd64
wheel-manylinux-2014-cp311-arm64
wheel-manylinux-2014-cp312-amd64
wheel-manylinux-2014-cp312-arm64
wheel-manylinux-2014-cp38-amd64
wheel-manylinux-2014-cp38-arm64
wheel-manylinux-2014-cp39-amd64
wheel-manylinux-2014-cp39-arm64
wheel-windows-cp310-amd64
wheel-windows-cp311-amd64
wheel-windows-cp312-amd64
wheel-windows-cp38-amd64
wheel-windows-cp39-amd64

[pitrou]

Our new test-conda-python-3.12-cpython-debug CI job now passes.

[icexelloss]

@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]

I am not very familiar with the UDF code, but I can follow the changes and that looks good to me

[icexelloss]

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.

[conbench-apache-arrow]

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.