Skip to content

Vector search on repository level #3037

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

Closed
wants to merge 2 commits into from
Closed

Vector search on repository level #3037

wants to merge 2 commits into from

Conversation

meistermeier
Copy link
Collaborator

This will introduce the support for Neo4j vector search.
For now it's focused on the repository level to enhance the derived finder methods.

Copy link
Member

@mp911de mp911de left a comment

Choose a reason for hiding this comment

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

Awesome work, congrats. I left a few high-level thoughts on the PR which might be useful to reduce repetition.

@@ -98,11 +101,30 @@ boolean isGeoNearQuery() {
return GeoPage.class.isAssignableFrom(returnType);
}

boolean isVectorSearchQuery() {
var repositoryMethod = this.queryMethod.getMethod();
Copy link
Member

Choose a reason for hiding this comment

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

Could QueryMethod.isSearchQuery() be useful here?

Would it also make sense to check for the presence of the @VectorSearch annotation?

@@ -73,10 +75,21 @@ public final class QueryFragmentsAndParameters {
@Nullable
private NodeDescription<?> nodeDescription;

public QueryFragmentsAndParameters(@Nullable NodeDescription<?> nodeDescription, QueryFragments queryFragments,
Copy link
Member

Choose a reason for hiding this comment

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

Design-wise, using a builder could help to avoid introducing additional constructors for such a class when extending arguments. That's a learning from various other Spring Data modules that can make your changes less painful.

@@ -57,6 +57,10 @@ private ReactivePartTreeNeo4jQuery(ReactiveNeo4jOperations neo4jOperations, Neo4

static RepositoryQuery create(ReactiveNeo4jOperations neo4jOperations, Neo4jMappingContext mappingContext,
Neo4jQueryMethod queryMethod, ProjectionFactory factory) {
if (queryMethod.hasVectorSearchAnnotation()
&& queryMethod.getVectorSearchAnnotation().get().numberOfNodes() < 1) {
throw new IllegalArgumentException("Number of nodes in a vector search has to be greater than zero.");
Copy link
Member

Choose a reason for hiding this comment

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

Nit: It makes sense to check how these exceptions translate during runtime and whether an enclosing exception provides sufficient context to identify the query method that has caused this verification error.

*
* @author Gerrit Meier
*/
@Retention(RetentionPolicy.RUNTIME)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: A bit of Javadoc helps with dev experience, also, @since tags for feature discoverability.

* @param numberOfNodes number of nodes to fetch from the index search
* @param score score filter
*/
record VectorSearchFragment(String indexName, int numberOfNodes, @Nullable Double score) {
Copy link
Member

Choose a reason for hiding this comment

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

Package-protected records FTW. We use these in a similar manner.

Nerd sniping: OptionalDouble 😵‍💫

protected static Neo4jExtension.Neo4jConnectionSupport neo4jConnectionSupport;

@BeforeEach
void setupData(@Autowired BookmarkCapture bookmarkCapture, @Autowired Driver driver) {
Copy link
Member

Choose a reason for hiding this comment

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

Another nerd-snipe: Inject a Session that gets closed after the test run to reduce try repetition.

@meistermeier
Copy link
Collaborator Author

Closed with merge of d1b7692

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.

2 participants