-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Simplify and speed up timestamp parsing #3063
base: develop
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #3063 +/- ##
==========================================
Coverage ? 93.06%
==========================================
Files ? 66
Lines ? 14472
Branches ? 0
==========================================
Hits ? 13468
Misses ? 1004
Partials ? 0 ☔ View full report in Codecov by Sentry. |
@nateprewitt I have some tests you seem to have added for #1987 failing with this
Might I be right in guessing the |
This comment was marked as outdated.
This comment was marked as outdated.
(Gentle review nudge, @nateprewitt? 😄) |
Another nudge, @nateprewitt... I noticed you'd said
in #3062 – anything I can do to help things along? |
Sorry for Yet Another Ping, @nateprewitt 😅 |
Another ping... @nateprewitt? |
0631762
to
a5866ba
Compare
Rebased post conflicts from #3206... |
I decided to just add the optimization commit I'd mentioned in a previous comment in here, because why not – after all this PR has been brewing for over a year so might as well. Benchmark results for parsing timestamps found in the test suite comparing 05538ce and this PR, showing a mean OPS improvement of 16.75×:
|
53 ops/s vs 2.8 ops/s
As mentioned in #2972 (comment):
_parse_timestamp_with_tzinfo()
already attempts to dofromtimestamp
parsing; not much point in doing that work twice for timestamp-esque strings (and failing in the cases described in #2972).In the subsequent commits, this PR also cleans up remaining seemingly Useless Use of
tzlocal
.