Skip to content

Conversation

Tishj
Copy link
Contributor

@Tishj Tishj commented Apr 10, 2023

When reading from file-like objects with the python client, we rely on fsspec to create an in-memory filesystem to store them.

The import cache is utilized for this, but a recent change that allows importing optional attributes/modules is causing problems here. Because the import silently fails, the object.ptr() is left as nullptr.
When the object's constructor is called, this causes a segfault.

I've fixed this here by checking if the object could not be imported before calling the constructor, but there are likely other places where this could become a problem.

In #7005 I've made changes to py::isinstance to support nullptr objects, maybe a better change would be to modify py::isinstance to take a PythonImportCacheItem object instead, and throw from the operator () of the item if it's called on an item that wasn't imported.

Also, I can't add a test for this, I've tried monkeypatching, but in the pyduckdb/filesystem.py we import fsspec, overriding the monkeypatch.

@Mytherin
Copy link
Collaborator

Mytherin commented Apr 10, 2023

Thanks for the fix! LGTM. Perhaps we can turn the PyObject * in the ImportCache into an optional_ptr<PyObject> instead, to avoid such problems in the future?

@Mytherin Mytherin merged commit 7312132 into duckdb:master Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants