[sql] Correct SQL parameter formatting

[john-bodley]

TL;DR This PR removes the need for double escaping the percent character both in SQL Lab and explorer and ensures both approaches interface with the SQLA engine in a consistent manner.

This PR came about due to an error when exporting a SQL Lab result to CSV. For example in Presto one can successfully run the following query:

SELECT '%'

which executes directly via the SQLAlchemy engine, however an error was thrown when trying to export to CSV which uses the Pandas interface where the parameters argument is {} which causes an exception in PyHive which uses the pyformat SQL parameterstyle. In essence,

SELECT '%' %  {}

is invalid and thus one would need to escape the % character, i.e.,

SELECT '%%' %  {}

The reason for the error is the Pandas SQL uses the engine connection (which doesn't expose the execute arguments) whereas SQL Lab uses a raw connection (which exposes the execute arguments).

We should ensure that both SQL Lab and explore use the same interface, which means that either i) the compiled SQL statements include escaping of % to satisfy pyformat and enforce the parameter args to be either () or {}, or ii) the compiled SQL statements remove all escaping and enforce the parameter args to be None. Note in both cases one can copy-and-paste explorer queries directly in SQL Lab, though this PR implements the second option as i) one doesn't need to escape the % character for the appropriate dialects, and ii) the raw SQL is what one would run in the native DB CLI.

The fix is simply to bypass the Pandas API for querying and mimic the logic that SQL Lab uses which ensures that for presto parameters is None and thus it will not re-format the string.

Note this PR also fixes the Hive and MySQL engine specs inline with their API, i.e., both Hive and MySQL explicitly test whether the parameters and args respectively are None (rather than doing a boolean check).

This means for MySQL in SQL Lab one can run queries of the form,

SELECT * FROM table WHERE column LIKE '%foo%'

as opposed to:

SELECT * FROM table WHERE column LIKE '%%foo%%'

since previously the MySQL engine spec defined args to {} causing the string to be formatted with the empty dictionary args.

Closes #4098, #4857

to: @betodealmeida @michellethomas @mistercrunch
cc: @graceguo-supercat @timifasubaa

[mistercrunch]

Very nice. Happy to see this as read_sql has been getting in the way of getting consistency across SQL Lab & charts.

[codecov-io]

Codecov Report

Merging #5178 into master will increase coverage by 0.05%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master    #5178      +/-   ##
==========================================
+ Coverage   60.71%   60.77%   +0.05%     
==========================================
  Files         258      258              
  Lines       19701    19707       +6     
  Branches     1970     1970              
==========================================
+ Hits        11962    11977      +15     
+ Misses       7730     7721       -9     
  Partials        9        9

Impacted Files	Coverage Δ
superset/sql_lab.py	75.4% <100%> (ø)	⬆️
superset/connectors/sqla/models.py	78% <100%> (+0.7%)	⬆️
superset/db_engine_specs.py	53.85% <80%> (+0.97%)	⬆️
superset/models/core.py	86.71% <94.11%> (+0.14%)	⬆️

---

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 d483ed1...eeebcb0. Read the comment docs.

[john-bodley]

@mistercrunch my one concern is for Hive and whether running async=True (as defined by the engine spec) makes sense here.

[mistercrunch]

Hmm yeah looks like this won't work. The async=True is to get the progress bar. Some options:

• Make db_engine_spec.handle_cursor broader as in an execute method that returns the last recordset or dataframe. This means moving a fair amount of logic from SQL Lab and explore into db_engine_spec. It's probably the right thing to do.
• hacks around intercepting and disabling async when in explore/dashboard, but not in SQL Lab. That's more hacky but less disruptive

[john-bodley]

I originally was thinking of wrapping the execute method, though one still needs to decipher when to run in sync (explorer) vs. async (SQL Lab) mode.

[john-bodley]

@mistercrunch I address your comment regarding the async logic for SQL Lab vs. explorer. Would you mind taking a look?

[mistercrunch]

This LGTM. There's a bit of complexity/differences still left around the way explore vs sqllab get their data. I think the main difference is around Hive, the async param and whether handle_cursor is called or not (called only in SQL Lab).

This PR is a step in the right direction. Maybe eventually we want to make bring more into db_engine_spec, and have a get_records that receives update_process=False that internally would deal with async.

[john-bodley]

@mistercrunch do you want me to makes these changes here, or should these issues be addressed in a future PR?

[mistercrunch]

Side note, we may be able to drop 3.4 support soon. While 2.7 should have a long tail of support, I don't think it's as needed with 3.4

[john-bodley]

Agreed. This PR just adds the missing Python 3.6 version which I was using for testing locally. Note we've kind of already dropped 3.4 per here. I'll create a new PR to remove reference to it in Tox.

[john-bodley]

Note this PR is gated by #5206 as I didn't want to have to mutate multiple columns in the migration script.

[john-bodley]

@mistercrunch are you ok if I merge this PR?

[mistercrunch]

Yup @john-bodley go for it.