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

Fix issue when converting datetimes and pandas Timestamps in an expression #1097

Merged
merged 6 commits into from May 26, 2015

Conversation

Projects
None yet
2 participants
@cpcloud
Member

cpcloud commented May 20, 2015

@cpcloud cpcloud added this to the 0.8.1 milestone May 20, 2015

@cpcloud cpcloud added the bug label May 20, 2015

@cpcloud cpcloud self-assigned this May 20, 2015

cpcloud added some commits May 20, 2015

@cpcloud

This comment has been minimized.

Member

cpcloud commented May 20, 2015

@mrocklin okay with this?

@cpcloud cpcloud changed the title from Fix issue when storing datetimes and pandas Timestamps in an expression to Fix issue when converting datetimes and pandas Timestamps in an expression May 20, 2015

@@ -63,7 +64,7 @@ def _print_python(expr, leaves=None):
@dispatch((datetime.datetime, datetime.date))
def _print_python(expr, leaves=None):
return repr(expr), {'datetime': datetime}
return repr(expr), {'datetime': datetime, 'Timestamp': pd.Timestamp}

This comment has been minimized.

@mrocklin

mrocklin May 20, 2015

Member

Are there cases where we intended to print python datetime objects that we actually end up printing Pandas Timestamps? If so is there an issue? Do we ever not assume that pandas is around?

This comment has been minimized.

@cpcloud

cpcloud May 20, 2015

Member

Are there cases where we intended to print python datetime objects that we actually end up printing Pandas Timestamps?

Not sure. FWIW, Timestamp is a subclass of datetime.datetime.

Do we ever not assume that pandas is around?

No, pandas is a hard dependency.

@@ -220,7 +221,7 @@ def scalar_coerce(_, val):
@dispatch(ct.DateTime, _strtypes)
def scalar_coerce(_, val):
return dt_parse(val)
return pd.Timestamp(val)

This comment has been minimized.

@mrocklin

mrocklin May 20, 2015

Member

Does this work nicely with sqlalchemy?

This comment has been minimized.

@cpcloud

cpcloud May 20, 2015

Member

In what sense?

In [26]: import sqlalchemy as sa

In [27]: c = sa.Column('col', sa.types.TIMESTAMP)

In [28]: expr = c >= pd.Timestamp('now').to_pydatetime()

In [29]: print(expr.compile().render_literal_bindparam(expr.right))

raises NotImplementedError, and so does the version with the Timestamp instance.

This comment has been minimized.

@mrocklin

mrocklin May 20, 2015

Member

Hrm, that's unfortunate. I'm surprised that sqlalchemy doesn't manage python datetimes well.

This comment has been minimized.

@cpcloud

cpcloud May 20, 2015

Member

It handles them just fine, except when rendering literal values. You can use literal_column(dt.strftime('...')) or it's up to the dialect to manage literal rendering. You can of course pass datetime around in expressions and it deals with sending those values to the database quite nicely.

This comment has been minimized.

@cpcloud

cpcloud May 20, 2015

Member

As an example, I render literal datetime date and timedelta values in kdbpy in the way that kdbpy expects them

@cpcloud

This comment has been minimized.

Member

cpcloud commented May 21, 2015

I'll add that one thing that's nice about pandas Timestamps is that they can handle the precision of other systems' timestamps, such as kdb which also has nanosecond precision timestamp types.

@cpcloud

This comment has been minimized.

Member

cpcloud commented May 26, 2015

merging

cpcloud added a commit that referenced this pull request May 26, 2015

Merge pull request #1097 from cpcloud/ts-fix
Fix issue when converting datetimes and pandas Timestamps in an expression

@cpcloud cpcloud merged commit 5562599 into blaze:master May 26, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@cpcloud cpcloud deleted the cpcloud:ts-fix branch May 26, 2015

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