Visitar URL original
[PythonDev] Don't dereference None when creating pandas dataframe by Tishj · Pull Request #9127 · duckdb/duckdb · GitHub
Skip to content

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

Merged
Mytherin merged 6 commits into
duckdb:mainfrom
Tishj:dont_deref_none
Sep 28, 2023
Merged

[PythonDev] Don't dereference None when creating pandas dataframe#9127
Mytherin merged 6 commits into
duckdb:mainfrom
Tishj:dont_deref_none

Conversation

@Tishj

@Tishj Tishj commented Sep 27, 2023

Copy link
Copy Markdown
Member

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

Tishj commented Sep 27, 2023

Copy link
Copy Markdown
Member Author

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

Mytherin commented Sep 27, 2023

Copy link
Copy Markdown
Member

Perhaps try Py_RETURN_NONE?

Mause
Mause suggested changes Sep 27, 2023
View reviewed changes
Comment thread tools/pythonpkg/tests/fast/pandas/test_pandas_df_none.py Outdated
@Mytherin
Mytherin marked this pull request as ready for review September 27, 2023 11:24
@pankajp

pankajp commented Sep 27, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Mytherin commented Sep 27, 2023

Copy link
Copy Markdown
Member

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

@Tishj

Tishj commented Sep 27, 2023

Copy link
Copy Markdown
Member Author

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
Comment thread tools/pythonpkg/tests/fast/pandas/test_pandas_df_none.py Outdated
@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

Mytherin commented Sep 28, 2023

Copy link
Copy Markdown
Member

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