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

API: Stop modifying SQL column and names, and warn when pertinent. #6902

Merged

Conversation

danielballan
Copy link
Contributor

Closes #6796

If any columns names written to_sql via a legacy connection would have been modified by previous versions of pandas, a warning is issued, both on table creation and row insertion. But no names are modified.

There is a stray warning message in nosetests. Otherwise good, I think. Does this reflect our consensus?

@jorisvandenbossche
Copy link
Member

Looking good, I think this does reflect consensus!

Maybe also add a test to check that a column name with a space is written correctly?

And what warning messages do you get in nose?

@@ -769,6 +765,11 @@ def _create_sql_schema(self, frame, table_name):
}


_SAFE_NAMES_WARNING = ("The spaces in these column names will not be changed."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A space after changed. is missing

@jreback jreback added the SQL label Apr 21, 2014
@jreback jreback added this to the 0.14.0 milestone Apr 21, 2014
@danielballan
Copy link
Contributor Author

Edited in response to @jorisvandenbossche's review and added a test for a bug in the original PR: DataFrame column names must be converted to strings.

@jorisvandenbossche
Copy link
Member

Ah, good, the integer column name error was still a to-fix on the #6292 list. Checked it as fixed!

Looking good for the rest. Ready to merge for you?

@danielballan
Copy link
Contributor Author

nosetests still spews a straw warning. Know a trick for finding out which test is to blame? I am pretty sure tm.assert_produces_warning suppresses the warning, so this is leaking out from some other test.

@danielballan
Copy link
Contributor Author

*straw -> stray

@jorisvandenbossche
Copy link
Member

which warning do you get?

If you just execute the file (tests_sql.py, there is a main that calls nose), then I see the progress of the tests in my terminal, and you can see for which test it is generated.

@jorisvandenbossche
Copy link
Member

BTW, there is still a warning that you should not care about for the moment (about not being a SQLAlchemy engine, at the moment you get that always when using the legacy, so just the first test using a DBAPI connection is triggering this).

@danielballan
Copy link
Contributor Author

Oh OK. I thought that warning was due to me. Then this is ready to merge.

jorisvandenbossche added a commit that referenced this pull request Apr 22, 2014
API: Stop modifying SQL column and names, and warn when pertinent.
@jorisvandenbossche jorisvandenbossche merged commit 6c36769 into pandas-dev:master Apr 22, 2014
@jorisvandenbossche
Copy link
Member

No it was not you, but I think we should do something about the warning (but that is a discussion for legacy-status issue).

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IO SQL to_sql, read_sql, read_sql_query
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG/API: converting invalid column names in to_sql
3 participants