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

SQL: Status of the legacy mode? #6900

Closed
jorisvandenbossche opened this Issue Apr 17, 2014 · 21 comments

Comments

Projects
None yet
4 participants
@jorisvandenbossche
Member

jorisvandenbossche commented Apr 17, 2014

To discuss: do we regard the sql legacy mode (using a DBAPI2 connection, for sqlite and mysql) as a 'first-class citizen' in pandas? Or is it mainly for backwards compatibility?
Is it useful to have something that is not dependent on sqlalchemy? And consequently we alo try to enhance it /accept enhancements to it? Or is it really just 'legacy' and we don't develop it further (only bug fixes)?

I think it is important to discuss this (and 'formally' decide on this), as there are some issues with this:

  • If it is mainly for backwards compatibility, are we also planning to deprecate it? (which is not necessarily the consequence, if we just regard it as what the name says: legacy)

  • If it is not only for backwards compatibility, why do we raise a warning?:

    UserWarning: Not an SQLAlchemy engine,
    attempting to use as legacy DBAPI connection
    

    As this implies to me that it is not really preferred to use this.

  • Do we want to keep the functionality equal to the sqlalchemy version? (as we are using the same functions for both, it would make it complex if it is not)

    • Eg, we could easily provide a read_table functionality to legacy mode, by just doing a read_sql("SELECT * FROM specified_table", ..).
  • We should clarify the docs on this status: http://pandas-docs.github.io/pandas-docs-travis/io.html#legacy (as it is not really clear now)

  • If we decide we want to fully support this, we should maybe try to find another name than 'legacy' .. :-)

Maybe the bottom line we should ask ourselves is: do we want that we can really recommend it to users to use the legacy sqlite without sqlalchemy version? (and in that case I think it should be fully supported and not regarded as 'legacy')

@jorisvandenbossche

This comment has been minimized.

Show comment
Hide comment
@jorisvandenbossche

jorisvandenbossche Apr 17, 2014

Member

We already started the discussion here (#6881 (comment)), to quote @danielballan:

In the long term, I'd like to keep full-fledged support for connections generated by sqlite3. For light applications, I still favor those over sqlalchemy for code portability (e.g., to collaborators who might not have sqlaclemy). Since all other flavors already require dependencies that are not built in to Python, I think it is reasonable to expect users to also obtain sqlalchemy if they want the very best support. One man's opinion.

So, I'd like to build MultiIndex support for legacy connections.

Member

jorisvandenbossche commented Apr 17, 2014

We already started the discussion here (#6881 (comment)), to quote @danielballan:

In the long term, I'd like to keep full-fledged support for connections generated by sqlite3. For light applications, I still favor those over sqlalchemy for code portability (e.g., to collaborators who might not have sqlaclemy). Since all other flavors already require dependencies that are not built in to Python, I think it is reasonable to expect users to also obtain sqlalchemy if they want the very best support. One man's opinion.

So, I'd like to build MultiIndex support for legacy connections.

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Apr 21, 2014

Contributor

I think it might make sense to have full-fledged support for sqlite3 DBAPI only as it doesn't have any deps, while all other flavors have the python driver deps at the very least.

So support sqlite3 thru direct DBAPI and sqlalchemy, whilst other flavors ONLY with sqlalchemy. If at some point their is sufficient interest / PR's for supporting other direct DBAPI then could be added later.

Contributor

jreback commented Apr 21, 2014

I think it might make sense to have full-fledged support for sqlite3 DBAPI only as it doesn't have any deps, while all other flavors have the python driver deps at the very least.

So support sqlite3 thru direct DBAPI and sqlalchemy, whilst other flavors ONLY with sqlalchemy. If at some point their is sufficient interest / PR's for supporting other direct DBAPI then could be added later.

@jorisvandenbossche

This comment has been minimized.

Show comment
Hide comment
@jorisvandenbossche

jorisvandenbossche Apr 21, 2014

Member

@hayd @danielballan @mangecoeur @jtratner

And if we decide for a full-fledged support of sqlite3, this does also mean we deprecate the MySQL support via DBAPI connection (without sqlalchemy)?
Also, only supporting sqlite3 without sqlalchemy makes sense in the sense that it is the simplest flavor to support fully.

Member

jorisvandenbossche commented Apr 21, 2014

@hayd @danielballan @mangecoeur @jtratner

And if we decide for a full-fledged support of sqlite3, this does also mean we deprecate the MySQL support via DBAPI connection (without sqlalchemy)?
Also, only supporting sqlite3 without sqlalchemy makes sense in the sense that it is the simplest flavor to support fully.

@hayd

This comment has been minimized.

Show comment
Hide comment
@hayd

hayd Apr 21, 2014

Contributor

I kind of thing we should try and bring back the DBAPI support for this release (and potentially depreciate it, at least for mysql/postgres etc), it's going to shock people that their sql code is broken without warning/dep (which is what's happening atm??).

I've tried a few times to get the old tests working, but haven't been able to fix the mysql issues, so would be great if someone else could have a try!

Contributor

hayd commented Apr 21, 2014

I kind of thing we should try and bring back the DBAPI support for this release (and potentially depreciate it, at least for mysql/postgres etc), it's going to shock people that their sql code is broken without warning/dep (which is what's happening atm??).

I've tried a few times to get the old tests working, but haven't been able to fix the mysql issues, so would be great if someone else could have a try!

@jorisvandenbossche

This comment has been minimized.

Show comment
Hide comment
@jorisvandenbossche

jorisvandenbossche Apr 21, 2014

Member

@hayd There is nothing to 'bring back', as DBAPI is still supported via legacy mode. Normally all code that did run previously, should also run now (as only sqlite and mysql flavors were supported, postgresql was never supported). That is certainly something we have to guarantee. But of course if the tests say otherwise, we should look at that ..

Previously, only 'sqlite' and 'mysql' flavors were really supported, and those are now both supported via the legacy mode. Of course, some things previously also worked for other flavors, eg the read_sql to read a query is rather flavor independent and did also work for eg postgresql.
So the question is also if we continue to 'allow' this, or explicitly raise an error and point to sqlalchemy mode if a DPAPI connection is provided that is not sqlite (or mysql at this moment).

Member

jorisvandenbossche commented Apr 21, 2014

@hayd There is nothing to 'bring back', as DBAPI is still supported via legacy mode. Normally all code that did run previously, should also run now (as only sqlite and mysql flavors were supported, postgresql was never supported). That is certainly something we have to guarantee. But of course if the tests say otherwise, we should look at that ..

Previously, only 'sqlite' and 'mysql' flavors were really supported, and those are now both supported via the legacy mode. Of course, some things previously also worked for other flavors, eg the read_sql to read a query is rather flavor independent and did also work for eg postgresql.
So the question is also if we continue to 'allow' this, or explicitly raise an error and point to sqlalchemy mode if a DPAPI connection is provided that is not sqlite (or mysql at this moment).

@danielballan

This comment has been minimized.

Show comment
Hide comment
@danielballan

danielballan Apr 21, 2014

Contributor

Yes, let's make sure the legacy tests are in order but deprecate legacy connections in future releases.

This way, we will not waste effort trying to fix bugs in the legacy code -- the bugs that motivated us to adopt sqlalchemy to begin with -- but neither will we pull out the rug from people who are using the legacy connections now and finding them adequate.

Contributor

danielballan commented Apr 21, 2014

Yes, let's make sure the legacy tests are in order but deprecate legacy connections in future releases.

This way, we will not waste effort trying to fix bugs in the legacy code -- the bugs that motivated us to adopt sqlalchemy to begin with -- but neither will we pull out the rug from people who are using the legacy connections now and finding them adequate.

@jorisvandenbossche

This comment has been minimized.

Show comment
Hide comment
@jorisvandenbossche

jorisvandenbossche Apr 21, 2014

Member

@danielballan With 'deprecate legacy connections in future releases', do you also mean sqlite? Or only the others, because in the other thread you were in favor of a fully supported sqlite fallback?

I will take a look at the old tests, and try to get them running with the new code.

Member

jorisvandenbossche commented Apr 21, 2014

@danielballan With 'deprecate legacy connections in future releases', do you also mean sqlite? Or only the others, because in the other thread you were in favor of a fully supported sqlite fallback?

I will take a look at the old tests, and try to get them running with the new code.

@danielballan

This comment has been minimized.

Show comment
Hide comment
@danielballan

danielballan Apr 21, 2014

Contributor

Yes, that was ambiguous. I am still in favor of continued fully sqlite3 support. I mean that we should deprecate the others.

(Incidentally, supporting sqlite3 is a little less work because it only supports five datatypes.)

Contributor

danielballan commented Apr 21, 2014

Yes, that was ambiguous. I am still in favor of continued fully sqlite3 support. I mean that we should deprecate the others.

(Incidentally, supporting sqlite3 is a little less work because it only supports five datatypes.)

@jorisvandenbossche

This comment has been minimized.

Show comment
Hide comment
@jorisvandenbossche

jorisvandenbossche Apr 21, 2014

Member

yeah, that is what I meant with it being the 'simplest flavor to support fully.'. Actually just int, real and text (and null and blob). No bool, no datetime in different forms, ..

Member

jorisvandenbossche commented Apr 21, 2014

yeah, that is what I meant with it being the 'simplest flavor to support fully.'. Actually just int, real and text (and null and blob). No bool, no datetime in different forms, ..

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Apr 21, 2014

Contributor

so sqlalchemy prob writes datetimes as string and does the conversions? (for sqlite3)

Contributor

jreback commented Apr 21, 2014

so sqlalchemy prob writes datetimes as string and does the conversions? (for sqlite3)

@danielballan

This comment has been minimized.

Show comment
Hide comment
@danielballan

danielballan Apr 22, 2014

Contributor

@jreback Yes, that's right. As I explored that, I discovered that sqlalchemy cannot write timedeltas. It raises a StatementError. I'll open a separate issue.

Contributor

danielballan commented Apr 22, 2014

@jreback Yes, that's right. As I explored that, I discovered that sqlalchemy cannot write timedeltas. It raises a StatementError. I'll open a separate issue.

@jorisvandenbossche

This comment has been minimized.

Show comment
Hide comment
@jorisvandenbossche

jorisvandenbossche Apr 22, 2014

Member

And actually (I didn't realize before), sqlalchemy converts it to string, but keeps it as a 'datetime' column in the database (in sqlite, you can use declared data type as datetime as an alias, it's just not a datetime but a string), but in this way, also when reading that frame back in, sqlalchemy automatically converts it back to a datetime

Member

jorisvandenbossche commented Apr 22, 2014

And actually (I didn't realize before), sqlalchemy converts it to string, but keeps it as a 'datetime' column in the database (in sqlite, you can use declared data type as datetime as an alias, it's just not a datetime but a string), but in this way, also when reading that frame back in, sqlalchemy automatically converts it back to a datetime

@hayd

This comment has been minimized.

Show comment
Hide comment
@hayd

hayd Apr 22, 2014

Contributor

@jorisvandenbossche bring back the old test file (to ensure we're not breaking too much stuff), sorry.

Contributor

hayd commented Apr 22, 2014

@jorisvandenbossche bring back the old test file (to ensure we're not breaking too much stuff), sorry.

@danielballan

This comment has been minimized.

Show comment
Hide comment
@danielballan

danielballan Apr 22, 2014

Contributor

@hayd Is the latest copy of test_legacy_sql.py sitting the history of the master branch? I know I have an old copy in one of my SQL branches. Have you done recent work on it?

Contributor

danielballan commented Apr 22, 2014

@hayd Is the latest copy of test_legacy_sql.py sitting the history of the master branch? I know I have an old copy in one of my SQL branches. Have you done recent work on it?

@hayd

This comment has been minimized.

Show comment
Hide comment
@hayd

hayd Apr 22, 2014

Contributor

@danielballan I haven't done any recent work on it, although there was a couple of additional tests (e.g. not deleteing the database when in append mode) which could be added. I had tweaked it a little (e.g remove removed arguments), but mysql was choking/taking forever on the tests.

Contributor

hayd commented Apr 22, 2014

@danielballan I haven't done any recent work on it, although there was a couple of additional tests (e.g. not deleteing the database when in append mode) which could be added. I had tweaked it a little (e.g remove removed arguments), but mysql was choking/taking forever on the tests.

@jorisvandenbossche

This comment has been minimized.

Show comment
Hide comment
@jorisvandenbossche

jorisvandenbossche Apr 22, 2014

Member

@hayd @danielballan shouldn't we just look at the test_sql.py from the 0.13.1 tagged version? Or were there changes done to the tests after that?

Member

jorisvandenbossche commented Apr 22, 2014

@hayd @danielballan shouldn't we just look at the test_sql.py from the 0.13.1 tagged version? Or were there changes done to the tests after that?

@hayd

This comment has been minimized.

Show comment
Hide comment
@hayd

hayd Apr 23, 2014

Contributor

@jorisvandenbossche that's right, no changes were added so we should just use that version's tests (potentially could add on for table deleting when trying to append ?).

Contributor

hayd commented Apr 23, 2014

@jorisvandenbossche that's right, no changes were added so we should just use that version's tests (potentially could add on for table deleting when trying to append ?).

@jorisvandenbossche

This comment has been minimized.

Show comment
Hide comment
@jorisvandenbossche

jorisvandenbossche Apr 27, 2014

Member

I looked at the old tests, and most are running now (submitted PR #6987 to fix some issues).

What do the others think of deprecating the 'mysql' flavor for the legacy code with DBAPI connections? And only keep full support for sqlite without sqlalchemy?
Because if we would do that, we could better do it now I think.

Member

jorisvandenbossche commented Apr 27, 2014

I looked at the old tests, and most are running now (submitted PR #6987 to fix some issues).

What do the others think of deprecating the 'mysql' flavor for the legacy code with DBAPI connections? And only keep full support for sqlite without sqlalchemy?
Because if we would do that, we could better do it now I think.

@danielballan

This comment has been minimized.

Show comment
Hide comment
@danielballan

danielballan Apr 29, 2014

Contributor

@jorisvandenbossche Yes.

We want to get out of the business of supporting flavors' various idiosyncrasies. We've established that making an exception for sqlite3 makes sense. But if we support 'mysql' and not postgresql, oracle, etc. it will only be for historical reasons, and I worry that the code won't be well maintained.

Speaking as someone who will have a lot of his code broken by this change, I still think it's the right choice. If the deprecation warning incites a riot we can always reevaluate....

Contributor

danielballan commented Apr 29, 2014

@jorisvandenbossche Yes.

We want to get out of the business of supporting flavors' various idiosyncrasies. We've established that making an exception for sqlite3 makes sense. But if we support 'mysql' and not postgresql, oracle, etc. it will only be for historical reasons, and I worry that the code won't be well maintained.

Speaking as someone who will have a lot of his code broken by this change, I still think it's the right choice. If the deprecation warning incites a riot we can always reevaluate....

@jorisvandenbossche

This comment has been minimized.

Show comment
Hide comment
@jorisvandenbossche

jorisvandenbossche Apr 30, 2014

Member

So I will go forward:

  • deprecate 'mysql' flavor (and point to use sqlalchemy)
  • remove the UserWarning of not being a sqlalchemy engine

Somebody a proposal for another name than 'legacy'? Something like sqlite3 'fallback'?

@danielballan Normally your code should not be broken (yet), as the changes should be almost fully backwards compatible. And to adapt, I think it changing the connection an sqlalchemy engine will be enough in most cases as the interface stayed the same (but of course, this can also be a lot of work ..)

Member

jorisvandenbossche commented Apr 30, 2014

So I will go forward:

  • deprecate 'mysql' flavor (and point to use sqlalchemy)
  • remove the UserWarning of not being a sqlalchemy engine

Somebody a proposal for another name than 'legacy'? Something like sqlite3 'fallback'?

@danielballan Normally your code should not be broken (yet), as the changes should be almost fully backwards compatible. And to adapt, I think it changing the connection an sqlalchemy engine will be enough in most cases as the interface stayed the same (but of course, this can also be a lot of work ..)

@jorisvandenbossche

This comment has been minimized.

Show comment
Hide comment
@jorisvandenbossche

jorisvandenbossche Apr 30, 2014

Member

OK, then going forward with:

  • deprecate mysql flavor
  • remove the UserWarning of not using an sqlalchemy engine

Somebody a good idea how to name the 'legacy' mode, eg sqlite 'fallback'?

Normally, your code should not be broken (yet), as the changes are almost fully backwards compatible. And to adapt, the main thing to change is replacing the mysql connections with sqlalchemy engines as the interface should be the same for the rest (but of course, not saying that this + some of the corner cases could not be a lot of work ...)

Member

jorisvandenbossche commented Apr 30, 2014

OK, then going forward with:

  • deprecate mysql flavor
  • remove the UserWarning of not using an sqlalchemy engine

Somebody a good idea how to name the 'legacy' mode, eg sqlite 'fallback'?

Normally, your code should not be broken (yet), as the changes are almost fully backwards compatible. And to adapt, the main thing to change is replacing the mysql connections with sqlalchemy engines as the interface should be the same for the rest (but of course, not saying that this + some of the corner cases could not be a lot of work ...)

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