You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/08/12 03:50:11 UTC
[GitHub] [superset] dungdm93 opened a new pull request, #21066: Base presto and trino
dungdm93 opened a new pull request, #21066:
URL: https://github.com/apache/superset/pull/21066
### SUMMARY
#20152 Making the `TrinoEngineSpec` derive from the `PrestoEngineSpec` to reusing code. However, underlay driver of [Trino](https://pypi.org/project/trino/) is different from Presto - [PyHive](https://pypi.org/project/PyHive/) which lead to some issues.
This PR is rework on that PR by create `PrestoBaseEngineSpec` to share common functions between Presto variants.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
### TESTING INSTRUCTIONS
Unit tests
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #21066: fix(Trino): create `PrestoBaseEngineSpec` base class to share common code between Trino and Presto
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #21066:
URL: https://github.com/apache/superset/pull/21066#discussion_r953405378
##########
superset/db_engine_specs/presto.py:
##########
@@ -21,27 +21,24 @@
import time
from collections import defaultdict, deque
from contextlib import closing
-from datetime import datetime
from distutils.version import StrictVersion
from typing import Any, cast, Dict, List, Optional, Pattern, Tuple, TYPE_CHECKING, Union
-from urllib import parse
import pandas as pd
-import simplejson as json
from flask import current_app
from flask_babel import gettext as __, lazy_gettext as _
from sqlalchemy import Column, literal_column, types
from sqlalchemy.engine.base import Engine
from sqlalchemy.engine.reflection import Inspector
from sqlalchemy.engine.result import Row as ResultRow
-from sqlalchemy.engine.url import URL
from sqlalchemy.orm import Session
from sqlalchemy.sql.expression import ColumnClause, Select
from superset import cache_manager, is_feature_enabled
from superset.common.db_query_status import QueryStatus
from superset.databases.utils import make_url_safe
-from superset.db_engine_specs.base import BaseEngineSpec, ColumnTypeMapping
+from superset.db_engine_specs.base import ColumnTypeMapping
+from superset.db_engine_specs.presto_base import PrestoBaseEngineSpec
Review Comment:
I think we could just as well keep `PrestoBaseEngineSpec` in `superset.db_engine_specs.presto`, like we do for `PostgresBaseEngineSpec`, as it kinda keeps the list of modules in `db_engine_specs` shorter. Not strongly opinionated here, but let's at least keep it consistent; if we do decide to break out the bases, then I'd prefer to do it in a follow-up refactor PR and doing the same for other specs, too.
##########
superset/db_engine_specs/trino.py:
##########
@@ -89,32 +113,6 @@ def get_url_for_impersonation(
def get_allow_cost_estimate(cls, extra: Dict[str, Any]) -> bool:
return True
- @classmethod
- def get_table_names(
- cls,
- database: Database,
- inspector: Inspector,
- schema: Optional[str],
- ) -> List[str]:
- return BaseEngineSpec.get_table_names(
- database=database,
- inspector=inspector,
- schema=schema,
- )
-
- @classmethod
- def get_view_names(
- cls,
- database: Database,
- inspector: Inspector,
- schema: Optional[str],
- ) -> List[str]:
- return BaseEngineSpec.get_view_names(
- database=database,
- inspector=inspector,
- schema=schema,
- )
-
Review Comment:
We can also remove `has_implicit_cancel`, as it's no longer needed (it returns the same as `BaseEngineSpec` which isn't overridden in `PrestoBaseEngineSpec`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #21066: fix(Trino): create `PrestoBaseEngineSpec` base class to share common code between Trino and Presto
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #21066:
URL: https://github.com/apache/superset/pull/21066#issuecomment-1353537469
@john-bodley the problem is that PyHive and the new Trino connector are diverging at a rapid pace right now, so extending the Trino spec from the old Presto spec is IMO not a good idea (that actually created a few regressions of its own when we did that). I'm fine moving everything that's known to work on both to `PrestoBaseEngineSpec` (I can chip in with adding tests when I have some spare time).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] dungdm93 commented on a diff in pull request #21066: fix(Trino): create `PrestoBaseEngineSpec` base class to share common code between Trino and Presto
Posted by GitBox <gi...@apache.org>.
dungdm93 commented on code in PR #21066:
URL: https://github.com/apache/superset/pull/21066#discussion_r953679441
##########
superset/db_engine_specs/trino.py:
##########
@@ -89,32 +113,6 @@ def get_url_for_impersonation(
def get_allow_cost_estimate(cls, extra: Dict[str, Any]) -> bool:
return True
- @classmethod
- def get_table_names(
- cls,
- database: Database,
- inspector: Inspector,
- schema: Optional[str],
- ) -> List[str]:
- return BaseEngineSpec.get_table_names(
- database=database,
- inspector=inspector,
- schema=schema,
- )
-
- @classmethod
- def get_view_names(
- cls,
- database: Database,
- inspector: Inspector,
- schema: Optional[str],
- ) -> List[str]:
- return BaseEngineSpec.get_view_names(
- database=database,
- inspector=inspector,
- schema=schema,
- )
-
Review Comment:
`has_implicit_cancel` is removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro merged pull request #21066: fix(Trino): create `PrestoBaseEngineSpec` base class to share common code between Trino and Presto
Posted by GitBox <gi...@apache.org>.
villebro merged PR #21066:
URL: https://github.com/apache/superset/pull/21066
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #21066: fix(Trino): create `PrestoBaseEngineSpec` base class to share common code between Trino and Presto
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #21066:
URL: https://github.com/apache/superset/pull/21066#issuecomment-1212820532
# [Codecov](https://codecov.io/gh/apache/superset/pull/21066?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#21066](https://codecov.io/gh/apache/superset/pull/21066?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (38d6438) into [master](https://codecov.io/gh/apache/superset/commit/5113b01031705128df2064068a0809f07019c8ae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5113b01) will **decrease** coverage by `11.64%`.
> The diff coverage is `52.57%`.
```diff
@@ Coverage Diff @@
## master #21066 +/- ##
===========================================
- Coverage 66.28% 54.63% -11.65%
===========================================
Files 1770 1771 +1
Lines 67522 67537 +15
Branches 7177 7177
===========================================
- Hits 44754 36900 -7854
- Misses 20934 28803 +7869
Partials 1834 1834
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `53.05% <50.51%> (-0.02%)` | :arrow_down: |
| python | `57.43% <52.57%> (-24.06%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `50.71% <45.36%> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/21066?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/21066/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `34.76% <ø> (-52.11%)` | :arrow_down: |
| [superset/db\_engine\_specs/trino.py](https://codecov.io/gh/apache/superset/pull/21066/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3RyaW5vLnB5) | `38.46% <30.43%> (-36.31%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto\_base.py](https://codecov.io/gh/apache/superset/pull/21066/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0b19iYXNlLnB5) | `57.74% <57.74%> (ø)` | |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/21066/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `38.07% <100.00%> (-49.91%)` | :arrow_down: |
| [superset/utils/dashboard\_import\_export.py](https://codecov.io/gh/apache/superset/pull/21066/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvZGFzaGJvYXJkX2ltcG9ydF9leHBvcnQucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset/key\_value/commands/update.py](https://codecov.io/gh/apache/superset/pull/21066/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `0.00% <0.00%> (-88.89%)` | :arrow_down: |
| [superset/key\_value/commands/delete.py](https://codecov.io/gh/apache/superset/pull/21066/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZS5weQ==) | `0.00% <0.00%> (-85.30%)` | :arrow_down: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/21066/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.19%)` | :arrow_down: |
| [superset/key\_value/commands/delete\_expired.py](https://codecov.io/gh/apache/superset/pull/21066/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZV9leHBpcmVkLnB5) | `0.00% <0.00%> (-80.77%)` | :arrow_down: |
| [superset/dashboards/commands/importers/v0.py](https://codecov.io/gh/apache/superset/pull/21066/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy9pbXBvcnRlcnMvdjAucHk=) | `15.62% <0.00%> (-76.25%)` | :arrow_down: |
| ... and [283 more](https://codecov.io/gh/apache/superset/pull/21066/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on pull request #21066: fix(Trino): create `PrestoBaseEngineSpec` base class to share common code between Trino and Presto
Posted by GitBox <gi...@apache.org>.
john-bodley commented on PR #21066:
URL: https://github.com/apache/superset/pull/21066#issuecomment-1352858680
@dungdm93 and @villebro the issue I see with this approach is that it introduced several regressions, i.e., the `trino.latest_partition`, `trino.latest_sub_partition` macros no longer work as this functionality is part of the `PrestoEngineSpec` rather than the `PrestoBaseEngineSpec`. Are there plans to either:
1. Add this functionality to the `PrestoBaseEngineSpec`, or
2. Port this functionality to the `TrinoEngineSpec`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on pull request #21066: fix(Trino): create `PrestoBaseEngineSpec` base class to share common code between Trino and Presto
Posted by GitBox <gi...@apache.org>.
john-bodley commented on PR #21066:
URL: https://github.com/apache/superset/pull/21066#issuecomment-1353540898
@villebro understood. I'm 50/50 about whether option 1 or 2 is the preferred solution. Option 1 might be easiest for now (conditional on how coupled the logic is with the DB-API), but long term I sense option 2 is likely preferred.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] dungdm93 commented on a diff in pull request #21066: fix(Trino): create `PrestoBaseEngineSpec` base class to share common code between Trino and Presto
Posted by GitBox <gi...@apache.org>.
dungdm93 commented on code in PR #21066:
URL: https://github.com/apache/superset/pull/21066#discussion_r953687283
##########
superset/db_engine_specs/trino.py:
##########
@@ -17,36 +17,59 @@
from __future__ import annotations
import logging
-from typing import Any, Dict, List, Optional, TYPE_CHECKING
+from typing import Any, Dict, Optional, TYPE_CHECKING
import simplejson as json
from flask import current_app
-from sqlalchemy.engine.reflection import Inspector
from sqlalchemy.engine.url import URL
from sqlalchemy.orm import Session
+from superset.constants import USER_AGENT
from superset.databases.utils import make_url_safe
from superset.db_engine_specs.base import BaseEngineSpec
-from superset.db_engine_specs.presto import PrestoEngineSpec
+from superset.db_engine_specs.presto import PrestoBaseEngineSpec
from superset.models.sql_lab import Query
from superset.utils import core as utils
if TYPE_CHECKING:
from superset.models.core import Database
try:
- from trino.dbapi import Cursor # pylint: disable=unused-import
+ from trino.dbapi import Cursor
except ImportError:
pass
logger = logging.getLogger(__name__)
-class TrinoEngineSpec(PrestoEngineSpec):
+class TrinoEngineSpec(PrestoBaseEngineSpec):
engine = "trino"
- engine_aliases = {"trinonative"} # Required for backwards compatibility.
Review Comment:
`trinonative` in `setup.py` already removed since #20152
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] dungdm93 commented on pull request #21066: fix(Trino): create `PrestoBaseEngineSpec` base class to share common code between Trino and Presto
Posted by GitBox <gi...@apache.org>.
dungdm93 commented on PR #21066:
URL: https://github.com/apache/superset/pull/21066#issuecomment-1212710514
cc @villebro, @john-bodley
Could you guys help me listing issues, or features in Presto u like to have in Trino.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] dungdm93 commented on pull request #21066: Base presto and trino
Posted by GitBox <gi...@apache.org>.
dungdm93 commented on PR #21066:
URL: https://github.com/apache/superset/pull/21066#issuecomment-1212707273
cc @villebro, @john-bodley
Could you guys help me list out issues / missing features of Trino when its base class is changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org