Skip to content

gh-59648: Nanosecond support for datetime #92078

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
Draft
wants to merge 85 commits into
base: main
Choose a base branch
from
Draft

gh-59648: Nanosecond support for datetime #92078

wants to merge 85 commits into from
+819 −425

Conversation

SmartManoj
Copy link

@SmartManoj SmartManoj commented Apr 30, 2022 •
edited
Loading

Adds nanosecond as a keyword argument for backward compatibility

Solves: #59648

@bedevere-bot
Copy link

Every change to Python requires a NEWS entry.

Please, add it using the blurb_it Web app or the blurb command-line tool.

MojoVampire
MojoVampire requested changes May 3, 2022
View reviewed changes
Copy link
Contributor

@MojoVampire MojoVampire left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Summary:

  1. Don't apply automatic style fixes; it bloats the patch and makes it much harder to review
  2. Using Decimal the way you're using it is inherently unsafe due to unknown contextual precision, and will make the code slower to boot. Find a way to do your work with int when using float becomes impossible. Bonus: It won't make importing datetime force you to load Decimal in the process (a relatively heavyweight module)
  3. Several comparison and hash functions aren't using the submicrosecond data, so the extra data gets ignored, which seems like a problem.
  4. You broke pickling. Please don't do that.
  5. (A biggy I couldn't comment on inline) This only changes the Python level datetime module, that CPython itself doesn't even use, by and large. Any change to the Python level code in datetime.py requires a complementary change in _datetimemodule.c so the C accelerated version of the module has the same behavior (you may have avoided at least a few test failures because they were testing the C module and didn't notice the broken code in the Python module)

@rhettinger
Copy link
Contributor

This API looks reasonable to me. @tim-one What do you think?

FlorianLudwig
FlorianLudwig reviewed Jun 13, 2022
View reviewed changes
@bedevere-app bedevere-app bot mentioned this pull request Jan 28, 2025
Open
FichteFoll
FichteFoll reviewed Jan 29, 2025
View reviewed changes
@pganssle
Copy link
Member

@SmartManoj I see that you are diligently working on this, but I think that what we lack to solve this issue is not an implementation but rather some decisions about what the interface is going to look like.

Feel free to continue working on a proof of concept, but my recommendation here would be to start with a proposal that lays out the unresolved issues with the current designs and how you plan to address them, plus any other details about trade-offs that will need to be made.

@SmartManoj
Copy link
Author

SmartManoj commented Apr 28, 2025 •
edited
Loading

New interface: nanosecond as a keyword argument for backward compatibility.

Currently porting out the Python code to C.

test_backdoor_resistance is not needed, as datetime currently requires 12 bytes?

cpython/Lib/test/datetimetester.py

Lines 1961 to 1985 in 1b7470f

def test_backdoor_resistance(self):
# For fast unpickling, the constructor accepts a pickle byte string.
# This is a low-overhead backdoor. A user can (by intent or
# mistake) pass a string directly, which (if it's the right length)
# will get treated like a pickle, and bypass the normal sanity
# checks in the constructor. This can create insane objects.
# The constructor doesn't want to burn the time to validate all
# fields, but does check the month field. This stops, e.g.,
# datetime.datetime('1995-03-25') from yielding an insane object.
base = b'1995-03-25'
if not issubclass(self.theclass, datetime):
base = base[:4]
for month_byte in b'9', b'\0', b'\r', b'\xff':
self.assertRaises(TypeError, self.theclass,
base[:2] + month_byte + base[3:])
if issubclass(self.theclass, datetime):
# Good bytes, but bad tzinfo:
with self.assertRaisesRegex(TypeError, '^bad tzinfo state arg$'):
self.theclass(bytes([1] * len(base)), 'EST')
for ord_byte in range(1, 13):
# This shouldn't blow up because of the month byte alone. If
# the implementation changes to do more-careful checking, it may
# blow up because other fields are insane.
self.theclass(base[:2] + bytes([ord_byte]) + base[3:])

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StanFromIreland StanFromIreland StanFromIreland requested changes

@FlorianLudwig FlorianLudwig Awaiting requested review from FlorianLudwig

@MojoVampire MojoVampire Awaiting requested review from MojoVampire

@FichteFoll FichteFoll Awaiting requested review from FichteFoll

@pganssle pganssle Awaiting requested review from pganssle pganssle is a code owner

@abalkin abalkin Awaiting requested review from abalkin abalkin is a code owner

Assignees

@tim-one tim-one

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@SmartManoj @bedevere-bot @rhettinger @StanFromIreland @pganssle @FlorianLudwig @FichteFoll @MojoVampire @tim-one