-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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: add YDB as a new database engine #31141
Conversation
4389a74
to
7ab18c0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #31141 +/- ##
===========================================
+ Coverage 60.48% 83.81% +23.33%
===========================================
Files 1931 537 -1394
Lines 76236 38980 -37256
Branches 8568 0 -8568
===========================================
- Hits 46114 32673 -13441
+ Misses 28017 6307 -21710
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
7ab18c0
to
8c5ba7f
Compare
8c5ba7f
to
129462a
Compare
Also the logic in your spec should have some unit tests. |
@giftig No ephemeral environment action detected. Please use '/testenv up' or '/testenv down'. View workflow run. |
/testenv up |
Can you also provide some test instructions for how to verify this is working / describe what you tested, please? I'm assuming it's mostly just install the ydb driver, connect a ydb database, and try running some queries and building some dashboards, but maybe others will catch some edge case testing which may be useful with a new engine spec. |
@giftig Ephemeral environment spinning up at http://18.237.77.159:8080. Credentials are |
4b28fb7
to
a414bb7
Compare
98f26cf
to
aa9b089
Compare
aa9b089
to
4367dd6
Compare
4367dd6
to
ffb2309
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments / questions but looks good to me.
I haven't had chance to try running your branch with YDB though, and we'll probably need to check if there's a process for including that ydb.svg
, so we should wait for some more reviews.
connect_args["protocol"] = encrypted_extra["protocol"] | ||
|
||
if "credentials" in encrypted_extra: | ||
credentials_info = encrypted_extra["credentials"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This argument seems unnecessary, could just be one line like L100
SUMMARY
This PR provides YDB database support based on YDB Dialect
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Steps needed to install sqlalchemy dialect described on databases.mdx.
To test it you can start local YDB via docker-compose:
sqlalchemy uri will be like
ADDITIONAL INFORMATION