[DB Engine] Support old and new Presto syntax #7977
Conversation
superset/db_engine_specs/presto.py
Outdated
| from superset.db_engine_specs.base import BaseEngineSpec | ||
| from superset.exceptions import SupersetTemplateException | ||
| from superset.models.sql_types.presto_sql_types import type_map as presto_type_map | ||
| from superset.utils import core as utils | ||
|
|
||
| QueryStatus = utils.QueryStatus | ||
|
|
||
| PRESTO_VERSION = app.config.get("PRESTO_VERSION") |
There was a problem hiding this comment.
@etr2460 I don't think this is correct as there may be multiple Presto databases. Either this should be defined in the Database CRUD model (preferred) or this variable needs to be a dictionary keyed by database ID.
There was a problem hiding this comment.
Eek, that's true... This should be added to the database model
There was a problem hiding this comment.
There may be a way of determining this from the connection, though regardless it probably should be stored either in the model or cached.
There was a problem hiding this comment.
yeah, i tried to figure out a way to from the connection, but couldn't find one. I've added it to the extra params in the database model now, as i didn't think it should use a new column (since we're only using it with presto dbs right now)
etr2460
force-pushed
the
erik-ritter--fix-presto-partition
branch
6 times, most recently
from
64d1daa
to
1722a3f
Compare
Codecov Report
@@ Coverage Diff @@
## master #7977 +/- ##
==========================================
+ Coverage 65.59% 65.59% +<.01%
==========================================
Files 469 469
Lines 22401 22403 +2
Branches 2432 2432
==========================================
+ Hits 14694 14696 +2
Misses 7586 7586
Partials 121 121
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Most SQL engines support querying the version: For example, on sqlite the query select sqlite_version(); for me yields the result 3.28.0. I would expect Presto to have something similar, but I was unable to find anything in the docs. If other similar use cases turn up, I think it would be worthwhile adding some type of mechanism for version checking in db_engine_specs, but for now I think this solution is ok. However, it would be good to add a note in docs/installation.rst that the version needs to be defined in the extra params for Presto to support the old syntax.
superset/db_engine_specs/presto.py
Outdated
| # Default to the new syntax if version is unset. | ||
| partition_select_clause = ( | ||
| f'SELECT * FROM "{table_name}$partitions"' | ||
| if not presto_version or presto_version >= "0.199" |
There was a problem hiding this comment.
I think you may want something like
version_tuple = lambda v: tuple(map(int, v.split('.')))
version_tuple(presto_version) > (0, 199)There was a problem hiding this comment.
I would suggest using distutils which provides a version comparison method as described here.
There was a problem hiding this comment.
I would suggest using distutils which provides a version comparison method as described here.
There was a problem hiding this comment.
Although string compare is probably going to work in almost all cases, I agree that using distutils is a better solution. Updated!
etr2460
force-pushed
the
erik-ritter--fix-presto-partition
branch
from
1722a3f
to
aee8af4
Compare
|
@villebro I've added additional documentation in |
(cherry picked from commit d58dbad)
mistercrunch
added
🏷️ bot
CATEGORY
Choose one
SUMMARY
In presto v0.199 the syntax to get partitions for a table changed. This logic was changed to only support the new syntax in #7250, but some people might still be on the old versions. This PR adds a config var to set your presto version (as i couldn't find any way to programmatically get it with sqlalchemy) and uses the correct syntax for your version of Presto.
TEST PLAN
CI
ADDITIONAL INFORMATION
REVIEWERS
@graceguo-supercat @betodealmeida @michellethomas @john-bodley