-
Notifications
You must be signed in to change notification settings - Fork 13.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: more specific presto error messages #11099
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no clue, but i was matching the line breaks from earlier in the file. @pkdotson could you comment? |
||
validate that all columns or tables referenced within the query exist and are spelled | ||
correctly. | ||
|
||
## Issue 1004 | ||
|
||
``` | ||
The column was deleted or renamed in the database. | ||
``` | ||
|
||
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 deleted or renamed in the database. | ||
``` | ||
|
||
Your query failed because it is referencing a table that no longer exists in | ||
the underlying database. You should modify your query to reference the correct | ||
table. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ | |
# KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations | ||
# under the License. | ||
import dataclasses | ||
import logging | ||
import re | ||
import textwrap | ||
|
@@ -27,7 +28,7 @@ | |
|
||
import pandas as pd | ||
import simplejson as json | ||
from flask_babel import lazy_gettext as _ | ||
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 | ||
|
@@ -38,6 +39,7 @@ | |
|
||
from superset import app, cache, is_feature_enabled, security_manager | ||
from superset.db_engine_specs.base import BaseEngineSpec | ||
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType | ||
from superset.exceptions import SupersetTemplateException | ||
from superset.models.sql_lab import Query | ||
from superset.models.sql_types.presto_sql_types import ( | ||
|
@@ -55,6 +57,9 @@ | |
# prevent circular imports | ||
from superset.models.core import Database | ||
|
||
COLUMN_NOT_RESOLVED_ERROR_REGEX = "line (.+?): .*Column '(.+?)' cannot be resolved" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this answered my previous question. |
||
TABLE_DOES_NOT_EXIST_ERROR_REGEX = ".*Table (.+?) does not exist" | ||
|
||
QueryStatus = utils.QueryStatus | ||
config = app.config | ||
logger = logging.getLogger(__name__) | ||
|
@@ -1035,3 +1040,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.COLUMN_DOES_NOT_EXIST_ERROR, | ||
message=__( | ||
'We can\'t seem to resolve the column "%(column_name)s" at ' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we use a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is how flask_babel says to do string replacement in translation functions. I'm not sure this works with f strings by default (and am uncertain how to test): https://flask-babel.tkte.ch/#using-translations |
||
"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.TABLE_DOES_NOT_EXIST_ERROR, | ||
message=__( | ||
'The table "%(table_name)s" does not exist. ' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See ^^. |
||
"A valid table must be used to run this query.", | ||
table_name=table_match.group(1), | ||
), | ||
level=ErrorLevel.ERROR, | ||
extra={"engine_name": cls.engine_name}, | ||
) | ||
) | ||
] | ||
|
||
return [ | ||
dataclasses.asdict( | ||
SupersetError( | ||
error_type=SupersetErrorType.GENERIC_DB_ENGINE_ERROR, | ||
message=cls._extract_error_message(ex), | ||
level=ErrorLevel.ERROR, | ||
extra={"engine_name": cls.engine_name}, | ||
) | ||
) | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My hope was this this could actually be db agnostic, and we could use the same issue number for any db