Skip to content

BUG: Regression in pd.read_sql_query between 1.0.5 and 1.1.0 #35871

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

Open
1 task
rbubley opened this issue Aug 24, 2020 · 7 comments
Open
1 task

BUG: Regression in pd.read_sql_query between 1.0.5 and 1.1.0 #35871

rbubley opened this issue Aug 24, 2020 · 7 comments
Labels
Bug IO SQL to_sql, read_sql, read_sql_query

Comments

@rbubley
Copy link

rbubley commented Aug 24, 2020

  • [ x] I have checked that this issue has not already been reported.

  • [ x] I have confirmed this bug exists on the latest version of pandas.

  • (optional) I have confirmed this bug exists on the master branch of pandas.


Note: Please read this guide detailing how to provide the necessary information for us to reproduce your bug.

Code Sample, a copy-pastable example

import pandas as pd
import sqlalchemy
from config import  credentials

engine = sqlalchemy.create_engine(
        "mysql+mysqldb://%(user)s:%(password)s@%(host)s/%(default)s" % credentials,
        echo=False,
    )

sql_query = r"""
SELECT 
  time_string,
  STR_TO_DATE(time_string, '%%d/%%m/%%Y'),
  data
FROM
  test_table
"""

data = pd.read_sql_query(sql_query, engine)

print(data)

Problem description

In Pandas 1.0.5, '%' in a sql query needed to be escaped as '%%'.
This changes in 1.1.0, but I couldn't see any relevant comment in the release notes. (The behaviour is the same in 1.1.1).

My feeling is that the new behaviour is actually better, but this still seems to be a regression (or an undocumented change).

Expected Output

Output of pd.show_versions()

INSTALLED VERSIONS

commit : d9fff27
python : 3.7.8.final.0
python-bits : 64
OS : Darwin
OS-release : 17.7.0
Version : Darwin Kernel Version 17.7.0: Wed Feb 27 00:43:23 PST 2019; root:xnu-4570.71.35~1/RELEASE_X86_64
machine : x86_64
processor : i386
byteorder : little
LC_ALL : None
LANG : en_GB.UTF-8
LOCALE : en_GB.UTF-8

pandas : 1.1.0
numpy : 1.18.5
pytz : 2018.9
dateutil : 2.8.0
pip : 20.2.2
setuptools : 40.8.0
Cython : None
pytest : None
hypothesis : None
sphinx : None
blosc : None
feather : None
xlsxwriter : 1.2.9
lxml.etree : None
html5lib : None
pymysql : None
psycopg2 : None
jinja2 : None
IPython : None
pandas_datareader: None
bs4 : None
bottleneck : None
fsspec : None
fastparquet : None
gcsfs : None
matplotlib : None
numexpr : None
odfpy : None
openpyxl : None
pandas_gbq : None
pyarrow : None
pytables : None
pyxlsb : None
s3fs : None
scipy : None
sqlalchemy : 1.3.17
tables : None
tabulate : None
xarray : None
xlrd : 1.2.0
xlwt : None
numba : None

@rbubley rbubley added Bug Needs Triage Issue that has not been reviewed by a pandas team member labels Aug 24, 2020
@Liam3851
Copy link
Contributor

This may be linked to the changes from #34211 and #34212. The what's new entry was this:

Bug in execute() was raising a ProgrammingError for some DB-API drivers when the SQL statement contained the % character and no parameters were present (GH34211)

Does the previously-working code with the escaped % break? #35484 may be related.

@rbubley
Copy link
Author

rbubley commented Aug 24, 2020

I agree, it does look likely that this stems from the changes following #34212.

The previously-working code does break - specifically, as the double % gets passed through to the database, the middle column of data is now filled with NULLs, rather than dates - i.e. no error is generated, but the returned data is now different.

@jbrockmendel jbrockmendel added the IO SQL to_sql, read_sql, read_sql_query label Sep 2, 2020
@jbrockmendel
Copy link
Member

cc @john-bodley thoughts on this?

@john-bodley
Copy link
Contributor

@jbrockmendel I'm not sure what Panda's perspective is on breaking changes with regards to minor releases. I'm sorry I wasn't more explicit in the whatsnew document and should have tested whether this was a breaking change. Is it viable update the document post release?

@TomAugspurger
Copy link
Contributor

We can certainly update the documentation, for future readers.

Do people have a sense for whether (partially?) reverting this change is going to cause more issues than just saying that the new behavior is the way it's going to be?

@TomAugspurger TomAugspurger removed the Needs Triage Issue that has not been reviewed by a pandas team member label Sep 4, 2020
@rbubley
Copy link
Author

rbubley commented Sep 4, 2020

As the reporting user (being a huge sample of 1!), I have already changed my usages to migrate from the old to the current syntax.

FWIW, the new syntax feels more natural to me.

@machow
Copy link

machow commented Sep 10, 2020

Hey! I added a test for this regression marked xfail in the above PR, which addresses a somewhat related issue. AFAIK it's not simple to support both % and %% representing the same thing in SQL queries without params. The PR has a table with the kinds of queries supported in it, v1.05, v1.1.

(RE @TomAugspurger 's question--not sure the impact on current code, but seems like v1.1's behavior is an overall useful change: guessing the bulk of users think in terms of plain SQL queries, without params)

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

No branches or pull requests

6 participants