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

feat: postgresql store #157

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

milancermak
Copy link

Resolves #5

I've modeled the crate on the SQLite one.

On the DB side, the crate is relying on the pgvector extension. It creates an HSNW index and uses the cosine distane operator to compare embeddings.

Please let me know it this looks ok or if there are any parts of the code that should be changed.

@0xMochan
Copy link
Contributor

0xMochan commented Dec 18, 2024

Thanks for your contribution, nearly done with my review but I'll go ahead and approve and run the workflow. I think you might need to cargo fmt and pass cargo clippy as well.

@b0xtch
Copy link

b0xtch commented Dec 20, 2024

Hey great work, any update on this?

@0xMochan
Copy link
Contributor

Hey great work, any update on this?

I'm still in my review, I got wrapped up in some other responsibilities, will get it out ASAP!

Copy link
Contributor

@0xMochan 0xMochan left a comment

Choose a reason for hiding this comment

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

Apologies for the delay on this review! Mostly looking good, a couple of changes I've requested here.

One note: I noticed that 'static was used a good bit here which was likely inherited from the sqlite implementation. I think this is due to how the connection requires an closure with an async block which makes borrowing and using such things really odd.

Recently, proper async closures which will get into Rust stable sometime in 2025 which should help us alleviate our usage of 'static here (i think!)

rig-postgres/examples/vector_search_postgres.rs Outdated Show resolved Hide resolved
rig-postgres/src/lib.rs Show resolved Hide resolved
rig-postgres/src/lib.rs Show resolved Hide resolved
rig-postgres/src/lib.rs Show resolved Hide resolved
rig-postgres/README.md Outdated Show resolved Hide resolved
rig-postgres/README.md Show resolved Hide resolved
rig-postgres/src/lib.rs Show resolved Hide resolved
@milancermak
Copy link
Author

Thanks for the review. I'm still in holiday mode until New Year's, will address the comments on Jan 2 or 3.

@milancermak
Copy link
Author

Hey @0xMochan ready for another round. I've addressed the comments you brought up, optimistically resolved the ones I think are done, but feel free to reopen if you think otherwise.

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

Successfully merging this pull request may close these issues.

feat: Add support for PostgreSQL vector store
3 participants