community[patch]: Add pgvector index using HNSW #5564
Conversation
dosubot
bot
added
the
size:L
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
dosubot
bot
added
auto:documentation
|
Adding this for reference https://python.langchain.com/v0.1/docs/integrations/vectorstores/pgembedding/#create-hnsw-index |
|
|
||
| const createIndexQuery = `CREATE INDEX IF NOT EXISTS ${ | ||
| this.computedTableName | ||
| }_embedding_idx |
There was a problem hiding this comment.
Should this name be differentiated based on the number of dimensions and/or column?
Could maybe see a case where someone has multiple embedding fields in a given table?
There was a problem hiding this comment.
Should this name be differentiated based on the number of dimensions and/or column?
As far as I can see, there's only support to create one embedding field per table. I'll change the index name to use the vectorColumnName that's computed in the class for consistency.
Could maybe see a case where someone has multiple embedding fields in a given table?
Haven't seen this pattern yet... I guess is possible, but IMO its easier to handle one vector per table + metadata.
There was a problem hiding this comment.
I think this works for now
|
Very cool! |
jacoblee93
changed the title
|
Hey apologies for the delay - @eyurtsev is on vacation but I'd really like him to take a look. Might be a few more days. |
| * @returns Promise that resolves with the query response of creating the index. | ||
| */ | ||
| async createHnswIndex(config?: { | ||
| dims?: number; |
There was a problem hiding this comment.
Should we require the dimension instead of assigning a default? Users likely use lots of different embedding models?
There was a problem hiding this comment.
I refactored to be mandatory... I also think that assigning the value is less prone error.
| * Method to create the HNSW index on the vector column. | ||
| * | ||
| * @param dims - Defines the number of dimensions in your vector data, max: 2000. For example, use 1536 for OpenAI's text-embedding-ada-002 model and 1024 for amazon.titan-embed-text-v2:0 | ||
| * @param m - The max number of connections per layer (16 by default) |
There was a problem hiding this comment.
Any reference material we could link to that would help someone figure out how to choose values for m?
There was a problem hiding this comment.
I added a reference to the paper in the docs (pgvector.mdx) and added a bit more context in the comments for m and efConstruction parameters
Looks like this:
More info at the Pgvector GitHub project and the HNSW paper from Malkov Yu A. and Yashunin D. A.. 2020. Efficient and robust approximate nearest neighbor search using hierarchical navigable small world graphs
| }_embedding_idx | ||
| ON ${this.computedTableName} USING hnsw ((${ | ||
| this.vectorColumnName | ||
| }::vector(${config?.dims || 1536})) ${idxDistanceFunction}) |
There was a problem hiding this comment.
Do we have any utilities to sanitize interpolated values? This code should be only exposed to developers, but if we have existing sanitization code won't hurt to add some more defensive code
There was a problem hiding this comment.
I've added the pg-format lib https://www.npmjs.com/package/pg-format to format the sql literals.
It should provide some guards for this.
There was a problem hiding this comment.
Can we actually remove this (for now)? It'll make existing users install a new package.
In the near-ish term, we should factor out into a @langchain/pgvector package that can have this as a hard dep.
|
|
||
| const createIndexQuery = `CREATE INDEX IF NOT EXISTS ${ | ||
| this.computedTableName | ||
| }_embedding_idx |
There was a problem hiding this comment.
I think this works for now
|
Hey @jl4nz, can we revert that new |
jl4nz
force-pushed
the
pg-hnsw-create-index
branch
from
9adedd5 to
7bfec42
Compare
Got it, done. |
|
Hey @jacoblee93, just pinging to check if this would be good to go. |
|
Yes, thank you! |
Add support to optionally create a HNSW index on for pgvector