-
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more.
superset/superset-frontend/src/utils/textUtils.ts Lines 18 to 25 in ec9e9a4
|
||||||||||||||||||
fallbackDocsUrl = | ||||||||||||||||||
SupersetText.DB_MODAL_SQLALCHEMY_FORM?.SQLALCHEMY_DOCS_URL; | ||||||||||||||||||
fallbackDisplayText = | ||||||||||||||||||
SupersetText.DB_MODAL_SQLALCHEMY_FORM?.SQLALCHEMY_DISPLAY_TEXT; | ||||||||||||||||||
} else { | ||||||||||||||||||
fallbackDocsUrl = 'https://docs.sqlalchemy.org/en/13/core/engines.html'; | ||||||||||||||||||
fallbackDisplayText = 'SQLAlchemy docs'; | ||||||||||||||||||
} | ||||||||||||||||||
const fallbackDocsUrl = | ||||||||||||||||||
SupersetText?.DB_MODAL_SQLALCHEMY_FORM?.SQLALCHEMY_DOCS_URL || | ||||||||||||||||||
'https://docs.sqlalchemy.org/en/13/core/engines.html'; | ||||||||||||||||||
const fallbackDisplayText = | ||||||||||||||||||
SupersetText?.DB_MODAL_SQLALCHEMY_FORM?.SQLALCHEMY_DISPLAY_TEXT || | ||||||||||||||||||
'SQLAlchemy docs'; | ||||||||||||||||||
|
||||||||||||||||||
return ( | ||||||||||||||||||
<> | ||||||||||||||||||
<StyledInputContainer> | ||||||||||||||||||
|
@@ -82,9 +78,10 @@ const SqlAlchemyTab = ({ | |||||||||||||||||
data-test="sqlalchemy-uri-input" | ||||||||||||||||||
value={db?.sqlalchemy_uri || ''} | ||||||||||||||||||
autoComplete="off" | ||||||||||||||||||
placeholder={t( | ||||||||||||||||||
'dialect+driver://username:password@host:port/database', | ||||||||||||||||||
)} | ||||||||||||||||||
placeholder={ | ||||||||||||||||||
db?.sqlalchemy_uri_placeholder || | ||||||||||||||||||
t('dialect+driver://username:password@host:port/database') | ||||||||||||||||||
} | ||||||||||||||||||
onChange={onInputChange} | ||||||||||||||||||
/> | ||||||||||||||||||
</div> | ||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -185,6 +185,12 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods | |
engine_aliases: set[str] = set() | ||
drivers: dict[str, str] = {} | ||
default_driver: str | None = None | ||
|
||
# placeholder with the SQLAlchemy URI template | ||
sqlalchemy_uri_placeholder = ( | ||
"engine+driver://user:password@host:port/dbname[?key=value&key=value...]" | ||
) | ||
|
||
disable_ssh_tunneling = False | ||
|
||
_date_trunc_functions: dict[str, str] = {} | ||
|
@@ -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 commentThe 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. |
||
"engine+driver://user:password@host:port/dbname[?key=value&key=value...]" | ||
) | ||
|
||
# query parameter to enable encryption in the database connection | ||
# for Postgres this would be `{"sslmode": "verify-ca"}`, eg. | ||
encryption_parameters: dict[str, str] = {} | ||
|
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 and0.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.