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: first step native support Trino #13105

Merged
merged 3 commits into from
Feb 15, 2021
Merged

feat: first step native support Trino #13105

merged 3 commits into from
Feb 15, 2021

Conversation

dungdm93
Copy link
Contributor

@dungdm93 dungdm93 commented Feb 13, 2021

SUMMARY

Previously, SuperSet was able to access Presto through the PyHive, even though it's not the official python driver, as well as it is not in active development. However, since Dec 27, 2020, PrestoSQL was renamed to Trino, this mean you can't using PyHive to access Trino any more.
So this MR is first step native support Trino using official python driver

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

Unit test already added

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@codecov-io
Copy link

codecov-io commented Feb 13, 2021

Codecov Report

Merging #13105 (967a6e3) into master (2ce7982) will increase coverage by 9.05%.
The diff coverage is 36.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13105      +/-   ##
==========================================
+ Coverage   53.06%   62.11%   +9.05%     
==========================================
  Files         489      982     +493     
  Lines       17314    46314   +29000     
  Branches     4482     4505      +23     
==========================================
+ Hits         9187    28769   +19582     
- Misses       8127    17545    +9418     
Flag Coverage Δ
cypress 53.37% <50.00%> (+0.31%) ⬆️
python 67.35% <31.14%> (?)

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

Impacted Files Coverage Δ
...end/src/SqlLab/components/RunQueryActionButton.tsx 52.77% <ø> (ø)
superset-frontend/src/chart/ChartContainer.jsx 100.00% <ø> (ø)
superset-frontend/src/chart/ChartRenderer.jsx 75.67% <0.00%> (-1.04%) ⬇️
...perset-frontend/src/common/components/Dropdown.tsx 50.00% <ø> (ø)
...rontend/src/components/ListView/CardSortSelect.tsx 78.94% <ø> (ø)
...ontend/src/components/ListViewCard/ImageLoader.tsx 75.00% <0.00%> (ø)
...set-frontend/src/components/URLShortLinkButton.jsx 100.00% <ø> (ø)
...rset-frontend/src/components/URLShortLinkModal.tsx 77.77% <ø> (ø)
...src/dashboard/components/HeaderActionsDropdown.jsx 68.49% <ø> (+0.92%) ⬆️
...end/src/dashboard/components/StickyVerticalBar.tsx 100.00% <ø> (ø)
... and 545 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update acca3a4...967a6e3. Read the comment docs.

Signed-off-by: Đặng Minh Dũng <dungdm93@live.com>
Signed-off-by: Đặng Minh Dũng <dungdm93@live.com>
Signed-off-by: Đặng Minh Dũng <dungdm93@live.com>
@junlincc junlincc added the new:contributor The author is a new contributor label Feb 15, 2021
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Thanks so much for this! The Presto spec has been in need of some TLC for a while now, so this is a perfect opportunity to make sure the Trino connector is well supported and has good test coverage!

@villebro villebro merged commit 2dbe92b into apache:master Feb 15, 2021
@dungdm93 dungdm93 deleted the trino branch February 15, 2021 15:42
@betodealmeida
Copy link
Member

@dungdm93 PyHive nows supports Trino. Do you think it would be better to use PyHIve, given that it's SQLAlchemy dialect is more mature, or do you think we should use the SQLAlchemy dialect you created, since it uses the official trino Python driver?

@dungdm93
Copy link
Contributor Author

Hi @betodealmeida, greet question.
PyHive code is messy, that is why I create separate package. And for your info, I also working on porting this dialect to upstream trino python client trinodb/trino-python-client#81

@findepi
Copy link
Member

findepi commented Apr 21, 2021

Yes, pyhive is definitely more mature now, but I'd still recommend the official Trino driver which we (the Trino community) maintain.
as @dungdm93 pointed out, the SQLAlchemy support is going to be integral part of that package.

@olivier-inion
Copy link

Hi, i try to use trino with superset and i got this error :'trino error: 'Cursor' object has no attribute 'poll'
( trino works well with cli or jdbc driver ).
in sqllab , i only see 'jdbc','information_schema','metadate','runtime' schemas (and none of my 4 catalogs )

it's a fresh install with docker-compose, i try to add sqlachemy-trino in 'requirements-local.txt' but logs says that this dependency is already here ( i can see it in the setup.py ).

i don't understand that i'm missing to do.

@olivier-inion
Copy link

ok; it's working with v1.5.0 superset

@mingfang
Copy link

mingfang commented Jun 8, 2022

I'm still getting the 'Cursor' object has no attribute 'poll' error in the latest docker image

@olivier-inion
Copy link

use this method to install
after cloning repo, go to superset directory and do :

git checkout 1.5.0
TAG=1.5.0 docker-compose -f docker-compose-non-dev.yml pull
TAG=1.5.0 docker-compose -f docker-compose-non-dev.yml up

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels new:contributor The author is a new contributor size/M 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants