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/07/21 01:30:32 UTC
[GitHub] [superset] john-bodley commented on a diff in pull request #20799: feat(sql lab): display presto and trino tracking url
john-bodley commented on code in PR #20799:
URL: https://github.com/apache/superset/pull/20799#discussion_r926172621
##########
superset/db_engine_specs/presto.py:
##########
@@ -64,6 +64,13 @@
# prevent circular imports
from superset.models.core import Database
+ # need try/catch because pyhive may not be installed
+ try:
+ # pylint: disable=unused-import
+ from pyhive.presto import Cursor
Review Comment:
```suggestion
from pyhive.presto import Cursor # pylint: disable=unused-import
```
##########
superset/db_engine_specs/presto.py:
##########
@@ -957,8 +964,22 @@ def get_create_view(
return rows[0][0]
@classmethod
- def handle_cursor(cls, cursor: Any, query: Query, session: Session) -> None:
+ def get_tracking_url(cls, cursor: "Cursor") -> Optional[str]:
+ if cursor.last_query_id:
+ # pylint: disable=protected-access
+ return (
+ f"{cursor._protocol}://{cursor._host}:{cursor._port}"
Review Comment:
Why not just one line?
##########
superset/db_engine_specs/presto.py:
##########
@@ -957,8 +964,22 @@ def get_create_view(
return rows[0][0]
@classmethod
- def handle_cursor(cls, cursor: Any, query: Query, session: Session) -> None:
+ def get_tracking_url(cls, cursor: "Cursor") -> Optional[str]:
+ if cursor.last_query_id:
+ # pylint: disable=protected-access
+ return (
+ f"{cursor._protocol}://{cursor._host}:{cursor._port}"
+ + f"/ui/query.html?{cursor.last_query_id}"
+ )
+ return None
+
+ @classmethod
+ def handle_cursor(cls, cursor: "Cursor", query: Query, session: Session) -> None:
"""Updates progress information"""
+ tracking_url = cls.get_tracking_url(cursor)
+ if tracking_url:
+ query.tracking_url = tracking_url
Review Comment:
We may need a commit here.
##########
superset/models/sql_lab.py:
##########
@@ -279,6 +284,27 @@ def default_endpoint(self) -> str:
def get_extra_cache_keys(query_obj: Dict[str, Any]) -> List[str]:
return []
+ @property
+ def tracking_url(self) -> Optional[str]:
Review Comment:
Could this just be a method called `get_tracking_url` and then you wouldn't need to rename the column virtually or have a getter/setter.
##########
superset/db_engine_specs/presto.py:
##########
@@ -64,6 +64,13 @@
# prevent circular imports
from superset.models.core import Database
+ # need try/catch because pyhive may not be installed
+ try:
+ # pylint: disable=unused-import
Review Comment:
```suggestion
```
##########
superset/db_engine_specs/trino.py:
##########
@@ -109,8 +115,27 @@ def get_view_names(
)
@classmethod
- def handle_cursor(cls, cursor: Any, query: Query, session: Session) -> None:
+ def get_tracking_url(cls, cursor: "Cursor") -> Optional[str]:
+ try:
+ return cursor.info_uri
+ except AttributeError:
+ try:
+ conn = cursor.connection
+ # pylint: disable=protected-access
+ return (
+ f"{conn.http_scheme}://{conn.host}:{conn.port}/"
Review Comment:
See previous comment.
##########
superset/config.py:
##########
@@ -993,7 +993,7 @@ def CSV_TO_HIVE_UPLOAD_DIRECTORY_FUNC( # pylint: disable=invalid-name
# into a proxied one
-TRACKING_URL_TRANSFORMER = lambda x: x
+TRACKING_URL_TRANSFORMER = lambda url: url
Review Comment:
Nice!
##########
superset/db_engine_specs/presto.py:
##########
@@ -957,8 +964,22 @@ def get_create_view(
return rows[0][0]
@classmethod
- def handle_cursor(cls, cursor: Any, query: Query, session: Session) -> None:
+ def get_tracking_url(cls, cursor: "Cursor") -> Optional[str]:
+ if cursor.last_query_id:
+ # pylint: disable=protected-access
+ return (
+ f"{cursor._protocol}://{cursor._host}:{cursor._port}"
+ + f"/ui/query.html?{cursor.last_query_id}"
+ )
+ return None
+
+ @classmethod
+ def handle_cursor(cls, cursor: "Cursor", query: Query, session: Session) -> None:
"""Updates progress information"""
+ tracking_url = cls.get_tracking_url(cursor)
Review Comment:
I'm itching for us to drop Python 3.8 so we can use the walrus operator.
##########
superset/db_engine_specs/trino.py:
##########
@@ -32,6 +32,12 @@
if TYPE_CHECKING:
from superset.models.core import Database
+ try:
+ # pylint: disable=unused-import
+ from trino.dbapi import Cursor
Review Comment:
See previous comment.
##########
UPDATING.md:
##########
@@ -25,6 +25,7 @@ assists people when migrating to a new version.
## Next
- [20606](https://github.com/apache/superset/pull/20606): When user clicks on chart title or "Edit chart" button in Dashboard page, Explore opens in the same tab. Clicking while holding cmd/ctrl opens Explore in a new tab. To bring back the old behaviour (always opening Explore in a new tab), flip feature flag `DASHBOARD_EDIT_CHART_IN_NEW_TAB` to `True`.
+- [20799](https://github.com/apache/superset/pull/20799): Presto and Trino engine will now display tracking URL for running queries in SQL Lab. If for some reason you don't want to show the tracking URL (for example, when your data warehouse hasn't enable access for to Presto or Trino UI), update `TRACKING_URL_TRANSFORMER` in `config.py` to return an empty string or `None`.
Review Comment:
```suggestion
- [20799](https://github.com/apache/superset/pull/20799): Presto and Trino engine will now display tracking URL for running queries in SQL Lab. If for some reason you don't want to show the tracking URL (for example, when your data warehouse hasn't enable access for to Presto or Trino UI), update `TRACKING_URL_TRANSFORMER` in `config.py` to return `None`.
```
From both a code and communication perspective `None` seems clearer/cleaner.
##########
superset/sql_lab.py:
##########
@@ -96,8 +96,13 @@ def handle_query_error(
msg = f"{prefix_message} {str(ex)}".strip()
troubleshooting_link = config["TROUBLESHOOTING_LINK"]
query.error_message = msg
- query.status = QueryStatus.FAILED
query.tmp_table_name = None
+ query.status = QueryStatus.FAILED
+ # TODO: re-enable this after updating the frontend to properly display timeout status
+ # if query.status != QueryStatus.TIMED_OUT:
+ # query.status = QueryStatus.FAILED
Review Comment:
I'm a tad scared of this change. Why is the query no longer being marked as `FAILED`?
--
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