Bug: fixing async syntax for python 3.7

[xtinec]

Rename async to async_ so superset installs for python 3.7. 👀 @betodealmeida

[codecov-io]

Codecov Report

Merging #5759 into master will increase coverage by 0.02%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master    #5759      +/-   ##
==========================================
+ Coverage   63.34%   63.36%   +0.02%     
==========================================
  Files         364      364              
  Lines       23051    23051              
  Branches     2565     2565              
==========================================
+ Hits        14602    14607       +5     
+ Misses       8434     8429       -5     
  Partials       15       15

Impacted Files	Coverage Δ
superset/sql_lab.py	71.77% <100%> (ø)	⬆️
superset/db_engine_specs.py	54.01% <33.33%> (-0.15%)	⬇️
superset/views/core.py	74.29% <0%> (+0.44%)	⬆️

---

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 60ecd72...0dabf00. Read the comment docs.

[betodealmeida]

This turned out to be more complicated than I expected! I'll make a quick PR to Pyhive to fix their side.

[betodealmeida]

Here we're POSTing the data payload into the sql_json view, so this should actually be runAsync=async_. See: https://github.com/apache/incubator-superset/blob/60ecd72aac85b8faddeeb339f24299224ecbca91/superset/views/core.py#L2375

[betodealmeida]

Here the async_ keyword argument is not being used anywhere. The reason why it's here is because some engines (actually, only HiveEngineSpec) have the async option and override this method. SQL Lab doesn't differentiate between engines, and regardless of the backend will call:

db_engine_spec.execute(cursor, query.executed_sql, async_=True)

It's better to change the method signature here to:

def execute(cls, cursor, query, **kwargs):

This way, any extra keyword arguments are stored in kwargs, and derived classes can declare more specific signatures, eg, for HiveEngineSpec:

def execute(cls, cursor, query, async_=False, **kwargs):

[betodealmeida]

Change this to:

def execute(cursor, query, async_=False, **kwargs):

(See my comment above.)

[xtinec]

Should this one be def execute(cursor, query, **kwargs):? Then downstream systems like PyHive can specify async_?

[betodealmeida]

If we had def execute(cursor, query, **kwargs): here then the method would have to be like this:

def execute(cursor, query, **kwargs):
    kwargs['async'] = kwargs.pop('async_', False)
    cursor.execute(query, **kwargs) 

Since SQL Lab is passing async_=True, but PyHive expects an async named argument. We're basically mapping the keyword argument from async_ to async, as it's passed from SQL Lab to PyHive.

I prefer having it explicitly named in the function signature instead:

def execute(cursor, query, async_=False, **kwargs):
    kwargs = {'async': async_}
    cursor.execute(query, **kwargs)

This way if you're looking at this line in sql_lab.py:

https://github.com/apache/incubator-superset/blob/be04c98cd3a55aec9c9dd6d1457de5655ad20b30/superset/sql_lab.py#L175

You can grep and find which function takes the async (async_) argument.

[betodealmeida]

Here, cursor is a pyhive cursor, and unfortunately it expects a keyword argument called async: https://github.com/dropbox/PyHive/blob/65076bbc8697a423b438dc03e928a6dff86fd2cb/pyhive/hive.py#L337

There's an open issue to address this: dropbox/PyHive#148 Unfortunately, until they fix it, we won't be able to run Hive queries in Superset under Python 3.7. :(

In the meantime, I would recommend changing this to:

kwargs = {'async': async_}
cursor.execute(query, **kwargs)

This will allow Superset to work with Python 3.7 with other engines, and with Hive when using Python < 3.7.

[betodealmeida]

For reference: dropbox/PyHive#232

[betodealmeida]

This should actually be just:

cursor.execute(query)

Otherwise when SQL Lab calls:

db_engine_spec.execute(cursor, query.executed_sql, async_=True)

The async_ kwarg will be passed down to all the cursors, and this could potentially cause an exception if their method signatures don't have **kwargs.

[betodealmeida]

This won't work as is, since your PR changes SQL Lab to pass the async_ keyword to the execute method, and PyHive is (still) expecting the async keyword. You need to map the async_ that you receive from SQL Lab into async for PyHive, see two approaches in this comment: #5759

(Sorry if this is confusing, this is a bit of Python black magic...)