fix(Trino): create `PrestoBaseEngineSpec` base class to share common code between Trino and Presto #21066

dungdm93 · 2022-08-12T03:50:09Z

SUMMARY

#20152 Making the TrinoEngineSpec derive from the PrestoEngineSpec to reusing code. However, underlay driver of Trino is different from Presto - PyHive which lead to some issues. (See discussion in #20729)

This PR is rework on that PR by create PrestoBaseEngineSpec to share common functions between Presto variants.

Fix:

Suppress:

fix: Ensure Presto database engine spec correctly handles Trino #20729

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Unit tests

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

dungdm93 · 2022-08-12T04:10:28Z

cc @villebro, @john-bodley
Could you guys help me listing issues, or features in Presto u like to have in Trino.

codecov · 2022-08-12T07:36:03Z

Codecov Report

Merging #21066 (dedcacf) into master (5113b01) will decrease coverage by 0.06%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master   #21066      +/-   ##
==========================================
- Coverage   66.28%   66.21%   -0.07%     
==========================================
  Files        1770     1770              
  Lines       67522    67574      +52     
  Branches     7177     7177              
==========================================
- Hits        44754    44747       -7     
- Misses      20934    20993      +59     
  Partials     1834     1834

Flag	Coverage Δ
hive	`53.14% <31.76%> (-0.03%)`	⬇️
mysql	`80.97% <80.00%> (+0.01%)`	⬆️
postgres	`?`
presto	`53.04% <44.70%> (-0.03%)`	⬇️
python	`81.34% <80.00%> (-0.16%)`	⬇️
sqlite	`?`
unit	`50.71% <38.82%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
superset/db_engine_specs/hive.py	`87.10% <ø> (+0.23%)`	⬆️
superset/db_engine_specs/trino.py	`69.02% <43.47%> (-5.74%)`	⬇️
superset/db_engine_specs/presto.py	`87.95% <93.54%> (-0.03%)`	⬇️
superset/sql_validators/postgres.py	`50.00% <0.00%> (-50.00%)`	⬇️
superset/sql_validators/__init__.py	`80.00% <0.00%> (-20.00%)`	⬇️
superset/sqllab/sql_json_executer.py	`64.63% <0.00%> (-9.76%)`	⬇️
superset/sqllab/execution_context_convertor.py	`81.48% <0.00%> (-7.41%)`	⬇️
superset/db_engine_specs/postgres.py	`90.67% <0.00%> (-5.94%)`	⬇️
superset/db_engine_specs/sqlite.py	`91.89% <0.00%> (-5.41%)`	⬇️
superset/reports/commands/log_prune.py	`85.71% <0.00%> (-3.58%)`	⬇️
... and 31 more

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

villebro

Great work! Tested to work as expected on Trino with a few comments, LMKWYT.

villebro · 2022-08-24T06:40:06Z

superset/db_engine_specs/presto.py

 from sqlalchemy.orm import Session
 from sqlalchemy.sql.expression import ColumnClause, Select

 from superset import cache_manager, is_feature_enabled
 from superset.common.db_query_status import QueryStatus
 from superset.databases.utils import make_url_safe
-from superset.db_engine_specs.base import BaseEngineSpec, ColumnTypeMapping
+from superset.db_engine_specs.base import ColumnTypeMapping
+from superset.db_engine_specs.presto_base import PrestoBaseEngineSpec


I think we could just as well keep PrestoBaseEngineSpec in superset.db_engine_specs.presto, like we do for PostgresBaseEngineSpec, as it kinda keeps the list of modules in db_engine_specs shorter. Not strongly opinionated here, but let's at least keep it consistent; if we do decide to break out the bases, then I'd prefer to do it in a follow-up refactor PR and doing the same for other specs, too.

villebro · 2022-08-24T06:43:08Z

superset/db_engine_specs/trino.py

-    @classmethod
-    def get_table_names(
-        cls,
-        database: Database,
-        inspector: Inspector,
-        schema: Optional[str],
-    ) -> List[str]:
-        return BaseEngineSpec.get_table_names(
-            database=database,
-            inspector=inspector,
-            schema=schema,
-        )
-
-    @classmethod
-    def get_view_names(
-        cls,
-        database: Database,
-        inspector: Inspector,
-        schema: Optional[str],
-    ) -> List[str]:
-        return BaseEngineSpec.get_view_names(
-            database=database,
-            inspector=inspector,
-            schema=schema,
-        )
-


We can also remove has_implicit_cancel, as it's no longer needed (it returns the same as BaseEngineSpec which isn't overridden in PrestoBaseEngineSpec)

has_implicit_cancel is removed

dungdm93 · 2022-08-24T11:41:38Z

superset/db_engine_specs/trino.py

    engine = "trino"
-    engine_aliases = {"trinonative"}  # Required for backwards compatibility.


trinonative in setup.py already removed since #20152

villebro

LGTM assuming CI doesn't turn up any goblins

…s between Presto and Trino Signed-off-by: Đặng Minh Dũng <dungdm93@live.com>

Signed-off-by: Đặng Minh Dũng <dungdm93@live.com>

…code between Trino and Presto (apache#21066) * chore: create `PrestoBaseEngineSpec` class that share common functions between Presto and Trino Signed-off-by: Đặng Minh Dũng <dungdm93@live.com> * feat(Trino): support CertificateAuthentication * chore(Presto): move `get_function_names` to `PrestoBaseEngineSpec` Signed-off-by: Đặng Minh Dũng <dungdm93@live.com> * chores(Presto): remove `is_readonly_query` * feat(Trino): implement `extra_table_metadata` * feat(Trino): specify `User-Agent` Signed-off-by: Đặng Minh Dũng <dungdm93@live.com> * fix: pylint Signed-off-by: Đặng Minh Dũng <dungdm93@live.com> * chores(Presto): move `PrestoBaseEngineSpec` to `presto.py` Signed-off-by: Đặng Minh Dũng <dungdm93@live.com> * fix(Presto): typing annotations Signed-off-by: Đặng Minh Dũng <dungdm93@live.com> Signed-off-by: Đặng Minh Dũng <dungdm93@live.com> (cherry picked from commit ccb293a)

john-bodley · 2022-12-15T10:28:58Z

@dungdm93 and @villebro the issue I see with this approach is that it introduced several regressions (from a feature parity perspective; especially given that a migration path from Presto to Trino is somewhat likely—see here for details), i.e., the trino.latest_partition, trino.latest_sub_partition macros no longer work as this functionality is part of the PrestoEngineSpec rather than the PrestoBaseEngineSpec.

Are there plans to either:

Add this functionality to the PrestoBaseEngineSpec?
Port this functionality to the TrinoEngineSpec?

villebro · 2022-12-15T18:34:05Z

@john-bodley the problem is that PyHive and the new Trino connector are diverging at a rapid pace right now, so extending the Trino spec from the old Presto spec is IMO not a good idea (that actually created a few regressions of its own when we did that). I'm fine moving everything that's known to work on both to PrestoBaseEngineSpec (I can chip in with adding tests when I have some spare time).

john-bodley · 2022-12-15T18:37:23Z

@villebro understood. I'm 50/50 about whether option 1 or 2 is the preferred solution. Option 1 might be easiest for now (conditional on how coupled the logic is with the DB-API), but long term I sense option 2 is likely preferred.

pull-request-size bot added the size/L label Aug 12, 2022

dungdm93 changed the title ~~Base presto and trino~~ fix(Trino): create PrestoBaseEngineSpec base class to share common code between Trino and Presto Aug 12, 2022

dungdm93 force-pushed the base-presto-and-trino branch 4 times, most recently from 4b86bf3 to 38d6438 Compare August 12, 2022 07:29

dungdm93 force-pushed the base-presto-and-trino branch 2 times, most recently from 441ab0a to 6c4da3a Compare August 12, 2022 08:18

rusackas requested review from john-bodley, betodealmeida and nytai August 23, 2022 03:16

villebro reviewed Aug 24, 2022

View reviewed changes

dungdm93 force-pushed the base-presto-and-trino branch from bcbd7d8 to bbede4e Compare August 24, 2022 11:36

dungdm93 requested review from villebro and removed request for betodealmeida, john-bodley and nytai August 24, 2022 11:39

dungdm93 commented Aug 24, 2022

View reviewed changes

villebro approved these changes Aug 24, 2022

View reviewed changes

dungdm93 added 9 commits August 25, 2022 09:23

chore: create PrestoBaseEngineSpec class that share common function…

ea893bc

…s between Presto and Trino Signed-off-by: Đặng Minh Dũng <dungdm93@live.com>

feat(Trino): support CertificateAuthentication

105a950

chore(Presto): move get_function_names to PrestoBaseEngineSpec

0efc29b

Signed-off-by: Đặng Minh Dũng <dungdm93@live.com>

chores(Presto): remove is_readonly_query

640ac38

feat(Trino): implement extra_table_metadata

ac11fcb

feat(Trino): specify User-Agent

0017166

Signed-off-by: Đặng Minh Dũng <dungdm93@live.com>

fix: pylint

e37e66c

Signed-off-by: Đặng Minh Dũng <dungdm93@live.com>

chores(Presto): move PrestoBaseEngineSpec to presto.py

dd92edc

Signed-off-by: Đặng Minh Dũng <dungdm93@live.com>

fix(Presto): typing annotations

c759331

Signed-off-by: Đặng Minh Dũng <dungdm93@live.com>

dungdm93 force-pushed the base-presto-and-trino branch from dedcacf to c759331 Compare August 25, 2022 02:24

villebro merged commit ccb293a into apache:master Aug 29, 2022

dungdm93 deleted the base-presto-and-trino branch August 29, 2022 07:23

john-bodley mentioned this pull request Sep 2, 2022

fix: Ensure Presto database engine spec correctly handles Trino #20729

Closed

9 tasks

mdesmet mentioned this pull request Oct 6, 2022

Superset does not cast trino timestamp correctly #21567

Closed

john-bodley mentioned this pull request Dec 16, 2022

chore: Re-add inheritance of Presto macros for Trino et al. #22435

Merged

9 tasks

Khrol mentioned this pull request Feb 1, 2023

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

Closed

9 tasks

rusackas added the v2.0.2 label Feb 2, 2023

lilykuang added the v2.0 label Feb 23, 2023

eschutho removed v2.0 v2.0.2 labels Mar 1, 2023

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Trino): create `PrestoBaseEngineSpec` base class to share common code between Trino and Presto #21066

fix(Trino): create `PrestoBaseEngineSpec` base class to share common code between Trino and Presto #21066

dungdm93 commented Aug 12, 2022 •

edited

Loading

dungdm93 commented Aug 12, 2022

codecov bot commented Aug 12, 2022 •

edited

Loading

villebro left a comment

villebro Aug 24, 2022

villebro Aug 24, 2022

dungdm93 Aug 24, 2022

dungdm93 Aug 24, 2022

villebro left a comment

john-bodley commented Dec 15, 2022 •

edited

Loading

villebro commented Dec 15, 2022

john-bodley commented Dec 15, 2022

		engine = "trino"
		engine_aliases = {"trinonative"} # Required for backwards compatibility.

fix(Trino): create PrestoBaseEngineSpec base class to share common code between Trino and Presto #21066

fix(Trino): create PrestoBaseEngineSpec base class to share common code between Trino and Presto #21066

Conversation

dungdm93 commented Aug 12, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

dungdm93 commented Aug 12, 2022

codecov bot commented Aug 12, 2022 • edited Loading

Codecov Report

villebro left a comment

Choose a reason for hiding this comment

villebro Aug 24, 2022

Choose a reason for hiding this comment

villebro Aug 24, 2022

Choose a reason for hiding this comment

dungdm93 Aug 24, 2022

Choose a reason for hiding this comment

dungdm93 Aug 24, 2022

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

john-bodley commented Dec 15, 2022 • edited Loading

villebro commented Dec 15, 2022

john-bodley commented Dec 15, 2022

fix(Trino): create `PrestoBaseEngineSpec` base class to share common code between Trino and Presto #21066

fix(Trino): create `PrestoBaseEngineSpec` base class to share common code between Trino and Presto #21066

dungdm93 commented Aug 12, 2022 •

edited

Loading

codecov bot commented Aug 12, 2022 •

edited

Loading

john-bodley commented Dec 15, 2022 •

edited

Loading