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.

Sign up for GitHub

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

Jump to bottom

PGVector, remove by ID, same impl as #1020 but on top of last dev. #1113

Merged
merged 5 commits into from
May 23, 2024
Merged

PGVector, remove by ID, same impl as #1020 but on top of last dev. #1113

merged 5 commits into from
May 23, 2024

Conversation

humcqc
Copy link
Contributor

@humcqc humcqc commented May 15, 2024

same impl as #1020 but on top of last dev.

@humcqc
Copy link
Contributor Author

humcqc commented May 15, 2024

@langchain4j , @nottyjay, this is the same impl as #1020

@humcqc humcqc mentioned this pull request May 22, 2024
Open
langchain4j
langchain4j reviewed May 22, 2024
View reviewed changes
Copy link
Owner

@langchain4j langchain4j left a comment

Choose a reason for hiding this comment

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

@humcqc thanks a lot!

...-pgvector/src/main/java/dev/langchain4j/store/embedding/pgvector/PgVectorEmbeddingStore.java Outdated Show resolved Hide resolved
langchain4j-core/src/main/java/dev/langchain4j/store/embedding/EmbeddingStore.java Outdated Show resolved Hide resolved
langchain4j-core/src/main/java/dev/langchain4j/store/embedding/EmbeddingStore.java Outdated Show resolved Hide resolved
langchain4j-core/src/main/java/dev/langchain4j/store/embedding/EmbeddingStore.java Show resolved Hide resolved
@langchain4j
Copy link
Owner

@geoand @jmartisk could you please take a look as well? 🙏

@langchain4j
Copy link
Owner

@geoand @jmartisk could you please check? Does this API makes sense? I would love to include this in the release today.

@geoand
Copy link
Contributor

geoand commented May 23, 2024

LGTM, but I'll let @jmartisk comment further since he has interacted with the EmbeddingStore much more than I have

langchain4j
langchain4j approved these changes May 23, 2024
View reviewed changes
Copy link
Owner

@langchain4j langchain4j left a comment

Choose a reason for hiding this comment

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

@humcqc thanks a lot!
I will merge it now and do some changes in a separate commit, as I can't push changes to your fork.

@langchain4j langchain4j merged commit 1348786 into langchain4j:main May 23, 2024
6 checks passed
langchain4j added a commit that referenced this pull request May 23, 2024
@langchain4j
PGVector, remove by ID, same impl as #1020 but on top of last dev. (#…
358f63c 
…1113)

#301: Add support for remove operations in EmbeddingStore (PGVector)
@humcqc
Copy link
Contributor Author

humcqc commented May 23, 2024

humm, I had the review fixes in my local repo waiting for the answer on my last comment::
I'm not sure about removeAll(Collection). and removeAll(Filter)
If we keep this way, removeAll(null) will have 2 opposite behaviors:
-> https://github.com/langchain4j/langchain4j/pull/1113/files#diff-40e560d628f612b7350552586991a852f85d9a2c35f15604a4d345bc13209912R116
-> https://github.com/langchain4j/langchain4j/pull/1113/files#diff-40e560d628f612b7350552586991a852f85d9a2c35f15604a4d345bc13209912R133

@langchain4j , @jmartisk WDYT about:
removeAll() -> remove all
remove(String... ids) -> remove records by id ( one ore +), if null no record deleted
removeByMetadata(Filter) -> remove filtered records, if null all record will be deleted;

@langchain4j
Copy link
Owner

humm, I had the review fixes in my local repo waiting for the answer on my last comment::

@humcqc my bad, pardon!
I am a bit in a rush, wanted it to make it in this release that I am preparing now.
BTW, I did not see a comment from you. Maybe it is pending? If yes, I can't see it...

I have changed it so that null/empty ids/filter are not valid inputs.

@humcqc
Copy link
Contributor Author

humcqc commented May 23, 2024 •
edited

No pb! if the current version is ok for you it's ok for me.

BTW, I did not see a comment from you. Maybe it is pending? If yes, I can't see it...

:) My bad so, didn't notice the pending stuff, I was a little bit frustrated to not have answers :)
How can I change this pending stuff ?

@langchain4j
Copy link
Owner

@humcqc yeah, I made this mistake many times 😆 You need to go to files changed -> review changes -> submit

humcqc
humcqc commented May 23, 2024
View reviewed changes
Copy link
Contributor Author

@humcqc humcqc left a comment

Choose a reason for hiding this comment

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

:) a little bit late :)

...-pgvector/src/main/java/dev/langchain4j/store/embedding/pgvector/PgVectorEmbeddingStore.java Outdated
*/
@Override
public List<String> removeAll(List<String> ids) {
return ids.stream().filter(this::remove).collect(Collectors.toList());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be enhance with batch processing or in a single request if we just use a boolean in return.

...-pgvector/src/main/java/dev/langchain4j/store/embedding/pgvector/PgVectorEmbeddingStore.java Outdated Show resolved Hide resolved
langchain4j-core/src/main/java/dev/langchain4j/store/embedding/EmbeddingStore.java Show resolved Hide resolved
langchain4j-core/src/main/java/dev/langchain4j/store/embedding/EmbeddingStore.java Show resolved Hide resolved
langchain4j-core/src/main/java/dev/langchain4j/store/embedding/EmbeddingStore.java Show resolved Hide resolved
langchain4j-core/src/main/java/dev/langchain4j/store/embedding/EmbeddingStore.java Show resolved Hide resolved
langchain4j-core/src/main/java/dev/langchain4j/store/embedding/EmbeddingStore.java Show resolved Hide resolved
This was referenced May 23, 2024
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmartisk jmartisk jmartisk left review comments

@langchain4j langchain4j langchain4j approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@humcqc @langchain4j @geoand @jmartisk