Skip to content

gh-80620: Support negative values in fromtimestamp on Windows using 0 + timedelta #134461

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

jhohm
Copy link
Contributor

@jhohm jhohm commented May 21, 2025

Support negative values in datetime.date.fromtimestamp and datetime.datetime.fromtimestamp on Windows, by substituting 0 and using datetime.timedelta to go back in time.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

It's almost good, I just have a few last nitpicks :)

@@ -3241,6 +3241,13 @@ date_new(PyTypeObject *type, PyObject *args, PyObject *kw)
return self;
}

static PyObject *add_datetime_timedelta(PyDateTime_DateTime *date,
PyDateTime_Delta *delta,
int factor);
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the duplicated definition line 3762?

Can you also move these two definitions at line 154? In the "Forward declarations" block.

Comment on lines +3262 to +3263
if (_PyTime_localtime(0, &tm) != 0)
return NULL;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (_PyTime_localtime(0, &tm) != 0)
return NULL;
if (_PyTime_localtime(0, &tm) != 0) {
return NULL;
}

if (delta == NULL) {
Py_DECREF(date);
return NULL;
}
Copy link
Member

Choose a reason for hiding this comment

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

You can move negate and result definitions here. There is no need to initialize result to NULL.

if (delta == NULL) {
Py_DECREF(dt);
return NULL;
}
Copy link
Member

Choose a reason for hiding this comment

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

You can move result definition here. There is no need to initialize it to NULL.

@pganssle
Copy link
Member

Thanks for this PR!

I think that it is a net benefit over what we have today, but I do think it's probably pretty inefficient, and considering we're already in C-land, maybe we can just directly call the C functions underlying timedelta addition instead of constructing a timedelta for this?

I think what you need to do to accomplish that is basically convert the timestamp parameter to an integer number of seconds and microseconds, then do something like:

int year = 1970;
int month = 1;
int day = 1;
int hour = 0;
int minute = 0;
int second = seconds;
int microsecond = microseconds;
normalize_datetime(&year, &month, &day, &hour, &minute, &second, &microsecond);

Then construct a datetime object with the results.

@vstinner
Copy link
Member

Calling normalize_datetime() doesn't work since seconds has time_t time, whereas the function only accepts int.

@pganssle
Copy link
Member

Calling normalize_datetime() doesn't work since seconds has time_t time, whereas the function only accepts int.

I am suggesting that we make the proper conversions, which will be almost certainly be dramatically more efficient than converting everything to timedelta and constructing at least 2 datetime objects.

In the end it's not a big deal to be inefficient here, because it's a pretty rare situation that wasn't even possible before, but we might as well do it now rather than wait for someone to notice and show up with a new issue "negative timestamps on windows are dramatically slower than positive ones!"

@pganssle
Copy link
Member

Actually it occurs to me that if we just split into seconds and microseconds we will overflow in the late 19th century. I think it is best to convert to (hours, seconds, microseconds), then normalize the datetime. That will give 244k years, which is well outside the range supported by datetime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants