feat: Add get_by_ids and aget_by_ids to vectorstore #276
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
averikitsch
reviewed
Feb 27, 2025
|
|
||
| column_names = ", ".join(f'"{col}"' for col in columns) | ||
|
|
||
| query = f'SELECT {column_names} FROM "{self.schema_name}"."{self.table_name}" WHERE {self.id_column} IN ({id_list_str});' |
There was a problem hiding this comment.
id_column should be in quotes
averikitsch
requested changes
Feb 27, 2025
| Document( | ||
| page_content=row[self.content_column], | ||
| metadata=metadata, | ||
| id=row[self.id_column], |
There was a problem hiding this comment.
I am wondering if we should cast this to a string just in case
There was a problem hiding this comment.
Yes, we have that option, but wouldn't that negate the value of having customizable ID columns?
There was a problem hiding this comment.
The Document interface is enforcing the str Id in the document object https://github.com/langchain-ai/langchain/blob/33354f984fba660e71ca0039cfbd3cf37643cfab/libs/core/langchain_core/documents/base.py#L34. Users will still be able to insert and use the id data type of their choice
| @@ -291,6 +291,54 @@ async def __aadd_embeddings( | |||
|
|
|||
| return ids | |||
|
|
|||
| async def aget_by_ids(self, ids: Sequence[str]) -> List[Document]: | |||
There was a problem hiding this comment.
We should use lower case "list" to follow guidance of https://peps.python.org/pep-0585/
There was a problem hiding this comment.
Can you add a test for get_by_ids ?
There was a problem hiding this comment.
Sure, added a test for the sync function in the async vectorstore
averikitsch
approved these changes
Mar 3, 2025
dishaprakash
added a commit
to googleapis/langchain-google-alloydb-pg-python
that referenced
this pull request
Mar 4, 2025
chore: Minor cleanup of vectorstore All the review comments from the [cloud sql pr](googleapis/langchain-google-cloud-sql-pg-python#276) are being propogated here
dishaprakash
added a commit
to googleapis/langchain-google-alloydb-pg-python
that referenced
this pull request
Mar 4, 2025
* chore: Minor cleanup of vectorstore chore: Minor cleanup of vectorstore All the review comments from the [cloud sql pr](googleapis/langchain-google-cloud-sql-pg-python#276) are being propogated here * Use `list` instead of `List` * remove unused import * Add sync test in async vectorstore
dishaprakash
added a commit
to googleapis/langchain-google-alloydb-pg-python
that referenced
this pull request
Mar 19, 2025
…402) * chore: Add Langchain standard vectorstore tests. (#356) * chore: Add Langchain standard vectorstore tests. * add header to test file * change header to 2025 * suppress warnings * chore: support ids in Documents and update insert SQL query to upsert (#361) * chore: support ids in Documents and update insert SQL query to upsert * remove pulling id from metadata * Fix previous vectorstore tests * Add comment * feat: Add get_by_ids and aget_by_ids to vectorstore (#364) * feat: Add get_by_ids and aget_by_ids to vectorstore * remove embedding column from being fetched * fix test * chore: Minor cleanup of vectorstore (#371) * chore: Minor cleanup of vectorstore chore: Minor cleanup of vectorstore All the review comments from the [cloud sql pr](googleapis/langchain-google-cloud-sql-pg-python#276) are being propogated here * Use `list` instead of `List` * remove unused import * Add sync test in async vectorstore * feat: Enable DB-Agnostic metadata filtering in vectorstores (#382) * feat!: Enable DB-Agnostic metadata filtering in vectorstores * Add header to util file * Minor change * Remove older filter tests * Change the data path * Linter fix * Remove async from func definition * Add sync test to async class * Remove table after test * Add fixture marker * Test * debug * debug * debug * debug * debug * debug * debug * debug * debug * debug * Linter fix * debug * debug * debug * Make non breaking change by supporting string filter * add text filter tests * Unify and/or operator * minor fix * minor fix * debug cloud build * debug cloud build * debug cloud build * debug cloud build * debug cloud build * debug cloud build * debug cloud build * debug cloud build * Debug * Make sure add documents does not alter original documents. * Minor change * Minor change * Review changes * Update test_vectorstore_search.py * Add extra test for code coverage * Add extra negative tests
dishaprakash
added a commit
that referenced
this pull request
Mar 19, 2025
…#290) * chore: Add Langchain vectorstore standard suite tests (#272) * chore: Add Langchain vectorstore standard suite tests * fix test * chore: support ids in Documents and update insert SQL query to upsert (#274) * chore: support ids in Documents and update insert SQL query to upsert * chore: Add Langchain vectorstore standard suite tests (#272) * chore: Add Langchain vectorstore standard suite tests * fix test * Newline * remove Newline * Review changes * feat: Add get_by_ids and aget_by_ids to vectorstore (#276) * feat: Add get_by_ids and aget_by_ids to vectorstore * Review changes * Add sync test in async vectorstore * chore: Convert id field in document to str (#278) chore: Convert id field in document to str * feat: Enable DB-Agnostic metadata filtering in vectorstores (#289) * feat: Enable DB-Agnostic metadata filtering in vectorstores * minor change * minor change * minor change * minor change * Add negative tests for code coverager * Add extra tests for code coverage
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat: Add get_by_ids and aget_by_ids to vectorstore