-
Notifications
You must be signed in to change notification settings - Fork 19
SQL literal #176
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
base: main
Are you sure you want to change the base?
SQL literal #176
Changes from all commits
53fd778
9ef2c4c
e3b9737
0fd9c7a
08241ee
831ed61
284e50e
b35a8bd
75ad2a0
21c2bb1
942fd75
e041ee4
4a23f3d
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -6,7 +6,7 @@ import {hash, nameHash} from "../../lib/hash.js"; | |||||
| export type QueryParam = any; | ||||||
|
|
||||||
| /** @see https://observablehq.com/@observablehq/database-client-specification#%C2%A71 */ | ||||||
| export type QueryResult = Record<string, any>[] & {schema: ColumnSchema[]; date: Date}; | ||||||
| export type QueryResult<T = Record<string, any>> = T[] & {schema: ColumnSchema[]; date: Date}; | ||||||
|
|
||||||
| /** @see https://observablehq.com/@observablehq/database-client-specification#%C2%A72.2 */ | ||||||
| export interface ColumnSchema { | ||||||
|
|
@@ -39,10 +39,24 @@ export interface QueryOptions extends QueryOptionsSpec { | |||||
| since?: Date; | ||||||
| } | ||||||
|
|
||||||
| export type SqlDialect = | ||||||
| | "bigquery" | ||||||
| | "databricks" | ||||||
| | "duckdb" | ||||||
| | "mongosql" | ||||||
| | "mssql" | ||||||
| | "mysql" | ||||||
| | "oracle" | ||||||
| | "postgres" | ||||||
| | "snowflake" | ||||||
| | "sql" | ||||||
| | "sqlite"; | ||||||
|
|
||||||
| export interface DatabaseClient { | ||||||
| readonly name: string; | ||||||
| readonly options: QueryOptions; | ||||||
| sql(strings: readonly string[], ...params: QueryParam[]): Promise<QueryResult>; | ||||||
| readonly dialect?: SqlDialect; | ||||||
|
Member
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. TODO DatabaseClientImpl constructor needs to take a dialect.
Contributor
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. I've tried to implement this (together with binding to a database) in #177. |
||||||
| sql<T = Record<string, any>>(strings: readonly string[], ...params: QueryParam[]): Promise<QueryResult<T>>; // prettier-ignore | ||||||
| } | ||||||
|
|
||||||
| export const DatabaseClient = (name: string, options?: QueryOptionsSpec): DatabaseClient => { | ||||||
|
|
@@ -65,11 +79,11 @@ class DatabaseClientImpl implements DatabaseClient { | |||||
| options: {value: options, enumerable: true} | ||||||
| }); | ||||||
| } | ||||||
| async sql(strings: readonly string[], ...params: QueryParam[]): Promise<QueryResult> { | ||||||
| async sql<T = Record<string, any>>(strings: readonly string[], ...params: QueryParam[]): Promise<QueryResult<T>> { | ||||||
| const path = await this.cachePath(strings, ...params); | ||||||
| const response = await fetch(path); | ||||||
|
Member
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. This’ll need to be something like… const [Arrow, response] = await Promise.all([import("https://cdn.jsdelivr.net/npm/apache-arrow@17.0.0/+esm"), fetch(path)]); // prettier-ignore
if (!response.ok) throw new Error(`failed to fetch: ${path}`);
return Arrow.tableFromIPC(response); |
||||||
| if (!response.ok) throw new Error(`failed to fetch: ${path}`); | ||||||
| return await response.json().then(revive); | ||||||
| return (await response.json().then(revive)) as QueryResult<T>; | ||||||
| } | ||||||
| async cachePath(strings: readonly string[], ...params: QueryParam[]): Promise<string> { | ||||||
| return `.observable/cache/${await nameHash(this.name)}-${await hash(strings, ...params)}.json`; | ||||||
|
Member
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.
Suggested change
Contributor
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. btw should we Promise.all these two awaits? |
||||||
|
|
||||||
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.
Do we want to remove the date from here?
Also this type does not cover a resultset that would come back as an arrow table. Are we going to convert to arrow later, from the array of objects?
An idea could be to add a
format="arrow" | "json"property on sql cells, that would default to JSON (the current format); if you pass"arrow"a database handler that has this capability creates arrow immediately; any other gets converted to arrow.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 should go away entirely to be replaced with
Arrow.Table.