-
Notifications
You must be signed in to change notification settings - Fork 14k
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 MotherDuck DB engine spec #24934
Conversation
@@ -38,17 +38,13 @@ const SqlAlchemyTab = ({ | |||
testInProgress?: boolean; | |||
children?: ReactNode; | |||
}) => { | |||
let fallbackDocsUrl; | |||
let fallbackDisplayText; | |||
if (SupersetText) { |
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.
SupersetText
is either a module or {}
, and both are truthy.
superset/superset-frontend/src/utils/textUtils.ts
Lines 18 to 25 in ec9e9a4
const loadModule = () => { | |
try { | |
// eslint-disable-next-line global-require, import/no-unresolved | |
return require('../../../superset_text') || {}; | |
} catch (e) { | |
return {}; | |
} | |
}; |
@@ -1958,11 +1964,6 @@ class BasicParametersMixin: | |||
# recommended driver name for the DB engine spec | |||
default_driver = "" | |||
|
|||
# placeholder with the SQLAlchemy URI template | |||
sqlalchemy_uri_placeholder = ( |
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.
Moving this from the "dynamic form" mixin to the base DB engine spec.
Codecov Report
@@ Coverage Diff @@
## master #24934 +/- ##
=======================================
Coverage 69.00% 69.00%
=======================================
Files 1906 1906
Lines 74133 74149 +16
Branches 8208 8211 +3
=======================================
+ Hits 51153 51169 +16
+ Misses 20857 20856 -1
- Partials 2123 2124 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
setup.py
Outdated
@@ -150,6 +150,7 @@ def get_git_sha() -> str: | |||
"dremio": ["sqlalchemy-dremio>=1.1.5, <1.3"], | |||
"drill": ["sqlalchemy-drill==0.1.dev"], | |||
"druid": ["pydruid>=0.6.5,<0.7"], | |||
"duckdb": ["duckdb-engine==0.8.0"], |
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.
@betodealmeida should this be >=0.8.0,<1.0.0
?
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.
@john-bodley could be, but in general I prefer to pin 0.x
versions instead of using a range, since in theory the API is not sable and 0.8.1
could introduce breaking changes.
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.
On that note, 0.8.1 is out, and seems to follow semver
https://github.com/duckdb/duckdb/releases/
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.
I could go either way on this... trying to rationalize a preference.
If we pin the version, and there's an issue (potentially security) then we might have a fire drill on our hands.
If we use a range, and something breaks, we might have a bug on our hands.
By the "lesser evil" metric, I think this might put the range idea ahead in my mind by about the width of a frog hair, as long as they stick to semver ;)
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.
Yeah, I know they were planning to change how databases are returned by their dialect, so maybe we keep this frozen until they hit 1.0?
Also, this is an optional dependency, people can always install a newer version if they want.
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.
Not sure if we want to pin or not... and not sure if we want to bump the pinned version or not. But neither issue will keep me up at night and the rest looks good to me!
SUMMARY
Add a separate DB engine spec for MotherDuck, a cloud version of DuckDB. While adding the spec I did a few QoL improvements:
sqlalchemy_uri_placeholder
as the placeholder for the SQLAlchemy URI.duckdb
dependency to Superset, with the library needed to run DuckDB and connect to MotherDuck.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before, using the DuckDB driver, note that we show the generic placeholder and that the text is missing the link to the docs:
After, we show the correct placeholder, and the text is fixed:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION