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/12/16 14:48:26 UTC
[GitHub] [superset] wanghong1314 opened a new pull request, #22441: fix: stop query in SQL Lab with impala engine (#20950)
wanghong1314 opened a new pull request, #22441:
URL: https://github.com/apache/superset/pull/22441
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
fix(sqllab): Stop button for queries doesn't work in SQL Lab when using SQL Lab with impala engine and adding Progress Information
Fix the bug:https://github.com/apache/superset/issues/20950
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
BEFORE
AFTER
progress info
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### 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] wanghong1314 closed pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
wanghong1314 closed pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
URL: https://github.com/apache/superset/pull/22441
--
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] wanghong1314 commented on a diff in pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
wanghong1314 commented on code in PR #22441:
URL: https://github.com/apache/superset/pull/22441#discussion_r1051297797
##########
superset/db_engine_specs/impala.py:
##########
@@ -63,3 +74,117 @@ def get_schema_names(cls, inspector: Inspector) -> List[str]:
if not row[0].startswith("_")
]
return schemas
+
+ @classmethod
+ def has_implicit_cancel(cls) -> bool:
+ """
+ Return True if the live cursor handles the implicit cancelation of the query,
+ False otherise.
+
+ :return: Whether the live cursor implicitly cancels the query
+ :see: handle_cursor
+ """
+
+ return True
+
+ @classmethod
+ def execute( # pylint: disable=unused-argument
+ cls,
+ cursor: Any,
+ query: str,
+ **kwargs: Any,
+ ) -> None: # pylint: disable=arguments-differ
+ # kwargs = {"async": async_}
+ try:
+ cursor.execute_async(query)
+ except Exception as ex:
+ raise cls.get_dbapi_mapped_exception(ex)
+
+ @classmethod
+ def handle_cursor(cls, cursor: Any, query: Query, session: Session) -> None:
+ """Stop query and updates progress information"""
+
+ query_id = query.id
+ unfinished_states = (
+ "INITIALIZED_STATE",
+ "RUNNING_STATE",
+ )
+ # When the query status is pending, the stop operation is queried
+ # Refresh session so that the `query.status` and `query.extra.get(is_stopped)`
+ # modified in stop_query in views / core.py is reflected here.
+ # stop query
+ if cls.is_cancel_query(cls, query, session, query_id):
Review Comment:
this is my custom method, not the cancel_query method, because this method is used repeatedly
--
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] eschutho commented on pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
eschutho commented on PR #22441:
URL: https://github.com/apache/superset/pull/22441#issuecomment-1370308953
> @eschutho @bolkedebruin @eschutho I saw that the ci test passed, please help review the code, thank you
Thanks @wanghong1314. I'm going to defer to @betodealmeida or @villebro on whether the hive pattern is still the best option here.
--
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] eschutho commented on a diff in pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22441:
URL: https://github.com/apache/superset/pull/22441#discussion_r1051148662
##########
docker/pythonpath_dev/superset_config.py:
##########
@@ -106,6 +106,7 @@ class CeleryConfig(object):
WEBDRIVER_BASEURL = "http://superset:8088/"
# The base URL for the email report hyperlinks.
WEBDRIVER_BASEURL_USER_FRIENDLY = WEBDRIVER_BASEURL
+IMPALA_POLL_INTERVAL = int(timedelta(seconds=5).total_seconds())
Review Comment:
I don't think you need this if the value is the same as the config file.
--
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] eschutho commented on a diff in pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22441:
URL: https://github.com/apache/superset/pull/22441#discussion_r1053919320
##########
docker/pythonpath_dev/superset_config.py:
##########
@@ -106,6 +106,7 @@ class CeleryConfig(object):
WEBDRIVER_BASEURL = "http://superset:8088/"
# The base URL for the email report hyperlinks.
WEBDRIVER_BASEURL_USER_FRIENDLY = WEBDRIVER_BASEURL
+IMPALA_POLL_INTERVAL = int(timedelta(seconds=5).total_seconds())
Review Comment:
Great 👍
--
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] eschutho commented on a diff in pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22441:
URL: https://github.com/apache/superset/pull/22441#discussion_r1054948087
##########
superset/db_engine_specs/impala.py:
##########
@@ -63,3 +74,117 @@ def get_schema_names(cls, inspector: Inspector) -> List[str]:
if not row[0].startswith("_")
]
return schemas
+
+ @classmethod
+ def has_implicit_cancel(cls) -> bool:
+ """
+ Return True if the live cursor handles the implicit cancelation of the query,
+ False otherise.
+
+ :return: Whether the live cursor implicitly cancels the query
+ :see: handle_cursor
+ """
+
+ return True
+
+ @classmethod
+ def execute( # pylint: disable=unused-argument
+ cls,
+ cursor: Any,
+ query: str,
+ **kwargs: Any,
+ ) -> None: # pylint: disable=arguments-differ
+ # kwargs = {"async": async_}
+ try:
+ cursor.execute_async(query)
+ except Exception as ex:
+ raise cls.get_dbapi_mapped_exception(ex)
+
+ @classmethod
+ def handle_cursor(cls, cursor: Any, query: Query, session: Session) -> None:
+ """Stop query and updates progress information"""
+
+ query_id = query.id
+ unfinished_states = (
+ "INITIALIZED_STATE",
+ "RUNNING_STATE",
+ )
+ # When the query status is pending, the stop operation is queried
+ # Refresh session so that the `query.status` and `query.extra.get(is_stopped)`
+ # modified in stop_query in views / core.py is reflected here.
+ # stop query
+ if cls.is_cancel_query(cls, query, session, query_id):
Review Comment:
also cc @villebro
--
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] eschutho commented on pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
eschutho commented on PR #22441:
URL: https://github.com/apache/superset/pull/22441#issuecomment-1355609189
@wanghong1314 there are also some failing CI checks. Let us know if you need any help resolving them. cc @betodealmeida for another set of eyes on this review.
--
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] wanghong1314 commented on a diff in pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
wanghong1314 commented on code in PR #22441:
URL: https://github.com/apache/superset/pull/22441#discussion_r1051297231
##########
superset/db_engine_specs/impala.py:
##########
@@ -63,3 +74,117 @@ def get_schema_names(cls, inspector: Inspector) -> List[str]:
if not row[0].startswith("_")
]
return schemas
+
+ @classmethod
+ def has_implicit_cancel(cls) -> bool:
+ """
+ Return True if the live cursor handles the implicit cancelation of the query,
+ False otherise.
+
+ :return: Whether the live cursor implicitly cancels the query
+ :see: handle_cursor
+ """
+
+ return True
+
+ @classmethod
+ def execute( # pylint: disable=unused-argument
+ cls,
+ cursor: Any,
+ query: str,
+ **kwargs: Any,
+ ) -> None: # pylint: disable=arguments-differ
+ # kwargs = {"async": async_}
+ try:
+ cursor.execute_async(query)
+ except Exception as ex:
+ raise cls.get_dbapi_mapped_exception(ex)
+
+ @classmethod
+ def handle_cursor(cls, cursor: Any, query: Query, session: Session) -> None:
+ """Stop query and updates progress information"""
+
+ query_id = query.id
+ unfinished_states = (
+ "INITIALIZED_STATE",
+ "RUNNING_STATE",
+ )
+ # When the query status is pending, the stop operation is queried
+ # Refresh session so that the `query.status` and `query.extra.get(is_stopped)`
+ # modified in stop_query in views / core.py is reflected here.
+ # stop query
+ if cls.is_cancel_query(cls, query, session, query_id):
Review Comment:
the impala query cancellation needs to be obtained in the cursor, similar to the hive engine query cancellation
--
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] eschutho commented on a diff in pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22441:
URL: https://github.com/apache/superset/pull/22441#discussion_r1051159888
##########
superset/db_engine_specs/impala.py:
##########
@@ -63,3 +74,117 @@ def get_schema_names(cls, inspector: Inspector) -> List[str]:
if not row[0].startswith("_")
]
return schemas
+
+ @classmethod
+ def has_implicit_cancel(cls) -> bool:
+ """
+ Return True if the live cursor handles the implicit cancelation of the query,
+ False otherise.
+
+ :return: Whether the live cursor implicitly cancels the query
+ :see: handle_cursor
+ """
+
+ return True
+
+ @classmethod
+ def execute( # pylint: disable=unused-argument
+ cls,
+ cursor: Any,
+ query: str,
+ **kwargs: Any,
+ ) -> None: # pylint: disable=arguments-differ
+ # kwargs = {"async": async_}
+ try:
+ cursor.execute_async(query)
+ except Exception as ex:
+ raise cls.get_dbapi_mapped_exception(ex)
+
+ @classmethod
+ def handle_cursor(cls, cursor: Any, query: Query, session: Session) -> None:
+ """Stop query and updates progress information"""
+
+ query_id = query.id
+ unfinished_states = (
+ "INITIALIZED_STATE",
+ "RUNNING_STATE",
+ )
+ # When the query status is pending, the stop operation is queried
+ # Refresh session so that the `query.status` and `query.extra.get(is_stopped)`
+ # modified in stop_query in views / core.py is reflected here.
+ # stop query
+ if cls.is_cancel_query(cls, query, session, query_id):
Review Comment:
I'm not too familiar with impala, to be honest, but for the other databases, we usually handle the cancel query functionality when the `cancel_query` method is called. It looks like it would be more efficient to move this logic into `cancel_query` in this db engine spec so that it is only run when we know that a cancel query has been requested, instead of checking on each cursor operation.
--
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] eschutho commented on a diff in pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22441:
URL: https://github.com/apache/superset/pull/22441#discussion_r1051145536
##########
superset-frontend/src/SqlLab/actions/sqlLab.js:
##########
@@ -229,6 +229,9 @@ export function startQuery(query) {
export function querySuccess(query, results) {
return function (dispatch) {
+ if (!results.query) {
Review Comment:
is this line here to prevent an error on line 239? If so, I think it's still useful to store the query in line 245. I'll add a separate suggestion below if that's what your intention is.
--
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] wanghong1314 commented on a diff in pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
wanghong1314 commented on code in PR #22441:
URL: https://github.com/apache/superset/pull/22441#discussion_r1051296672
##########
superset/db_engine_specs/impala.py:
##########
@@ -63,3 +74,117 @@ def get_schema_names(cls, inspector: Inspector) -> List[str]:
if not row[0].startswith("_")
]
return schemas
+
+ @classmethod
+ def has_implicit_cancel(cls) -> bool:
+ """
+ Return True if the live cursor handles the implicit cancelation of the query,
+ False otherise.
+
+ :return: Whether the live cursor implicitly cancels the query
+ :see: handle_cursor
+ """
+
+ return True
+
+ @classmethod
+ def execute( # pylint: disable=unused-argument
+ cls,
+ cursor: Any,
+ query: str,
+ **kwargs: Any,
+ ) -> None: # pylint: disable=arguments-differ
+ # kwargs = {"async": async_}
+ try:
+ cursor.execute_async(query)
Review Comment:
1. since the impala engine does not support async with execute, it is possible that my comments are ambiguous
2. because the impala engine is asynchronous, the query status can be obtained
<img width="703" alt="image" src="https://user-images.githubusercontent.com/10183241/208220292-f0717c3c-9e03-470e-90e0-3cdea9dbe1c1.png">
--
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] wanghong1314 commented on a diff in pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
wanghong1314 commented on code in PR #22441:
URL: https://github.com/apache/superset/pull/22441#discussion_r1062417627
##########
superset-frontend/src/SqlLab/actions/sqlLab.js:
##########
@@ -235,7 +235,7 @@ export function querySuccess(query, results) {
!query.isDataPreview &&
isFeatureEnabled(FeatureFlag.SQLLAB_BACKEND_PERSISTENCE)
? SupersetClient.put({
- endpoint: encodeURI(`/tabstateview/${sqlEditorId}`),
+ endpoint: encodeURI(`/tabstateview/${results?.query?.sqlEditorId}`),
Review Comment:
OK, I'm going to roll back the code to solve this problem
--
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] wanghong1314 commented on a diff in pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
wanghong1314 commented on code in PR #22441:
URL: https://github.com/apache/superset/pull/22441#discussion_r1053961186
##########
superset/db_engine_specs/impala.py:
##########
@@ -63,3 +74,117 @@ def get_schema_names(cls, inspector: Inspector) -> List[str]:
if not row[0].startswith("_")
]
return schemas
+
+ @classmethod
+ def has_implicit_cancel(cls) -> bool:
+ """
+ Return True if the live cursor handles the implicit cancelation of the query,
+ False otherise.
+
+ :return: Whether the live cursor implicitly cancels the query
+ :see: handle_cursor
+ """
+
+ return True
+
+ @classmethod
+ def execute( # pylint: disable=unused-argument
+ cls,
+ cursor: Any,
+ query: str,
+ **kwargs: Any,
+ ) -> None: # pylint: disable=arguments-differ
+ # kwargs = {"async": async_}
+ try:
+ cursor.execute_async(query)
Review Comment:
Because you can't get the status of the query synchronously, you can't really stop the query, and the code is executed asynchronously by default 
--
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] eschutho commented on a diff in pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22441:
URL: https://github.com/apache/superset/pull/22441#discussion_r1051156044
##########
superset/db_engine_specs/impala.py:
##########
@@ -63,3 +74,117 @@ def get_schema_names(cls, inspector: Inspector) -> List[str]:
if not row[0].startswith("_")
]
return schemas
+
+ @classmethod
+ def has_implicit_cancel(cls) -> bool:
+ """
+ Return True if the live cursor handles the implicit cancelation of the query,
+ False otherise.
+
+ :return: Whether the live cursor implicitly cancels the query
+ :see: handle_cursor
+ """
+
+ return True
+
+ @classmethod
+ def execute( # pylint: disable=unused-argument
+ cls,
+ cursor: Any,
+ query: str,
+ **kwargs: Any,
+ ) -> None: # pylint: disable=arguments-differ
+ # kwargs = {"async": async_}
+ try:
+ cursor.execute_async(query)
Review Comment:
is the intention here to run `execute_async` if `_async=True` is passed in? If so, it looks like we should still keep the option to run this query synchronously.
--
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] wanghong1314 commented on a diff in pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
wanghong1314 commented on code in PR #22441:
URL: https://github.com/apache/superset/pull/22441#discussion_r1051329956
##########
docker/pythonpath_dev/superset_config.py:
##########
@@ -106,6 +106,7 @@ class CeleryConfig(object):
WEBDRIVER_BASEURL = "http://superset:8088/"
# The base URL for the email report hyperlinks.
WEBDRIVER_BASEURL_USER_FRIENDLY = WEBDRIVER_BASEURL
+IMPALA_POLL_INTERVAL = int(timedelta(seconds=5).total_seconds())
Review Comment:
I have removed the configuration property，thank you
--
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] wanghong1314 commented on a diff in pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
wanghong1314 commented on code in PR #22441:
URL: https://github.com/apache/superset/pull/22441#discussion_r1062424186
##########
superset/config.py:
##########
@@ -1107,6 +1107,8 @@ def CSV_TO_HIVE_UPLOAD_DIRECTORY_FUNC( # pylint: disable=invalid-name
# Interval between consecutive polls when using Hive Engine
HIVE_POLL_INTERVAL = int(timedelta(seconds=5).total_seconds())
+# Interval between consecutive polls when using Impala Engine
+IMPALA_POLL_INTERVAL = int(timedelta(seconds=5).total_seconds())
Review Comment:
This is a good idea. I will try it locally and then submit the code
--
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] wanghong1314 commented on pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
wanghong1314 commented on PR #22441:
URL: https://github.com/apache/superset/pull/22441#issuecomment-1355074021
@rusackas @eschutho @yousoph Have some time to help check whether it fits the solution
--
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] eschutho commented on a diff in pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22441:
URL: https://github.com/apache/superset/pull/22441#discussion_r1051145536
##########
superset-frontend/src/SqlLab/actions/sqlLab.js:
##########
@@ -229,6 +229,9 @@ export function startQuery(query) {
export function querySuccess(query, results) {
return function (dispatch) {
+ if (!results.query) {
Review Comment:
is this line here to prevent an error on line 239? If so, I think it's still useful to store the query in line 245 and just use optional chaining in case results.query doesn't exist. `results?.query?.sqlEditorId`
--
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] wanghong1314 commented on pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
wanghong1314 commented on PR #22441:
URL: https://github.com/apache/superset/pull/22441#issuecomment-1355986209
> @wanghong1314 there are also some failing CI checks. Let us know if you need any help resolving them. cc @betodealmeida for another set of eyes on this review.
@baldoalessandro please check that I have not changed the code of mysql and postgress related engine. Why does ci report an error?
--
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] eschutho commented on a diff in pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22441:
URL: https://github.com/apache/superset/pull/22441#discussion_r1054947861
##########
superset/db_engine_specs/impala.py:
##########
@@ -63,3 +74,117 @@ def get_schema_names(cls, inspector: Inspector) -> List[str]:
if not row[0].startswith("_")
]
return schemas
+
+ @classmethod
+ def has_implicit_cancel(cls) -> bool:
+ """
+ Return True if the live cursor handles the implicit cancelation of the query,
+ False otherise.
+
+ :return: Whether the live cursor implicitly cancels the query
+ :see: handle_cursor
+ """
+
+ return True
+
+ @classmethod
+ def execute( # pylint: disable=unused-argument
+ cls,
+ cursor: Any,
+ query: str,
+ **kwargs: Any,
+ ) -> None: # pylint: disable=arguments-differ
+ # kwargs = {"async": async_}
+ try:
+ cursor.execute_async(query)
+ except Exception as ex:
+ raise cls.get_dbapi_mapped_exception(ex)
+
+ @classmethod
+ def handle_cursor(cls, cursor: Any, query: Query, session: Session) -> None:
+ """Stop query and updates progress information"""
+
+ query_id = query.id
+ unfinished_states = (
+ "INITIALIZED_STATE",
+ "RUNNING_STATE",
+ )
+ # When the query status is pending, the stop operation is queried
+ # Refresh session so that the `query.status` and `query.extra.get(is_stopped)`
+ # modified in stop_query in views / core.py is reflected here.
+ # stop query
+ if cls.is_cancel_query(cls, query, session, query_id):
Review Comment:
@john-bodley or @betodealmeida this is the way that we cancel queries on hive, but I'm wondering if it would be more efficient to use the cancel_query method instead, providing that you can get the cursor. WDYT?
--
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] eschutho commented on a diff in pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #22441:
URL: https://github.com/apache/superset/pull/22441#discussion_r1053921304
##########
superset/db_engine_specs/impala.py:
##########
@@ -63,3 +74,117 @@ def get_schema_names(cls, inspector: Inspector) -> List[str]:
if not row[0].startswith("_")
]
return schemas
+
+ @classmethod
+ def has_implicit_cancel(cls) -> bool:
+ """
+ Return True if the live cursor handles the implicit cancelation of the query,
+ False otherise.
+
+ :return: Whether the live cursor implicitly cancels the query
+ :see: handle_cursor
+ """
+
+ return True
+
+ @classmethod
+ def execute( # pylint: disable=unused-argument
+ cls,
+ cursor: Any,
+ query: str,
+ **kwargs: Any,
+ ) -> None: # pylint: disable=arguments-differ
+ # kwargs = {"async": async_}
+ try:
+ cursor.execute_async(query)
Review Comment:
Can you run execute if synchronous and execute_async only if _async is True? That way we can still run synchronously as well.
--
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 #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #22441:
URL: https://github.com/apache/superset/pull/22441#discussion_r1062194520
##########
superset-frontend/src/SqlLab/actions/sqlLab.js:
##########
@@ -235,7 +235,7 @@ export function querySuccess(query, results) {
!query.isDataPreview &&
isFeatureEnabled(FeatureFlag.SQLLAB_BACKEND_PERSISTENCE)
? SupersetClient.put({
- endpoint: encodeURI(`/tabstateview/${sqlEditorId}`),
+ endpoint: encodeURI(`/tabstateview/${results?.query?.sqlEditorId}`),
Review Comment:
If you look at line 232, a recent PR of mine recently changed this logic: #22498 So this line needs to be changed back:
```suggestion
endpoint: encodeURI(`/tabstateview/${sqlEditorId}`),
```
##########
superset/config.py:
##########
@@ -1107,6 +1107,8 @@ def CSV_TO_HIVE_UPLOAD_DIRECTORY_FUNC( # pylint: disable=invalid-name
# Interval between consecutive polls when using Hive Engine
HIVE_POLL_INTERVAL = int(timedelta(seconds=5).total_seconds())
+# Interval between consecutive polls when using Impala Engine
+IMPALA_POLL_INTERVAL = int(timedelta(seconds=5).total_seconds())
Review Comment:
To make sure we don't clutter `config.py` with too many disparate config keys, maybe we should remove these `*_POLL_INTERVAL` keys and refactor this to something like
```python
DB_POLL_INTERVAL_SECONDS: Dict[str, int] = {}
```
This could be used to specify these per engine name in your `superset_config.py` (here I'd be overriding polling to 1 seconds for Hive):
```python
DB_POLL_INTERVAL_SECONDS = {
"hive": int(timedelta(seconds=1).total_seconds()),
}
```
I know it's a breaking change, so we can probably fall back to `HIVE_POLL_INTERVAL` in the hive spec for now. So maybe change the following line https://github.com/apache/superset/blob/01671b9d1b3a15c264bcfb9eced1776c70e293b5/superset/db_engine_specs/hive.py#L378 to something like this (see how I've moved the default to the engine spec, rather than having it in `config.py`):
```python
if sleep_interval := current_app.config.get("HIVE_POLL_INTERVAL"):
logger.warning("HIVE_POLL_INTERVAL is deprecated and will be removed in 3.0. Please use DB_POLL_INTERVAL instead")
else:
sleep_interval = current_app.config["DB_POLL_INTERVAL_SECONDS"].get(cls.engine, 5)
time.sleep(sleep_interval)
```
--
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 #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22441:
URL: https://github.com/apache/superset/pull/22441#issuecomment-1355106575
# [Codecov](https://codecov.io/gh/apache/superset/pull/22441?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 [#22441](https://codecov.io/gh/apache/superset/pull/22441?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (55c49ce) into [master](https://codecov.io/gh/apache/superset/commit/2679ee2e46edf53ab07c19e1186ce2877e159303?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2679ee2) will **decrease** coverage by `14.46%`.
> The diff coverage is `25.67%`.
```diff
@@ Coverage Diff @@
## master #22441 +/- ##
===========================================
- Coverage 66.90% 52.44% -14.47%
===========================================
Files 1850 1850
Lines 70701 70774 +73
Branches 7750 7750
===========================================
- Hits 47306 37117 -10189
- Misses 21379 31641 +10262
Partials 2016 2016
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `?` | |
| python | `50.89% <24.65%> (-30.36%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `50.89% <24.65%> (-0.06%)` | :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/22441?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/sql\_lab.py](https://codecov.io/gh/apache/superset/pull/22441/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-c3VwZXJzZXQvc3FsX2xhYi5weQ==) | `35.33% <0.00%> (-46.80%)` | :arrow_down: |
| [superset/views/core.py](https://codecov.io/gh/apache/superset/pull/22441/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-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `27.71% <0.00%> (-47.73%)` | :arrow_down: |
| [superset/db\_engine\_specs/impala.py](https://codecov.io/gh/apache/superset/pull/22441/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-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2ltcGFsYS5weQ==) | `34.78% <25.00%> (-48.56%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/actions/sqlLab.js](https://codecov.io/gh/apache/superset/pull/22441/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9hY3Rpb25zL3NxbExhYi5qcw==) | `63.47% <100.00%> (ø)` | |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/22441/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-c3VwZXJzZXQvY29uZmlnLnB5) | `90.88% <100.00%> (-0.59%)` | :arrow_down: |
| [superset/examples/css\_templates.py](https://codecov.io/gh/apache/superset/pull/22441/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-c3VwZXJzZXQvZXhhbXBsZXMvY3NzX3RlbXBsYXRlcy5weQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset/utils/dashboard\_import\_export.py](https://codecov.io/gh/apache/superset/pull/22441/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/migrations/shared/migrate\_viz/\_\_init\_\_.py](https://codecov.io/gh/apache/superset/pull/22441/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-c3VwZXJzZXQvbWlncmF0aW9ucy9zaGFyZWQvbWlncmF0ZV92aXovX19pbml0X18ucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...perset/migrations/shared/migrate\_viz/processors.py](https://codecov.io/gh/apache/superset/pull/22441/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-c3VwZXJzZXQvbWlncmF0aW9ucy9zaGFyZWQvbWlncmF0ZV92aXovcHJvY2Vzc29ycy5weQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset/reports/commands/alert.py](https://codecov.io/gh/apache/superset/pull/22441/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-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9hbGVydC5weQ==) | `0.00% <0.00%> (-96.71%)` | :arrow_down: |
| ... and [311 more](https://codecov.io/gh/apache/superset/pull/22441/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] wanghong1314 commented on a diff in pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
wanghong1314 commented on code in PR #22441:
URL: https://github.com/apache/superset/pull/22441#discussion_r1051286271
##########
docker/pythonpath_dev/superset_config.py:
##########
@@ -106,6 +106,7 @@ class CeleryConfig(object):
WEBDRIVER_BASEURL = "http://superset:8088/"
# The base URL for the email report hyperlinks.
WEBDRIVER_BASEURL_USER_FRIENDLY = WEBDRIVER_BASEURL
+IMPALA_POLL_INTERVAL = int(timedelta(seconds=5).total_seconds())
Review Comment:
I added this parameter for running the docker container environment, and I can remove it if don't need it
--
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] wanghong1314 commented on a diff in pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
wanghong1314 commented on code in PR #22441:
URL: https://github.com/apache/superset/pull/22441#discussion_r1051292047
##########
superset-frontend/src/SqlLab/actions/sqlLab.js:
##########
@@ -229,6 +229,9 @@ export function startQuery(query) {
export function querySuccess(query, results) {
return function (dispatch) {
+ if (!results.query) {
Review Comment:
yeah ,to prevent query.sqlEditorId is undefined, I have tested your method, it is OK, thank you for your suggestion, I need to submit a PR again?
<img width="986" alt="image" src="https://user-images.githubusercontent.com/10183241/208219063-bbbdee11-032e-47b1-bdb2-d5ce0fa90b52.png">
--
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] wanghong1314 commented on a diff in pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
wanghong1314 commented on code in PR #22441:
URL: https://github.com/apache/superset/pull/22441#discussion_r1051329866
##########
superset-frontend/src/SqlLab/actions/sqlLab.js:
##########
@@ -229,6 +229,9 @@ export function startQuery(query) {
export function querySuccess(query, results) {
return function (dispatch) {
+ if (!results.query) {
Review Comment:
the code has been changed, please kindly review it again
--
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] wanghong1314 commented on pull request #22441: fix: stop query in SQL Lab with impala engine (#20950)
Posted by GitBox <gi...@apache.org>.
wanghong1314 commented on PR #22441:
URL: https://github.com/apache/superset/pull/22441#issuecomment-1356167380
@eschutho @bolkedebruin @eschutho I saw that the ci test passed, please help review the code, thank you
--
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