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

fix: Trino - handle proper exception type in get_create_view #22921

Closed
wants to merge 1 commit into from

Conversation

Khrol
Copy link
Contributor

@Khrol Khrol commented Jan 31, 2023

SUMMARY

TrinoEngineSpec inherits PrestoBaseEngineSpec while the underlying sqlalchemy engine is throwing a different exception type if the table is not a view.

Traceback (most recent call last):
  File "/opt/superset/lib/python3.9/site-packages/flask_appbuilder/api/__init__.py", line 109, in wraps
    return f(self, *args, **kwargs)
  File "/opt/superset/lib/python3.9/site-packages/superset/views/base_api.py", line 113, in wraps
    raise ex
  File "/opt/superset/lib/python3.9/site-packages/superset/views/base_api.py", line 110, in wraps
    duration, response = time_function(f, self, *args, **kwargs)
  File "/opt/superset/lib/python3.9/site-packages/superset/utils/core.py", line 1524, in time_function
    response = func(*args, **kwargs)
  File "/opt/superset/lib/python3.9/site-packages/superset/utils/log.py", line 245, in wrapper
    value = f(*args, **kwargs)
  File "/opt/superset/lib/python3.9/site-packages/superset/databases/api.py", line 601, in table_extra_metadata
    payload = database.db_engine_spec.extra_table_metadata(
  File "/opt/superset/lib/python3.9/site-packages/superset/db_engine_specs/presto.py", line 927, in extra_table_metadata
    Any, cls.get_create_view(database, schema_name, table_name)
  File "/opt/superset/lib/python3.9/site-packages/superset/db_engine_specs/presto.py", line 951, in get_create_view
    cls.execute(cursor, sql)
  File "/opt/superset/lib/python3.9/site-packages/superset/db_engine_specs/base.py", line 1261, in execute
    raise cls.get_dbapi_mapped_exception(ex)
  File "/opt/superset/lib/python3.9/site-packages/superset/db_engine_specs/base.py", line 1259, in execute
    cursor.execute(query)
  File "/opt/superset/lib/python3.9/site-packages/trino/dbapi.py", line 439, in execute
    result = self._query.execute()
  File "/opt/superset/lib/python3.9/site-packages/trino/client.py", line 765, in execute
    self._result.rows += self.fetch()
  File "/opt/superset/lib/python3.9/site-packages/trino/client.py", line 780, in fetch
    status = self._request.process(response)
  File "/opt/superset/lib/python3.9/site-packages/trino/client.py", line 581, in process
    raise self._process_error(response["error"], response.get("id"))
trino.exceptions.TrinoUserError: TrinoUserError(type=USER_ERROR, name=NOT_SUPPORTED, message="line 1:1: Relation 'asdf' is a table, not a view", query_id=20230131_110859_18306_tzku9)

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Open "SQL Lab" with Trino connection and browse up to some specific table.
While the table info is loaded well, the error is shown to the user in the bottom right corner.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • 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

TrinoEngineSpec inherits PrestoBaseEngineSpec while the underlying sqlalchemy engine is throwing a different exception type if the table is not a view.

```
Traceback (most recent call last):
  File "/opt/superset/lib/python3.9/site-packages/flask_appbuilder/api/__init__.py", line 109, in wraps
    return f(self, *args, **kwargs)
  File "/opt/superset/lib/python3.9/site-packages/superset/views/base_api.py", line 113, in wraps
    raise ex
  File "/opt/superset/lib/python3.9/site-packages/superset/views/base_api.py", line 110, in wraps
    duration, response = time_function(f, self, *args, **kwargs)
  File "/opt/superset/lib/python3.9/site-packages/superset/utils/core.py", line 1524, in time_function
    response = func(*args, **kwargs)
  File "/opt/superset/lib/python3.9/site-packages/superset/utils/log.py", line 245, in wrapper
    value = f(*args, **kwargs)
  File "/opt/superset/lib/python3.9/site-packages/superset/databases/api.py", line 601, in table_extra_metadata
    payload = database.db_engine_spec.extra_table_metadata(
  File "/opt/superset/lib/python3.9/site-packages/superset/db_engine_specs/presto.py", line 927, in extra_table_metadata
    Any, cls.get_create_view(database, schema_name, table_name)
  File "/opt/superset/lib/python3.9/site-packages/superset/db_engine_specs/presto.py", line 951, in get_create_view
    cls.execute(cursor, sql)
  File "/opt/superset/lib/python3.9/site-packages/superset/db_engine_specs/base.py", line 1261, in execute
    raise cls.get_dbapi_mapped_exception(ex)
  File "/opt/superset/lib/python3.9/site-packages/superset/db_engine_specs/base.py", line 1259, in execute
    cursor.execute(query)
  File "/opt/superset/lib/python3.9/site-packages/trino/dbapi.py", line 439, in execute
    result = self._query.execute()
  File "/opt/superset/lib/python3.9/site-packages/trino/client.py", line 765, in execute
    self._result.rows += self.fetch()
  File "/opt/superset/lib/python3.9/site-packages/trino/client.py", line 780, in fetch
    status = self._request.process(response)
  File "/opt/superset/lib/python3.9/site-packages/trino/client.py", line 581, in process
    raise self._process_error(response["error"], response.get("id"))
trino.exceptions.TrinoUserError: TrinoUserError(type=USER_ERROR, name=NOT_SUPPORTED, message="line 1:1: Relation 'asdf' is a table, not a view", query_id=20230131_110859_18306_tzku9)
```
@Khrol Khrol changed the title Trino: handle proper exception type in get_create_view fix: Trino - handle proper exception type in get_create_view Jan 31, 2023
@rusackas
Copy link
Member

Thanks for the contribution!

Approving CI run... nag us here (or on Slack) if it needs to be run again.

Pinging a few people for review that know their way around this spot.

@codecov
Copy link

codecov bot commented Jan 31, 2023

Codecov Report

Merging #22921 (3907162) into master (cd6fc35) will decrease coverage by 0.01%.
The diff coverage is 28.57%.

@@            Coverage Diff             @@
##           master   #22921      +/-   ##
==========================================
- Coverage   67.36%   67.35%   -0.01%     
==========================================
  Files        1876     1876              
  Lines       72066    72073       +7     
  Branches     7868     7868              
==========================================
+ Hits        48545    48547       +2     
- Misses      21504    21509       +5     
  Partials     2017     2017              
Flag Coverage Δ
hive 52.72% <28.57%> (-0.01%) ⬇️
mysql 78.19% <28.57%> (-0.02%) ⬇️
postgres 78.26% <28.57%> (-0.02%) ⬇️
presto 52.62% <28.57%> (-0.01%) ⬇️
python 82.04% <28.57%> (-0.02%) ⬇️
sqlite 76.57% <28.57%> (-0.01%) ⬇️
unit 52.41% <28.57%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/db_engine_specs/trino.py 80.91% <28.57%> (-2.96%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Khrol Khrol marked this pull request as draft January 31, 2023 20:12
@Khrol
Copy link
Contributor Author

Khrol commented Jan 31, 2023

superset/db_engine_specs/trino.py:282: error: "get_create_view" undefined in superclass

Marked PR as draft while making mypy happy and adding a unit test.

@Khrol
Copy link
Contributor Author

Khrol commented Feb 1, 2023

@rusackas the problem from this PR is fixed by @dungdm93 in #21066 but it's not included yet in the latest 2.0.1 release. PR was merged on Dec 15 almost the same time as 2.0.1 was published.

When do you plan to release 2.0.2?

@Khrol Khrol closed this Feb 1, 2023
@rusackas
Copy link
Member

rusackas commented Feb 2, 2023

Not sure about 2.0.2's timeline at the moment. It should also be in 2.1 or 3.0, whichever gets released first, since both cut from master. I've labeled the merged PR for 2.0.2, and added it to the Discussion here: #22630

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants