Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: add Databricks ODBC engine spec #16862

Merged
merged 2 commits into from
Sep 29, 2021

Conversation

betodealmeida
Copy link
Member

@betodealmeida betodealmeida commented Sep 27, 2021

SUMMARY

Add an ODBC based Databricks DB engine spec. This allows Superset to connect to the new SQL endpoints.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

I was able to connect Superset to a Databricks cluster via ODBC, as well as to a SQL endpoint.

Screenshot 2021-09-27 at 14-05-03  DEV  Superset

The steps to reproduce:

  1. Follow the docs that I added. :)
  2. Run a query.

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

@betodealmeida betodealmeida requested review from villebro and removed request for villebro September 27, 2021 22:41
@codecov
Copy link

codecov bot commented Sep 27, 2021

Codecov Report

Merging #16862 (64574ce) into master (4086bed) will decrease coverage by 0.22%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16862      +/-   ##
==========================================
- Coverage   77.00%   76.77%   -0.23%     
==========================================
  Files        1018     1021       +3     
  Lines       54654    54733      +79     
  Branches     7454     7454              
==========================================
- Hits        42086    42022      -64     
- Misses      12324    12467     +143     
  Partials      244      244              
Flag Coverage Δ
hive ?
mysql 81.84% <83.33%> (+<0.01%) ⬆️
postgres 81.91% <83.33%> (-0.04%) ⬇️
presto ?
python 81.99% <83.33%> (-0.46%) ⬇️
sqlite 81.52% <83.33%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
superset/db_engine_specs/hive.py 69.76% <66.66%> (-16.90%) ⬇️
superset/db_engine_specs/databricks.py 90.00% <86.66%> (-10.00%) ⬇️
superset/db_engines/hive.py 0.00% <0.00%> (-82.15%) ⬇️
superset/db_engine_specs/presto.py 83.47% <0.00%> (-6.49%) ⬇️
superset/db_engine_specs/__init__.py 46.15% <0.00%> (-3.85%) ⬇️
superset/db_engine_specs/druid.py 86.27% <0.00%> (-2.62%) ⬇️
superset/connectors/sqla/models.py 85.59% <0.00%> (-2.16%) ⬇️
superset/db_engine_specs/bigquery.py 84.71% <0.00%> (-2.13%) ⬇️
superset/views/utils.py 81.81% <0.00%> (-1.76%) ⬇️
... and 19 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 4086bed...64574ce. Read the comment docs.

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.

Nice to see support added for the new SQL endpoints! 💯 One minor optional nit/comment, other than that LGTM

Comment on lines 23 to 27
engine_name = "Databricks"
engine_name = "Databricks (Hive)"
Copy link
Member

Choose a reason for hiding this comment

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

nit: to avoid confusion, maybe this should be "Databricks (PyHive)"

Copy link
Member Author

Choose a reason for hiding this comment

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

I talked with Eugenia and she suggested "Databricks Interactive Cluster" here, and "Databricks SQL Endpoint" for ODBC.

@betodealmeida betodealmeida merged commit 0ea83c5 into apache:master Sep 29, 2021
opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
* feat: add Databricks ODBC engine spec

* Rename Databricks specs
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021
* feat: add Databricks ODBC engine spec

* Rename Databricks specs
@bhavishya123
Copy link

@betodealmeida This feature is not available in latest superset pypi package - apache-superset 1.3.2
when it will be released ?

@betodealmeida
Copy link
Member Author

@betodealmeida This feature is not available in latest superset pypi package - apache-superset 1.3.2 when it will be released ?

@eschutho is currently working on getting 1.4.0 out, which should include this engine spec. Unfortunately we just found out a bug with 1.4.0rc3 and we're going to have to do an RC4, so it should take another couple of weeks.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.4.0 labels Mar 13, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 1.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants