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: use a new official CH driver: clickhouse-connect #22039

Merged
merged 7 commits into from
Nov 16, 2022
Merged

feat: use a new official CH driver: clickhouse-connect #22039

merged 7 commits into from
Nov 16, 2022

Conversation

EugeneTorap
Copy link
Contributor

Use official clickhouse-connect driver instead of clickhouse-sqlalchemy

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

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

@codecov
Copy link

codecov bot commented Nov 4, 2022

Codecov Report

Merging #22039 (b47f5fb) into master (c3f1873) will decrease coverage by 11.26%.
The diff coverage is n/a.

❗ Current head b47f5fb differs from pull request most recent head 40ac9f4. Consider uploading reports for the commit 40ac9f4 to get more accurate results

@@             Coverage Diff             @@
##           master   #22039       +/-   ##
===========================================
- Coverage   66.94%   55.67%   -11.27%     
===========================================
  Files        1831     1831               
  Lines       69833    69833               
  Branches     7570     7570               
===========================================
- Hits        46749    38880     -7869     
- Misses      21126    28995     +7869     
  Partials     1958     1958               
Flag Coverage Δ
hive 52.56% <ø> (ø)
postgres ?
presto 52.45% <ø> (ø)
python 57.75% <ø> (-23.58%) ⬇️
sqlite ?
unit 50.83% <ø> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
superset/utils/dashboard_import_export.py 0.00% <0.00%> (-100.00%) ⬇️
superset/tags/core.py 4.54% <0.00%> (-95.46%) ⬇️
superset/key_value/commands/update.py 0.00% <0.00%> (-90.91%) ⬇️
superset/key_value/commands/delete.py 0.00% <0.00%> (-87.88%) ⬇️
superset/key_value/commands/delete_expired.py 0.00% <0.00%> (-84.00%) ⬇️
superset/dashboards/commands/importers/v0.py 15.62% <0.00%> (-76.25%) ⬇️
superset/datasets/commands/create.py 30.61% <0.00%> (-69.39%) ⬇️
superset/datasets/commands/update.py 25.00% <0.00%> (-69.05%) ⬇️
superset/datasets/commands/importers/v0.py 24.03% <0.00%> (-69.00%) ⬇️
superset/reports/commands/execute.py 23.94% <0.00%> (-68.67%) ⬇️
... and 281 more

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

@EugeneTorap EugeneTorap changed the title Use a new official CH driver: clickhouse-connect feat: use a new official CH driver: clickhouse-connect Nov 4, 2022
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.

Excited about this development, thanks for the PR 👍 One question about upgrading to 0.4.0 (it worked well locally for me). Also, could we migrate the legacy tests from tests/integration_tests/db_engine_specs/clickhouse_tests.py to tests/unit_tests/db_engine_specs/test_clickhouse.py and make sure all relevant code paths are covered? If you need help please ping me on Slack.

setup.py Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/L and removed size/S labels Nov 10, 2022
@EugeneTorap
Copy link
Contributor Author

Waiting for the 0.4.1 version of clickhouse-connect

@EugeneTorap
Copy link
Contributor Author

@villebro @genzgd How to fix the next error?

test_clickhouse.py:42: error: Unexpected keyword argument "message" for "NewConnectionError"

@genzgd
Copy link
Contributor

genzgd commented Nov 10, 2022

I think tests are little tricky because clickhouse-connect provides its own EngineSpec class that is not part of the superset package or code base. As far as I know this is the only EngineSpec that works this way, and it uses the Python "entry point" mechanism to do that.

The result is the class to test is clickhouse_connect.cc_superset.engine.ClickHouseEngineSpec instead of superset.db_engine_specs.clickhouse -- superset.db_engine_specs.clickhouse is coupled with the clickhouse-sqlachemy library.

@genzgd
Copy link
Contributor

genzgd commented Nov 14, 2022

ClickHouse Connect 0.4.1 has been released on PyPI.

@EugeneTorap
Copy link
Contributor Author

ClickHouse Connect 0.4.1 has been released on PyPI.

Thank you so much!

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.

LGTM and tested to work nicely. Looking forward to using this one in the future! 👍

@betodealmeida betodealmeida merged commit 38a3fbd into apache:master Nov 16, 2022
@EugeneTorap EugeneTorap deleted the feat/use-official-ch-driver branch November 16, 2022 18:56
@mistercrunch 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
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 size/L 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants