feat: Add support for Azure Data Explorer (Kusto) db engine spec #17898

Ceridan · 2021-12-30T11:40:37Z

SUMMARY

Add support for Azure Data Explorer (Kusto). My team at @dodopizza has built and maintained the Kusto dialect for SQLAlchemy. It allows us to add a new Kusto engine spec.

Kusto can handle two dialects: SQL and KQL. Currently, we provide full support for the SQL dialect (SELECT queries, no DML) and experimental support for the KQL (work in progress). For more details, you may check sqlalchemy-kusto repo.

We are using SQL dialect in production at our company and working on full support for KQL dialect.

TESTING INSTRUCTIONS

To test manually you may connect to the Kusto database and run some queries in the SQL Lab against it.

Additionally, sqlalchemy-kusto includes dialect tests.

ADDITIONAL INFORMATION

Has associated issue: #10646
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 - new database support - Kusto.
Removes existing feature or API

…ode to support Kusto engine specs.

codecov · 2021-12-30T12:33:33Z

Codecov Report

Merging #17898 (4fa2914) into master (8ebec60) will increase coverage by 0.00%.
The diff coverage is 92.00%.

@@           Coverage Diff           @@
##           master   #17898   +/-   ##
=======================================
  Coverage   67.10%   67.10%           
=======================================
  Files        1609     1610    +1     
  Lines       64897    64970   +73     
  Branches     6866     6866           
=======================================
+ Hits        43547    43600   +53     
- Misses      19484    19504   +20     
  Partials     1866     1866

Flag	Coverage Δ
hive	`53.31% <65.33%> (-28.50%)`	⬇️
mysql	`82.21% <92.00%> (+0.02%)`	⬆️
postgres	`82.26% <92.00%> (+0.02%)`	⬆️
presto	`53.15% <65.33%> (-28.96%)`	⬇️
python	`82.71% <92.00%> (-0.03%)`	⬇️
sqlite	`81.95% <92.00%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
superset/db_engine_specs/kusto.py	`91.54% <91.54%> (ø)`
superset/db_engine_specs/base.py	`88.74% <100.00%> (+0.06%)`	⬆️
superset/models/core.py	`89.48% <100.00%> (-0.52%)`	⬇️
superset/views/database/mixins.py	`81.03% <0.00%> (-1.73%)`	⬇️
superset/db_engine_specs/hive.py	`85.71% <0.00%> (-1.55%)`	⬇️
superset/db_engine_specs/presto.py	`89.14% <0.00%> (-1.26%)`	⬇️
superset/connectors/sqla/models.py	`88.31% <0.00%> (-0.12%)`	⬇️

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 8ebec60...4fa2914. Read the comment docs.

villebro

This looks great! One nit + one question. Also, wondering if you would be open to adding some unit tests for typical cases, especially for the overridden class methods, like is_select_query and parse_sql.

superset/db_engine_specs/kustokql.py

superset/db_engine_specs/kustosql.py

Ceridan · 2022-01-04T11:37:28Z

Thank you for the review and suggestion on how to improve this code! We will add unit tests and merge two dialects in a single file, and I will back to you.

Add tests for Kusto db spec

villebro

Looking great - a few last comments, but I think this is really close to being mergeable.

villebro · 2022-01-10T09:01:16Z

setup.py

@@ -140,6 +140,7 @@ def get_git_sha() -> str:
        "hana": ["hdbcli==2.4.162", "sqlalchemy_hana==0.4.0"],
        "hive": ["pyhive[hive]>=0.6.1", "tableschema", "thrift>=0.11.0, <1.0.0"],
        "impala": ["impyla>0.16.2, <0.17"],
+        "kusto": ["sqlalchemy-kusto>=1.0.1"],


Just in case, could we restrict to the current major version?

Suggested change

"kusto": ["sqlalchemy-kusto>=1.0.1"],

"kusto": ["sqlalchemy-kusto>=1.0.1, <2"],

Yes, sounds good to me. We will add this restriction.

villebro · 2022-01-10T09:09:05Z

tests/unit_tests/db_engine_specs/test_kusto.py

+    [
+        ("tbl | limit 100", True),
+        ("let foo = 1; tbl | where bar == foo", True),
+        (".show tables", False),


Again, out of curiosity, is this actually a read-only query? I know that SHOW TABLES, EXPLAIN etc appears to currently be flagged as DML by parse_sql, but as this logic will now be contained in the db engine spec, this can now be easily altered. So maybe we'd like to change this to be flagged as read-only in the KQL spec? Also, could we add a more typical DML case here, like .set, .append or .drop? I assume they're the cases we want to make sure we're catching.

Your remark is valuable. Thank you. We will add more specific code.

villebro · 2022-01-10T09:23:42Z

tests/unit_tests/db_engine_specs/test_kusto.py

+        ("INSERT INTO tbl (foo) VALUES (1)", False),
+    ],
+)
+def test_sql_is_readonly_query(


Could we add a few more tests for the SQL spec, like a few timegrains, convert_dttm etc, just to get coverage up? You can probably copy some template tests from the pre-existing unit tests.

Added tests for convert_dttm.

xneg · 2022-01-10T12:01:08Z

@villebro Thank you for the review! We fixed your comments.

villebro · 2022-01-10T12:08:02Z

@xneg awesome 👍 I started CI and this definitely LGTM if it passes CI. Again, to further protect this connector from the unlikely risk of regressions I'd suggest adding unit tests for all overridden methods (e.g. convert_dttm etc), but I'm fine merging as-is.

xneg · 2022-01-10T12:10:51Z

@villebro Thank you! We already added unit tests for all overridden methods except get_dbapi_exception_mapping.

villebro

LGTM!

villebro · 2022-01-10T12:42:13Z

villebro Thank you! We already added unit tests for all overridden methods except get_dbapi_exception_mapping.

@xneg right you are - my apologies, I see that now! ✅

…che#17898) * Add two Kusto engine specs: KQL and SQL. Some minor changes in core code to support Kusto engine specs. * Remove redundant imports and logging. * docs: Kusto sqlalchemy docs * fix: Fix mypy and linting errors * fix: Handle Black vs Pylint checks * fix: isort problem * refactor: Merge kustosql and kustokql in the single kusto module * test: Add tests for Kusto db spec * feat: Schema override does not require in KQL anymore * Removed redundant imports. * Added ".show" queries to readonly query determination. * Fixed some bugs. Added tests for convert_dttm. * Fixed major sqlalchemy-kusto version. * Fixed by isort. Co-authored-by: Eugene Bikkinin <xnegxneg@gmail.com> Co-authored-by: k.tomak <k.tomak@dodopizza.com> Co-authored-by: Eugene Bikkinin <e.bikkinin@dodopizza.com>

anshulsharmas · 2022-04-20T03:09:30Z

Hi @xneg.. thanks for creating this amazing Kusto plugin.. I couldn't find the documentation for the connector though, I can see you had made commits but not sure why Kusto isn't listed here ? https://superset.apache.org/docs/intro

xneg · 2022-04-20T07:05:40Z

Hi @anshulsharmas, thank you.
I think Kusto isn't listed because this feature will be in v. 1.5.
https://github.com/apache/superset/blob/1.5.0rc1/CHANGELOG.md

xneg and others added 3 commits December 30, 2021 14:32

Add two Kusto engine specs: KQL and SQL. Some minor changes in core c…

04c4354

…ode to support Kusto engine specs.

Remove redundant imports and logging.

c08cf90

docs: Kusto sqlalchemy docs

28f3f64

pull-request-size bot added the size/L label Dec 30, 2021

Ceridan changed the title ~~Add support for Azure Data Explorer (Kusto) db engine spec~~ feat: Add support for Azure Data Explorer (Kusto) db engine spec Dec 30, 2021

Ceridan added 3 commits December 30, 2021 14:51

fix: Fix mypy and linting errors

bef6bea

fix: Handle Black vs Pylint checks

15bee79

fix: isort problem

992f64c

villebro reviewed Jan 4, 2022

View reviewed changes

superset/db_engine_specs/kustokql.py Outdated Show resolved Hide resolved

superset/db_engine_specs/kustosql.py Outdated Show resolved Hide resolved

Ceridan and others added 5 commits January 4, 2022 17:18

refactor: Merge kustosql and kustokql in the single kusto module

ea386d6

test: Add tests for Kusto db spec

235d5ae

feat: Schema override does not require in KQL anymore

8092fac

Merge pull request #2 from Ceridan/add-kusto-engine-support

d663b1b

Add tests for Kusto db spec

Removed redundant imports.

1d37226

villebro reviewed Jan 10, 2022

View reviewed changes

xneg added 4 commits January 10, 2022 14:17

Added ".show" queries to readonly query determination.

e208800

Fixed some bugs.

9a6598b

Added tests for convert_dttm.

Fixed major sqlalchemy-kusto version.

793d504

Fixed by isort.

4fa2914

villebro approved these changes Jan 10, 2022

View reviewed changes

villebro merged commit d2d4f8e into apache:master Jan 10, 2022

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for Azure Data Explorer (Kusto) db engine spec #17898

feat: Add support for Azure Data Explorer (Kusto) db engine spec #17898

Ceridan commented Dec 30, 2021 •

edited

Loading

codecov bot commented Dec 30, 2021 •

edited

Loading

villebro left a comment

Ceridan commented Jan 4, 2022

villebro left a comment

villebro Jan 10, 2022

Ceridan Jan 10, 2022

villebro Jan 10, 2022 •

edited

Loading

xneg Jan 10, 2022

villebro Jan 10, 2022

xneg commented Jan 10, 2022

villebro commented Jan 10, 2022

xneg commented Jan 10, 2022

villebro left a comment

villebro commented Jan 10, 2022

anshulsharmas commented Apr 20, 2022

xneg commented Apr 20, 2022

	"kusto": ["sqlalchemy-kusto>=1.0.1"],
	"kusto": ["sqlalchemy-kusto>=1.0.1, <2"],

feat: Add support for Azure Data Explorer (Kusto) db engine spec #17898

feat: Add support for Azure Data Explorer (Kusto) db engine spec #17898

Conversation

Ceridan commented Dec 30, 2021 • edited Loading

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Dec 30, 2021 • edited Loading

Codecov Report

villebro left a comment

Choose a reason for hiding this comment

Ceridan commented Jan 4, 2022

villebro left a comment

Choose a reason for hiding this comment

villebro Jan 10, 2022

Choose a reason for hiding this comment

Ceridan Jan 10, 2022

Choose a reason for hiding this comment

villebro Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

xneg Jan 10, 2022

Choose a reason for hiding this comment

villebro Jan 10, 2022

Choose a reason for hiding this comment

xneg commented Jan 10, 2022

villebro commented Jan 10, 2022

xneg commented Jan 10, 2022

villebro left a comment

Choose a reason for hiding this comment

villebro commented Jan 10, 2022

anshulsharmas commented Apr 20, 2022

xneg commented Apr 20, 2022

Ceridan commented Dec 30, 2021 •

edited

Loading

codecov bot commented Dec 30, 2021 •

edited

Loading

villebro Jan 10, 2022 •

edited

Loading