-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add DuckDB Connection #46
Conversation
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.
Couple of comments, overall this is looking great! Excited to have DuckDB support 🫡
src/connections/motherduck.ts
Outdated
duckDB: duckDB.Database | undefined | ||
|
||
constructor( | ||
path: string, |
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.
Currently in other connections we have the constructor just take an object instead of multiple parameters. This allows any connection to have a predictable pattern. I'd say we should probably lean in that direction here as well unless we have a good reason not to and then I'd remain open-minded.
src/connections/motherduck.ts
Outdated
return { | ||
data: result, | ||
error: null, | ||
query: query.query, |
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.
This response is intended to be the absolute raw query so users can debug what was executed against their database, so change to:
const rawSQL = constructRawQuery(query)
// ...
return {
data: result,
error: null,
query: rawSQL
}
src/connections/motherduck.ts
Outdated
return { | ||
data: null, | ||
error: error, | ||
query: query.query, |
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.
Same as above
Purpose
This PR is meant to add support for the DuckDB connection.
https://duckdb.org/docs/api/nodejs/overview.html
Using Just a Query
Using Parameters
Tasks
Verify
Before
After