Skip to content
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

TimeDelta support #2

Closed
onyxfish opened this issue Sep 23, 2015 · 6 comments
Closed

TimeDelta support #2

onyxfish opened this issue Sep 23, 2015 · 6 comments

Comments

@onyxfish
Copy link
Collaborator

Possible?

@aklaver
Copy link
Contributor

aklaver commented Oct 31, 2015

http://docs.sqlalchemy.org/en/rel_1_0/core/type_basics.html#generic-types

SQLAlchemy has the generic SQL type INTERVAL that outputs Python datetime.timedelta. which is what the agate TimeDelta is looking for. INTERVAL is a native data type for Postgres and ORACLE. On other databases INTERVAL it is stored as a date relative to the Unix epoch 1/1/1970. From the above I understand this to mean that the INTERVAL type will only show up in metadata from tables that use the interval type as in Postgres and Oracle. For other databases a date type will show up. In either case the type is covered. The only issue I can see is if a user is using SQLAlchemy as the ORM to create and use their tables outside agate and expect to see an INTERVAL type for a field that they pull against a database that does not support it natively i.e MySQL. They will still get the field it will just be a DATE type. The same goes for pushing data back into a table. The first solution that comes to mind is to create some way for the user to map fields they are using as INTERVAL, so agate-sql can use that SQLAlchemy type instead of DATE. This would not be needed in the Postgres and Oracle cases.

@onyxfish
Copy link
Collaborator Author

Hmmm, I think the "version 1" solution for this is documentation. Let's implement it the naive (database-agnostic) way and then add a note to the docs about db-specific support for TimeDelta. If anybody ever complains we can consider a more robust solution.

@aklaver
Copy link
Contributor

aklaver commented Nov 1, 2015

I agree with the above. I started working on this, but seemed to have hit a snag on the SQLAlchemy side in regards to naming of Interval(INTERVAL). I filed a issue when them:

https://bitbucket.org/zzzeek/sqlalchemy/issues/3571/interval-vs-interval-and-python_type

One option is to special case this, but I would prefer to wait until the above issue is dealt with one way or another.

@onyxfish
Copy link
Collaborator Author

onyxfish commented Nov 2, 2015

Ugh, just read through that issue. Fortunately I think the variability he describes in how the adapters return types is probably moot four purposes: all numeric types collapse to agate.Number() and all text types collapse to agate.Text().

Based on my reading of your back-and-forth it sounds like the best part forward for INTERVAL is just to hold off until now? Or is there a straightforward way to special case now you know the broader shape of the problem?

@aklaver
Copy link
Contributor

aklaver commented Nov 2, 2015

Longer term I am going to see about getting python_type set for INTERVAL, by submitting a PR to SQLAlchemy. Short term I am thinking a special case. The generic Interval already has python_type set, it is just a matter of intercepting INTERVAL and faking a python_type for it.

@onyxfish
Copy link
Collaborator Author

onyxfish commented Nov 2, 2015

Sounds good to me! 👍

@onyxfish onyxfish closed this as completed Nov 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants