-
Notifications
You must be signed in to change notification settings - Fork 388
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
poc: add databases in datasource #3735
base: main
Are you sure you want to change the base?
poc: add databases in datasource #3735
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
b77f09d
to
157c778
Compare
@Light2Dark, a user on Discord had 100+ tables. We don't need to lazy load tables too just yet, but could be worth keeping a lookout / design in such a way to support it later. I do think by avoiding loading the columns its only 2 round trips (instead of a fan-out). |
frontend/src/components/editor/chrome/panels/datasources-panel.tsx
Outdated
Show resolved
Hide resolved
@@ -15,3 +21,13 @@ export const FUNCTIONS_REGISTRY = new DeferredRequestRegistry< | |||
...req, | |||
}); | |||
}); | |||
|
|||
export const PreviewSQLTable = new DeferredRequestRegistry< |
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.
let me know if its worth wrapping DeferredRequestRegistry
in an LRU cache. happy to write a class for that if you'd want
@@ -472,6 +488,38 @@ components: | |||
- time | |||
- unknown | |||
type: string | |||
Database: |
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.
adding the new objects to marimo/_cli/development/commands.py
should result in a cleaner yaml file
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.
should be kinda clean already, the objects show up nicely 🤔
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.
oh shoot, sorry i misread this
hey @mscolnick, the frontend seems a little tricky. Is there a reason you're putting the following state in jotai? Instead of local to a component function initialState(): DatasetsState {
return {
tables: [],
expandedTables: new Set(),
expandedColumns: new Set(),
columnsPreviews: new Map(),
};
} referring to the |
@Light2Dark, jotai was originally used because request and responses came from different places. if you want to |
📝 Summary
TODOs:
datasources-panel.tsx
needs work, quite tricky :/🔍 Description of Changes
Experimenting a little, I find an optimal request solution is something like below. Make an initial req for db, schemas and table list, upon expanding a table, make another request to get column info, PKs, indexes. This avoids too long of an initial request time while fetching sufficient info. With snowflake sample db (7 schemas, 10+ tables each), initial request takes about 3s. If I fetched all data, this would take ~25s
📋 Checklist
📜 Reviewers
@akshayka OR @mscolnick