-
Notifications
You must be signed in to change notification settings - Fork 334
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: Create embeddings for meeting templates #9776
feat: Create embeddings for meeting templates #9776
Conversation
This one should wait until I verified or implemented the logic to update the |
…tingTemplateEmbeddings
await client.query(` | ||
DO $$ | ||
BEGIN | ||
DELETE FROM "EmbeddingsMetadata" WHERE "objectType" = 'meetingTemplate'; |
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.
+1 would using kysely work here instead?
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 think it would. I'm not sure what I would gain in that case. I guess typesafety after the up migration was run, but that seems a bit odd 🤔
Any particular reason we should use kysely in migrations?
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 don't see any examples of this elsewhere and I have some concerns regarding the typecheck. The typecheck would fail before it was run. In theory we could end up with migrations which never pass typecheck if the types differ between up
and down
too much.
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 think it's easier to read with kysely. The big SQL string feels more error prone to me. There's a little bit more type safety too as it ensures you write alterTable
etc correctly. But I don't think it matters much.
There is a recent migration from Matt that used kysely: https://github.com/ParabolInc/parabol/blob/0092d0baf6852e820384ae114f59e23294fed0e9/packages/server/postgres/migrations/1712075131388_retroReflectionGroups2.ts
Description
Fixes #9772
[Please include a summary of the changes and the related issue]
Demo
[If possible, please include a screenshot or gif/video, it'll make it easier for reviewers to understand the scope of the changes and how the change is supposed to work. If you're introducing something new or changing the existing patterns, please share a Loom and explain what decisions you've made and under what circumstances]
Testing scenarios
Final checklist