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 2020/09/29 17:06:22 UTC
[GitHub] [incubator-superset] john-bodley commented on a change in pull request #11099: feat: more specific presto error messages
john-bodley commented on a change in pull request #11099:
URL: https://github.com/apache/incubator-superset/pull/11099#discussion_r496892220
##########
File path: docs/src/pages/docs/Miscellaneous/issue_codes.mdx
##########
@@ -43,3 +43,33 @@ This may be due to a syntax error, a bug in your query, or some other
internal failure within the database. This is usually not an
issue within Superset, but instead a problem with the underlying
database that serves your query.
+
+## Issue 1003
Review comment:
I gather this is fine for now, i.e., the encoding can be mutated in the future, but I do wonder if issues should be engine specific and thus are identifiable, i.e., `PRESTO-1001`.
##########
File path: docs/src/pages/docs/Miscellaneous/issue_codes.mdx
##########
@@ -43,3 +43,33 @@ This may be due to a syntax error, a bug in your query, or some other
internal failure within the database. This is usually not an
issue within Superset, but instead a problem with the underlying
database that serves your query.
+
+## Issue 1003
+
+```
+There is a syntax error in the SQL query. Perhaps there was a misspelling or a typo.
+```
+
+Your query failed because of a syntax error within the underlying query. Please
+validate that all columns or tables referenced within the query are spelled
+correctly.
+
+## Issue 1004
+
+```
+The column was changed in the underlying database structure.
Review comment:
The term "changed" may be too general, changed name, changed type? I'm also not sure about the term "database structure" or whether "underlying" is necessary. Why not simply say,
> The column no longer exists in the datasource.
Also could this error surface due to free form filters? If so the "no longer" may not be correct as it may have never existed.
##########
File path: docs/src/pages/docs/Miscellaneous/issue_codes.mdx
##########
@@ -43,3 +43,33 @@ This may be due to a syntax error, a bug in your query, or some other
internal failure within the database. This is usually not an
issue within Superset, but instead a problem with the underlying
database that serves your query.
+
+## Issue 1003
+
+```
+There is a syntax error in the SQL query. Perhaps there was a misspelling or a typo.
+```
+
+Your query failed because of a syntax error within the underlying query. Please
Review comment:
I'm not familiar with this file format but why do these messages span multiple lines, i.e., contain line breaks? Shouldn't they be on a single line (unless they're paragraphs) and the presentation layer wrap the lines accordingly?
##########
File path: superset/db_engine_specs/presto.py
##########
@@ -55,6 +57,9 @@
# prevent circular imports
from superset.models.core import Database
+COLUMN_NOT_RESOLVED_ERROR_REGEX = "line (.+?): .*Column '(.+?)' cannot be resolved"
Review comment:
Maybe this answered my previous question.
##########
File path: docs/src/pages/docs/Miscellaneous/issue_codes.mdx
##########
@@ -43,3 +43,33 @@ This may be due to a syntax error, a bug in your query, or some other
internal failure within the database. This is usually not an
issue within Superset, but instead a problem with the underlying
database that serves your query.
+
+## Issue 1003
+
+```
+There is a syntax error in the SQL query. Perhaps there was a misspelling or a typo.
+```
+
+Your query failed because of a syntax error within the underlying query. Please
+validate that all columns or tables referenced within the query are spelled
+correctly.
+
+## Issue 1004
+
+```
+The column was changed in the underlying database structure.
+```
+
+Your query failed because it is referencing a column that no longer exists in
+the underlying datasource. You should modify the query to reference the
+replacement column, or remove this column from your query.
+
+## Issue 1005
+
+```
+The table was changed in the underlying database structure.
Review comment:
Similarly why not,
> The table no longer exists in the database.
##########
File path: superset/db_engine_specs/presto.py
##########
@@ -1034,3 +1039,53 @@ def get_function_names(cls, database: "Database") -> List[str]:
:return: A list of function names useable in the database
"""
return database.get_df("SHOW FUNCTIONS")["Function"].tolist()
+
+ @classmethod
+ def extract_errors(cls, ex: Exception) -> List[Dict[str, Any]]:
+ raw_message = cls._extract_error_message(ex)
+
+ column_match = re.search(COLUMN_NOT_RESOLVED_ERROR_REGEX, raw_message)
+ if column_match:
+ return [
+ dataclasses.asdict(
+ SupersetError(
+ error_type=SupersetErrorType.PRESTO_CANNOT_RESOLVE_COLUMN_ERROR,
+ message=__(
+ 'We can\'t seem to resolve the column "%(column_name)s" at '
Review comment:
Could we use a `f` string here? The `%(column_name)s` formatting is somewhat outdated.
##########
File path: superset-frontend/src/components/ErrorMessage/types.ts
##########
@@ -26,6 +26,8 @@ export const ErrorTypeEnum = {
// DB Engine errors
GENERIC_DB_ENGINE_ERROR: 'GENERIC_DB_ENGINE_ERROR',
+ PRESTO_CANNOT_RESOLVE_COLUMN_ERROR: 'PRESTO_CANNOT_RESOLVE_COLUMN_ERROR',
Review comment:
The terms "cannot resolve" and "does not exist" are quite different, i.e., the first is more generic. I wonder if it's worth examining how these errors can surface and ensuring that we use the most specific possible.
##########
File path: superset/db_engine_specs/presto.py
##########
@@ -1034,3 +1039,53 @@ def get_function_names(cls, database: "Database") -> List[str]:
:return: A list of function names useable in the database
"""
return database.get_df("SHOW FUNCTIONS")["Function"].tolist()
+
+ @classmethod
+ def extract_errors(cls, ex: Exception) -> List[Dict[str, Any]]:
+ raw_message = cls._extract_error_message(ex)
+
+ column_match = re.search(COLUMN_NOT_RESOLVED_ERROR_REGEX, raw_message)
+ if column_match:
+ return [
+ dataclasses.asdict(
+ SupersetError(
+ error_type=SupersetErrorType.PRESTO_CANNOT_RESOLVE_COLUMN_ERROR,
+ message=__(
+ 'We can\'t seem to resolve the column "%(column_name)s" at '
+ "line %(location)s.",
+ column_name=column_match.group(2),
+ location=column_match.group(1),
+ ),
+ level=ErrorLevel.ERROR,
+ extra={"engine_name": cls.engine_name},
+ )
+ )
+ ]
+
+ table_match = re.search(TABLE_DOES_NOT_EXIST_ERROR_REGEX, raw_message)
+ if table_match:
+ return [
+ dataclasses.asdict(
+ SupersetError(
+ error_type=SupersetErrorType.PRESTO_TABLE_DOES_NOT_EXIST_ERROR,
+ message=__(
+ 'The table "%(table_name)s" does not exist. '
Review comment:
See ^^.
----------------------------------------------------------------
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.
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