Skip to content
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: duckdb-wasm query UI [DRAFT] #39

Draft
wants to merge 19 commits into
base: main
Choose a base branch
from
Draft

Conversation

mattrothenberg
Copy link
Contributor

@mattrothenberg mattrothenberg commented May 12, 2022

This PR is a rough pass at #38, providing an interface for querying the current dataset via duckdb-wasm. cc @LoneRifle, I welcome any and all thoughts you may have here.

Screen Shot 2022-05-12 at 2 39 40 PM

Todos

  • Validate whether this "query as you type" UI/UX is helpful or awful. I kind of like that there's no discrete Run Query button
  • Handle transient DB connection errors that seem to happen when switching tabs
  • Figure out a UI/UX for toggling between the flat-ui component and the query interface
  • Hook up the flat-ui component to the results section of the query interface so that we can better visualize results
  • Make the code editor better (more robust column name and type detection)

@mattrothenberg mattrothenberg requested review from idan and jaked May 12, 2022 17:08
@mattrothenberg mattrothenberg self-assigned this May 12, 2022
@mattrothenberg mattrothenberg changed the title feat: duckdb-wasm query UI feat: duckdb-wasm query UI [DRAFT] May 12, 2022
@mattrothenberg mattrothenberg marked this pull request as draft May 12, 2022 17:08
src/api/index.ts Outdated Show resolved Hide resolved
Copy link

@jaked jaked left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really cool!

I don't totally get why it's necessary to include all the duckdb code explicitly in public. I wonder if there is a way to pass through the contents of a package somehow in the build (so use the code in the duckdb package dep instead of duplicating it explicitly); access it via the built code instead of via HTTP (probably hard to do with WASM); or maybe to point to the code on a CDN instead.

I get that this a spike, none of this is criticism, just curiosity.

@mattrothenberg
Copy link
Contributor Author

mattrothenberg commented May 12, 2022

@jaked Great questions! I can answer a few of these for you.

There are in fact docs for installing the library in a vite environment and they simply did not work for me. I ran into weird, esoteric issues that you can reproduce by following the steps in the linked README.

As for the CDN approach, this is one option. The drawback is that jsdelivr doesn't seem to like connections from localhost:3000, so I couldn't load the bundles that way.

EDIT: this is fixed in 99055d6. Turns out we were using an outdated version of vite which didn't seem to like the URL imports.

Copy link
Contributor

@LoneRifle LoneRifle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattrothenberg, thanks for giving me the chance to look at this first cut of the feature, it looks really promising!

Query as you type is very helpful, and the debounce seems to be tuned just nice.

Couple of specific things that I'll comment in the appropriate file locations

src/components/repo-detail.tsx Outdated Show resolved Hide resolved
src/components/db-explorer.tsx Outdated Show resolved Hide resolved
@LoneRifle
Copy link
Contributor

cc @ankoh of @duckdb in case the DuckDB team are interested in tracking this feature

@ankoh
Copy link

ankoh commented May 16, 2022

@jaked Great questions! I can answer a few of these for you.

There are in fact docs for installing the library in a vite environment and they simply did not work for me. I ran into weird, esoteric issues that you can reproduce by following the steps in the linked README.

As for the CDN approach, this is one option. The drawback is that jsdelivr doesn't seem to like connections from localhost:3000, so I couldn't load the bundles that way.

Maybe I can help here.
DuckDB-Wasm only requires to treat our WebAssembly modules as plain binary assets.
Bundling from npm should be more convenient for future updates.

This PR is exciting! 🚀

@mattrothenberg
Copy link
Contributor Author

This is wonderful feedback @LoneRifle and @ankoh, thank you both.

Stay tuned as we work through these updates 😎

const execQuery = async (query: string) => {
if (!connectionRef.current) return;
const queryRes = await connectionRef.current.query(query);
const asArray = queryRes.toArray();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to convert the result to an array. Arrow tables already behave like arrays of objects if you iterate over them for for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@domoritz Fantastic! This felt kludgy and I suspected there was a better way.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

map and forEach won't work so you may have to refactor your code a bit but it would be worth it since you avoid a lot of upfront conversion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@domoritz Good call. We're ultimately passing this data down to our flat-ui component , so it needs to be in a shape that this component understands (or we need to refactor the component)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may need to refactor the component. Shouldn't be a big deal, though, and would be great for interoperability anyway.

src/components/db-explorer.tsx Show resolved Hide resolved
src/components/db-explorer.tsx Outdated Show resolved Hide resolved
@domoritz
Copy link

Looks very exciting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants