Skip to content
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

Merged
merged 1 commit into from
Sep 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions docs/src/pages/docs/Miscellaneous/issue_codes.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

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.

Copy link
Member Author

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


```
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
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.
2 changes: 2 additions & 0 deletions superset-frontend/src/components/ErrorMessage/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ export const ErrorTypeEnum = {

// DB Engine errors
GENERIC_DB_ENGINE_ERROR: 'GENERIC_DB_ENGINE_ERROR',
COLUMN_DOES_NOT_EXIST_ERROR: 'COLUMN_DOES_NOT_EXIST_ERROR',
TABLE_DOES_NOT_EXIST_ERROR: 'TABLE_DOES_NOT_EXIST_ERROR',

// Viz errors
VIZ_GET_DF_ERROR: 'VIZ_GET_DF_ERROR',
Expand Down
8 changes: 8 additions & 0 deletions superset-frontend/src/setup/setupErrorMessages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,5 +38,13 @@ export default function setupErrorMessages() {
ErrorTypeEnum.GENERIC_DB_ENGINE_ERROR,
DatabaseErrorMessage,
);
errorMessageComponentRegistry.registerValue(
ErrorTypeEnum.COLUMN_DOES_NOT_EXIST_ERROR,
DatabaseErrorMessage,
);
errorMessageComponentRegistry.registerValue(
ErrorTypeEnum.TABLE_DOES_NOT_EXIST_ERROR,
DatabaseErrorMessage,
);
setupErrorMessagesExtra();
}
57 changes: 56 additions & 1 deletion superset/db_engine_specs/presto.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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 (
Expand All @@ -55,6 +57,9 @@
# prevent circular imports
from superset.models.core import Database

COLUMN_NOT_RESOLVED_ERROR_REGEX = "line (.+?): .*Column '(.+?)' cannot be resolved"
Copy link
Member

Choose a reason for hiding this comment

The 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__)
Expand Down Expand Up @@ -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 '
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use a f string here? The %(column_name)s formatting is somewhat outdated.

Copy link
Member Author

Choose a reason for hiding this comment

The 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. '
Copy link
Member

Choose a reason for hiding this comment

The 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},
)
)
]
32 changes: 32 additions & 0 deletions superset/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ class SupersetErrorType(str, Enum):

# DB Engine errors
GENERIC_DB_ENGINE_ERROR = "GENERIC_DB_ENGINE_ERROR"
COLUMN_DOES_NOT_EXIST_ERROR = "COLUMN_DOES_NOT_EXIST_ERROR"
TABLE_DOES_NOT_EXIST_ERROR = "TABLE_DOES_NOT_EXIST_ERROR"

# Viz errors
VIZ_GET_DF_ERROR = "VIZ_GET_DF_ERROR"
Expand Down Expand Up @@ -68,6 +70,36 @@ class SupersetErrorType(str, Enum):
"message": _("Issue 1002 - The database returned an unexpected error."),
}
],
SupersetErrorType.COLUMN_DOES_NOT_EXIST_ERROR: [
{
"code": 1003,
"message": _(
"Issue 1003 - There is a syntax error in the SQL query. "
"Perhaps there was a misspelling or a typo."
),
},
{
"code": 1004,
"message": _(
"Issue 1004 - The column was deleted or renamed in the database."
),
},
],
SupersetErrorType.TABLE_DOES_NOT_EXIST_ERROR: [
{
"code": 1003,
"message": _(
"Issue 1003 - There is a syntax error in the SQL query. "
"Perhaps there was a misspelling or a typo."
),
},
{
"code": 1005,
"message": _(
"Issue 1005 - The table was deleted or renamed in the database."
),
},
],
}


Expand Down