fix(20428): Address-Presto/Trino-Poll-Issue-Refactor #20434
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Thelin90
changed the title
Thelin90
changed the title
r Update linter
Thelin90
force-pushed
the
fix(20428)-Address-Presto/Trino-Poll-Issue-Refactor
branch
from
e554e8c to
a335ff1
Compare
Thelin90
changed the title
Thelin90
changed the title
Thelin90
changed the title
john-bodley
reviewed
Jun 19, 2022
superset/db_engine_specs/trino.py
Outdated
| logger.info("Query %i: Finished", query_id) | ||
| break | ||
|
|
||
| completed_splits = float(stats.get("completedSplits")) |
There was a problem hiding this comment.
Suggested change
| completed_splits = float(stats.get("completedSplits")) | |
| completed_splits = float(stats["completedSplits"]) |
superset/db_engine_specs/trino.py
Outdated
| break | ||
|
|
||
| completed_splits = float(stats.get("completedSplits")) | ||
| total_splits = float(stats.get("totalSplits")) |
There was a problem hiding this comment.
Suggested change
| total_splits = float(stats.get("totalSplits")) | |
| total_splits = float(stats["totalSplits"]) |
| @@ -949,11 +949,7 @@ def get_create_view( | |||
| sql = f"SHOW CREATE VIEW {schema}.{table}" | |||
| try: | |||
| cls.execute(cursor, sql) | |||
| polled = cursor.poll() | |||
There was a problem hiding this comment.
@Thelin90 let me test this using Presto to ensure this works. I'll report back.
There was a problem hiding this comment.
I confirmed, via the /extra_table_metadata/<int:database_id>/<table_name>/<schema>/ route—which is invokes said method for views, that the method works as expected without the polling.
superset/db_engine_specs/trino.py
Outdated
| @@ -15,15 +15,20 @@ | |||
| # specific language governing permissions and limitations | |||
| # under the License. | |||
| import logging | |||
| from typing import Any, Dict, Optional, TYPE_CHECKING | |||
| import time | |||
There was a problem hiding this comment.
You should revert any of the now unnecessary imports.
john-bodley
changed the title
Codecov Report
@@ Coverage Diff @@
## master #20434 +/- ##
=======================================
Coverage 66.70% 66.71%
=======================================
Files 1739 1739
Lines 65153 65161 +8
Branches 6899 6899
=======================================
+ Hits 43462 43469 +7
- Misses 19938 19939 +1
Partials 1753 1753
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
john-bodley
approved these changes
Jun 19, 2022
michael-s-molina
pushed a commit
that referenced
this pull request
Jun 23, 2022
* fix(20428)-Address-Presto/Trino-Poll-Issue-Refacto r Update linter * Update to only use BaseEngineSpec handle_cursor * Fix CI Co-authored-by: John Bodley <[email protected]> (cherry picked from commit 8b7262f)
3 tasks
michael-s-molina
pushed a commit
that referenced
this pull request
Aug 30, 2022
* fix(20428)-Address-Presto/Trino-Poll-Issue-Refacto r Update linter * Update to only use BaseEngineSpec handle_cursor * Fix CI Co-authored-by: John Bodley <[email protected]>
mistercrunch
added
🍒 1.5.2
🍒 1.5.3
🍒 2.0.0
🍒 2.0.1
🚢 2.1.3
🚢 2.1.0
and removed
🍒 2.0.1
🍒 1.5.3
labels
mistercrunch
added
🍒 1.5.2
🍒 1.5.3
🍒 2.0.0
🍒 2.0.1
🏷️ bot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
Change to modify
TrinoEngineSpecinherit classPrestoEngineSpecto address bug when running againstpresto-trino.Given issue:
Bug
Raised issue: #20428
Slack conversation: https://apache-superset.slack.com/archives/C015WAZL0KH/p1655449901353689
Additional Info:
See quote:
@S Thelin
I don’t believe we can provide query stats given the way the Trino client is configured. Statistics are updated when fetch is called per here, however this also fetches results and we cannot advance the cursor in the handle_cursor method. There could be some trickery we could do, but I think it’s safer to simply fallback to the default method.
@john-bodley