Skip to content

[PythonDev] Don't dereference None when creating pandas dataframe #9127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
Mytherin merged 6 commits into from
Sep 28, 2023
Merged

[PythonDev] Don't dereference None when creating pandas dataframe #9127

Mytherin merged 6 commits into from
Sep 28, 2023

Conversation

@Tishj
Copy link
Contributor

@Tishj Tishj commented Sep 27, 2023

This PR fixes #9110

When creating a pandas dataframe, we first create Numpy arrays.
Recently we changed the logic for pandas so None is returned for NULL when converting a varchar column.
Because we use Py_None this is an object and numpy takes ownership of it, lowering the refcount when it gets deleted.

Instead we should return nullptr so Numpy knows that it's not a none.

@github-actions github-actions bot marked this pull request as draft September 27, 2023 10:45
@Tishj
Copy link
Contributor Author

Tishj commented Sep 27, 2023

tools/pythonpkg/tests/fast/pandas/test_pyarrow_filter_pushdown.py is now causing a segfault because of this
So this doesn't look like the solution

Even though it does stop the initial problem
Should we just increase the refcount whenever we return None like this?

@Mytherin
Copy link
Collaborator

Mytherin commented Sep 27, 2023

Perhaps try Py_RETURN_NONE?

Mause
Mause suggested changes Sep 27, 2023
View reviewed changes
@Mytherin Mytherin marked this pull request as ready for review September 27, 2023 11:24
@pankajp
Copy link
Contributor

pankajp commented Sep 27, 2023 •
edited
Loading

I am encountering this abort too. Will we have a 0.9.1 release including this fix for the crash (and any other bug fixes) soon?

Edit: I'm working around it by using pd.DataFrame(conn.fetchnumpy()), hopefully they are equivalent (conversion from arrow isn't for category types)

@Mytherin
Copy link
Collaborator

Mytherin commented Sep 27, 2023

We are planning to push a v0.9.1 bugfix release, but not for a few days at least

@Tishj
Copy link
Contributor Author

Tishj commented Sep 27, 2023

I am encountering this abort too. Will we have a 0.9.1 release including this fix for the crash (and any other bug fixes) soon?

Edit: I'm working around it by using pd.DataFrame(conn.fetchnumpy()), hopefully they are equivalent (conversion from arrow isn't for category types)

Hi, yes they are essentially equivalent apart from a couple small things

@github-actions github-actions bot marked this pull request as draft September 28, 2023 09:43
@Tishj Tishj marked this pull request as ready for review September 28, 2023 09:44
pankajp
pankajp reviewed Sep 28, 2023
View reviewed changes
@github-actions github-actions bot marked this pull request as draft September 28, 2023 15:54
@Tishj Tishj marked this pull request as ready for review September 28, 2023 18:39
@Mytherin Mytherin merged commit 709ea29 into duckdb:main Sep 28, 2023
@Mytherin
Copy link
Collaborator

Mytherin commented Sep 28, 2023

Thanks!

@Tishj Tishj deleted the dont_deref_none branch November 7, 2025 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mytherin Mytherin Mytherin left review comments

+2 more reviewers

@pankajp pankajp pankajp left review comments

@Mause Mause Mause approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

fetchdf in DuckDB 0.9.0 is deallocating None

4 participants

@Tishj @Mytherin @pankajp @Mause