-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature/add bson column type #8
base: main
Are you sure you want to change the base?
Conversation
Renamed mysql to singlestore in singlestore-core
…dexes on columnstore
Feature/adapt drizzle kit to singlestore
1072d7e
to
1992b82
Compare
1992b82
to
25a56f9
Compare
@@ -42,6 +42,7 @@ export class SingleStoreJson<T extends ColumnBaseConfig<'json', 'SingleStoreJson | |||
} | |||
|
|||
override mapToDriverValue(value: T['data']): string { | |||
console.log('value', value); |
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.
nit: rm log
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.
Nice catch 👍
@@ -177,6 +177,7 @@ | |||
"@vercel/postgres": "^0.8.0", | |||
"@xata.io/client": "^0.29.3", | |||
"better-sqlite3": "^8.4.0", | |||
"bson": "^6.8.0", |
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.
I'm curious about how we want to represent the data here. Do we expect users to be dealing with BSON data in a manner like this, or are we expecting them to convert it to JSON anyways? Especially considering the to-driver stuff is using JSON.stringify
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.
From my perspective, the ideal scenario is that users deal only with JSON strings and let the form deal with the translation. That would make things more easy for users.
Something like:
db.singlestoreTable({
...
bson_column: bson().notnull().default({"key": "value"})
...
})
Currently there is a bug on the translation of literals into BSON on the engine side, so this will only be possible once the bug on the engine is fixed. For now the default value has to be converted to BSON object and then to hexa string
More info about the bug on slack: https://memsql.slack.com/archives/C02G51BAJ/p1724069568691219?thread_ts=1724061645.083329&cid=C02G51BAJ
I also found out that you can cast json literals on select and insert statements, but not on create table statements. Because of that, we can keep the jsonString:>BSON
on the mapToDriver
return value, since the drizzle-orm package only deals with insert and select operations, and the option above on drizzle-kit, that deals with scheam modifications (create table and create/alter column)
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.
Yeah, that all makes sense from the engine side. I guess I was more curious about the JS side of when we get the data out from the engine. Do we expect users to use BSON directly in TypeScript or should we also convert it back to JSON?
reminder to add back the removed BSON column type from #11 to this PR |
Support bson column type for
Includes a set of tests for singlestore and a green pipeline is expected
Note: When defining the default value of a bson type column during table creation, the default value of the column is converted to hexa code to be accepted by the engine.
The
CREATE TABLE
statement generated by drizzle-kit for the default value of {"key":"value"} will beThe SingleStore team is working to accept json literals for bson columns default values.
This would work in the future
When the json literals are supported we can simplify the code and remove the BSON external package.