-
Notifications
You must be signed in to change notification settings - Fork 591
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
perf(duckdb): speedup timestamp conversion by avoiding conversion to object #9556
Conversation
Still about 5x left that we're losing, but that can be done in a follow up. |
Ok found a few of the remaining Xs in a bug 🐞 |
Added a benchmark, which actually times out on |
7a2c13d
to
1bf015b
Compare
1bf015b
to
2757636
Compare
@@ -200,8 +200,10 @@ def convert_Boolean(cls, s, dtype, pandas_type): | |||
|
|||
@classmethod | |||
def convert_Timestamp(cls, s, dtype, pandas_type): | |||
if isinstance(dtype, pd.DatetimeTZDtype): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check was always False
, because dtype
is always an Ibis type.
7fdbba2
to
532c76e
Compare
532c76e
to
b7e6deb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, and I think the shift from ns
to us
precision is a non-issue here, since DuckDB doesn't support ns
timestamps in the first-place, so it's mostly a very small change in the representation of the results, but the values aren't fundamentally changing.
Redo of #9554. The slow bit here was object conversion of timestamps, which seemed to only be hit by this one test.