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

Feature/add bson column type #8

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

Conversation

Rodriguespn
Copy link
Collaborator

@Rodriguespn Rodriguespn commented Aug 19, 2024

Support bson column type for

  • drizzle-kit
  • drizzle-orm
  • drizzle-typebox
  • drizzle-valibot

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 TABLEstatement generated by drizzle-kit for the default value of {"key":"value"} will be

CREATE TABLE `table` (
        `bson` bson DEFAULT 0x14000000026b6579000600000076616c75650000 /* hexa code for {"key":"value"} */
);

The SingleStore team is working to accept json literals for bson columns default values.

This would work in the future

CREATE TABLE `table` (
        `bson` bson DEFAULT '{"key":"value"}'
);

When the json literals are supported we can simplify the code and remove the BSON external package.

@Rodriguespn Rodriguespn self-assigned this Aug 19, 2024
@Rodriguespn Rodriguespn added the enhancement New feature or request label Aug 19, 2024
@Rodriguespn Rodriguespn marked this pull request as ready for review August 19, 2024 12:26
@Rodriguespn Rodriguespn force-pushed the feature/add-bson-column-type branch 3 times, most recently from 1072d7e to 1992b82 Compare August 19, 2024 13:12
@Rodriguespn Rodriguespn marked this pull request as draft August 19, 2024 14:54
@@ -42,6 +42,7 @@ export class SingleStoreJson<T extends ColumnBaseConfig<'json', 'SingleStoreJson
}

override mapToDriverValue(value: T['data']): string {
console.log('value', value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: rm log

Copy link
Collaborator Author

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",
Copy link
Collaborator

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

Copy link
Collaborator Author

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)

Copy link
Collaborator

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?

@mitchwadair
Copy link
Collaborator

reminder to add back the removed BSON column type from #11 to this PR

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

Successfully merging this pull request may close these issues.

4 participants