feat(dbview): Add token request button to DuckDB and MotherDuck database modal #27908

guenp · 2024-04-05T00:04:07Z

SUMMARY

Add a DuckDBParametersSchema to break out the DuckDB connection URI into database, access token (for MotherDuck) and query
Add a button to go to MotherDuck and copy an access token to paste directly into Superset.

The latter is implemented by adding support for the metadata description and load_default values from the parameters schema fields.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

TESTING INSTRUCTIONS

See animated GIF.

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

…turns any

betodealmeida

This is really nice, I love how easy it makes to connect to MotherDuck!

I left a few comments... can we also add some unit tests for build_sqlalchemy_uri, get_parameters_from_uri, and for the serialization/deserialization of DuckDBURI?

superset/databases/utils.py

superset/db_engine_specs/duckdb.py

betodealmeida · 2024-04-05T00:18:58Z

superset/db_engine_specs/duckdb.py

+    @classmethod
+    def parameters_json_schema(cls) -> Any:
+        """
+        Return configuration parameters as OpenAPI.
+        """
+        if not cls.parameters_schema:
+            return None
+
+        spec = APISpec(
+            title="Database Parameters",
+            version="1.0.0",
+            openapi_version="3.0.2",
+            plugins=[MarshmallowPlugin()],
+        )
+        spec.components.schema(cls.__name__, schema=cls.parameters_schema)
+        return spec.to_dict()["components"]["schemas"][cls.__name__]


Note to self, we should not require this, it's the same for every database.

superset/db_engine_specs/duckdb.py

michael-s-molina · 2024-04-05T16:30:26Z

@guenp @betodealmeida What's our policy for this screen regarding specific database configs? I thought this was a generic database interface given that it would be really difficult to maintain this modal if we add UI-specific elements for each database type which seems the case here. Are we planning to support specific versions of this modal for each database type?

betodealmeida · 2024-04-05T17:02:43Z

@guenp @betodealmeida What's our policy for this screen regarding specific database configs? I thought this was a generic database interface given that it would be really difficult to maintain this modal if we add UI-specific elements for each database type which seems the case here. Are we planning to support specific versions of this modal for each database type?

@michael-s-molina the frontend components are mostly generic, and the configuration lives in the DB engine spec (eg, https://github.com/apache/superset/pull/27908/files#diff-651962bab25f2e142c7d1701b15bed4e3c055374d855f1340b85724b573966b7R116-R129). In this PR they're adding some optional parameters to show the URL to get the token, which is useful in general, IMHO.

That being said, I actually think that the frontend components should be more specific. I think we should have a MotherDuckDatabaseConfig component, a BigQueryDatabaseConfig, and so on. The current approach of building a UI based on generic components that are shared between databases doesn't scale, and as you mentioned, and the databases will start stepping on each other toes very quickly.

michael-s-molina · 2024-04-05T17:09:08Z

That being said, I actually think that the frontend components should be more specific. I think we should have a MotherDuckDatabaseConfig component, a BigQueryDatabaseConfig, and so on. The current approach of building a UI based on generic components that are shared between databases doesn't scale, and as you mentioned, and the databases will start stepping on each other toes very quickly.

That was exactly my thinking. It's not a blocker for this PR but something we should consider given the flexibility we'll gain. It's similar to database engine specs where you have common elements but can also extend them to include specific things that will improve overall UX for users.

Co-authored-by: Beto Dealmeida <[email protected]>

betodealmeida

Awesome!

…ase modal (apache#27908) Co-authored-by: Beto Dealmeida <[email protected]>

guenp added 8 commits April 3, 2024 09:57

Add database and motherduck_token fields

cf18d92

add a button to get token

f6f0d06

add MDURI class, use instead of URL class

7a00c51

sqlalchemy uri should be str, database not required, make_url_safe re…

a752454

…turns any

indentation

f3cd6cf

remove unused code

9c86708

move hard-coded url to duckdb engine spec

9b77d30

dynamically set link text from parameters via description

6c53983

pull-request-size bot added the size/L label Apr 5, 2024

betodealmeida reviewed Apr 5, 2024

View reviewed changes

guenp and others added 7 commits April 9, 2024 13:20

Update superset/databases/utils.py

05cd528

Co-authored-by: Beto Dealmeida <[email protected]>

Update superset/db_engine_specs/duckdb.py

d68f049

Co-authored-by: Beto Dealmeida <[email protected]>

refactor sqlalchemy uri build and remove unused code

0542c58

formatting

3763eef

set token to empty string by default

403111c

throw error if no token is provided for motherduck engine

8829806

formatting

2663e61

guenp requested a review from betodealmeida April 11, 2024 16:52

guenp added 2 commits April 12, 2024 01:51

add UTs, return :memory: as default db for duckdb engine

6430dd2

use the Superset default button instead of bare antd version

9eebe83

guenp force-pushed the guenp/add-token-request-button branch from 24b4c93 to 9eebe83 Compare April 12, 2024 08:52

guenp added 2 commits April 12, 2024 02:18

set parameters to empty dict if None

31736a3

add docstrings

836cd7f

betodealmeida approved these changes Apr 15, 2024

View reviewed changes

betodealmeida merged commit 08aaebb into apache:master Apr 15, 2024
29 checks passed

EnxDev pushed a commit to EnxDev/superset that referenced this pull request Apr 15, 2024

feat(dbview): Add token request button to DuckDB and MotherDuck datab…

6a34912

…ase modal (apache#27908) Co-authored-by: Beto Dealmeida <[email protected]>

guenp deleted the guenp/add-token-request-button branch April 15, 2024 17:16

qleroy pushed a commit to qleroy/superset that referenced this pull request Apr 28, 2024

feat(dbview): Add token request button to DuckDB and MotherDuck datab…

831c376

…ase modal (apache#27908) Co-authored-by: Beto Dealmeida <[email protected]>

jzhao62 pushed a commit to jzhao62/superset that referenced this pull request May 16, 2024

feat(dbview): Add token request button to DuckDB and MotherDuck datab…

71a8370

…ase modal (apache#27908) Co-authored-by: Beto Dealmeida <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dbview): Add token request button to DuckDB and MotherDuck database modal #27908

feat(dbview): Add token request button to DuckDB and MotherDuck database modal #27908

guenp commented Apr 5, 2024

betodealmeida left a comment

betodealmeida Apr 5, 2024

michael-s-molina commented Apr 5, 2024

betodealmeida commented Apr 5, 2024

michael-s-molina commented Apr 5, 2024

betodealmeida left a comment

feat(dbview): Add token request button to DuckDB and MotherDuck database modal #27908

feat(dbview): Add token request button to DuckDB and MotherDuck database modal #27908

Conversation

guenp commented Apr 5, 2024

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

betodealmeida left a comment

Choose a reason for hiding this comment

betodealmeida Apr 5, 2024

Choose a reason for hiding this comment

michael-s-molina commented Apr 5, 2024

betodealmeida commented Apr 5, 2024

michael-s-molina commented Apr 5, 2024

betodealmeida left a comment

Choose a reason for hiding this comment