Skip to content

REF: use datetime C API instead of getattrs #51368

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.

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

Closed

Conversation

jbrockmendel
Copy link
Member

@WillAyd i think there are a couple of paths that must not be reached, since it looks like they pass np.datetime64 objects to these pydatetime-requiring functions. in python/cython id check that with an assert False. have a favorite way of doing that in c code?

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea think we need to approach the implementation differently though

@@ -79,7 +79,9 @@ char *PyDateTimeToIso(PyObject *obj, NPY_DATETIMEUNIT base,
npy_datetimestruct dts;
int ret;

ret = convert_pydatetime_to_datetimestruct(obj, &dts);
PyDateTime_IMPORT;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the cpython source the datetime library seems to be implemented as a capsule. There's some documentaiton on that here:

https://docs.python.org/3/extending/extending.html#providing-a-c-api-for-an-extension-module

I really don't want to keep scattering these imports throughout the code base because it goes against the CPython documentation - can you check if the capsule approach would work?

ret = convert_pydatetime_to_datetimestruct(obj, &dts);
PyDateTime_IMPORT;

ret = convert_pydatetime_to_datetimestruct((PyDateTime_Date*)obj, &dts);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure to avoid blind casting - these should all be preceded by a PyDate_Check call with appropriate error handling, otherwise its anybody guess what happens when you pass the wrong object type to this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so something like

    PyDateTime_IMPORT;
    if (!PyDate_Check(obj)) {
        PyErr_SetString(PyExc_ValueError,
                        "Object is not a datetime.date object");
        return NULL;
    }
    PyDateTime_Date *dtobj = (PyDateTime_Date*)obj;

    ret = convert_pydatetime_to_datetimestruct(dtobj, &dts);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in objToJSON.c L1552 we call PyDateTimeToEpoch but IIUC obj ther eis a np.datetime64 object, not a pydatetime object. Isn't that call doomed to failure?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea you have got the right idea with that error handling - nice job

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that is the case I think that behavior would be undefined, so could segfault or get garbage values. Do you know of a reproducer?

if (PyObject_HasAttrString(obj, "tzinfo")) {
PyObject *offset = extract_utc_offset(obj);
// TODO(py3.10): in py3.10 we can use PyDateTime_DATE_GET_TZINFO
PyObject *offset = extract_utc_offset((PyObject*)obj);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this cast necessary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without it i see a bunch of warnings

warning: incompatible pointer types passing 'PyDateTime_DateTime *' to parameter of type 'PyObject *' (aka 'struct _object *') [-Wincompatible-pointer-types]
        PyObject *offset = extract_utc_offset(obj);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha makes sense

@jbrockmendel
Copy link
Member Author

it works locally, but im seeing build failures from the type checking

@jbrockmendel
Copy link
Member Author

Closing as not worth the trouble.

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