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

[sql] Using read_sql_query instead of read_sql #4853

Merged
merged 1 commit into from
Apr 20, 2018

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Apr 20, 2018

The Pandas read_sql method either reads a SQL query or a database table (we require only the former) into a DataFrame. This results in an additional query (for non-SQLite databases) to determine whether the sql object represents a SQL query or table name. This is unnecessary as the sql object is always a SQL statement when called from get_df.

This PR uses the read_sql_query method instead which merely reads the SQL query into a DataFrame thus saving us executing an unnecessary statement.

Note for context about 15% of all our Presto queries originating from PyHive represented the now obsolete query.

@codecov-io
Copy link

codecov-io commented Apr 20, 2018

Codecov Report

Merging #4853 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4853   +/-   ##
=======================================
  Coverage   76.96%   76.96%           
=======================================
  Files          44       44           
  Lines        8534     8534           
=======================================
  Hits         6568     6568           
  Misses       1966     1966
Impacted Files Coverage Δ
superset/models/core.py 86.54% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e88b0b6...14990c0. Read the comment docs.

@mistercrunch
Copy link
Member

LGTM

@john-bodley john-bodley merged commit 8934717 into apache:master Apr 20, 2018
john-bodley added a commit to john-bodley/superset that referenced this pull request Apr 20, 2018
timifasubaa added a commit to airbnb/superset-fork that referenced this pull request Apr 23, 2018
[sql] Using read_sql_query instead of read_sql (apache#4853)
michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
timifasubaa pushed a commit to timifasubaa/incubator-superset that referenced this pull request May 31, 2018
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.25.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.25.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants