Skip to content

Conversation

rahulmitt
Copy link
Contributor

@rahulmitt rahulmitt commented Apr 2, 2024

Thank you for taking time to contribute this pull request!
You might have already read the [contributor guide][1], but as a reminder, please make sure to:

  • * Sign the contributor license agreement
  • * Rebase your changes on the latest main branch and squash your commits
  • * Add/Update unit tests as needed
  • * Run a build and make sure all tests pass prior to submission

Copy link
Contributor

@tzolov tzolov left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @rahulmitt !
I left few review comments in the code.
In addition you have to

  • add the vector store dependency to the spring-bom.
  • will consider adding auto-configuration and spring-boot for it? This is the primary way to use the vector stores.

package org.springframework.ai.vectorstore;

import com.fasterxml.jackson.core.JsonProcessingException;
import lombok.extern.slf4j.Slf4j;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't use the lombok in the vector store implementation.
Later might be fine for end user applications, but because of multiple technical and compatibility issues is inappropriate for utilities/libraries such as spring-ai.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+ACK

<version>2.20.11</version>
</dependency>

<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't use the lombok in the vector store implementation.
Later might be fine for end user applications, but because of multiple technical and compatibility issues is inappropriate for utilities/libraries such as spring-ai.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+ACK

<dependency>
<groupId>com.sap.cloud.db.jdbc</groupId>
<artifactId>ngdbc</artifactId>
<version>2.20.11</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Move the version as a property in the parent pom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+ACK

@@ -0,0 +1,284 @@
# SpringAI using SAP HANA Cloud vector engine
Copy link
Contributor

Choose a reason for hiding this comment

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

Please convert the README into an adoc under the vectordbs and add a link to the nav.adoc.
The README can contain a single line link to the adoc documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+ACK

}

@Override
public List<Document> similaritySearch(SearchRequest request) {
Copy link
Contributor

@tzolov tzolov Apr 4, 2024

Choose a reason for hiding this comment

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

Most of the VectorStore integrations provide support for the Metadata Expression Filtering.
Please consider adding filtering support. Otherwise you should throw and exception that the feature is not supported like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+ACK

@rahulmitt rahulmitt requested a review from tzolov April 6, 2024 09:34
Copy link
Contributor

@tzolov tzolov left a comment

Choose a reason for hiding this comment

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

Great job! Thanks @rahulmitt
LGTM, will merge it soon.

@tzolov
Copy link
Contributor

tzolov commented Apr 6, 2024

@rahulmitt when i try to run the ITs locally ./mvnw clean install -pl vector-stores/spring-ai-hanadb-store -Pintegration-tests it fails with:

        ... 33 more
Caused by: org.springframework.beans.BeanInstantiationException: Failed to instantiate [com.zaxxer.hikari.HikariDataSource]: Factory method 'dataSource' threw exception with message: Failed to determine a suitable driver class
        at org.springframework.beans.factory.support.SimpleInstantiationStrategy.instantiate(SimpleInstantiationStrategy.java:177)
        at org.springframework.beans.factory.support.ConstructorResolver.instantiate(ConstructorResolver.java:644)
        ... 47 more
Caused by: org.springframework.boot.autoconfigure.jdbc.DataSourceProperties$DataSourceBeanCreationException: Failed to determine a suitable driver class
        at org.springframework.boot.autoconfigure.jdbc.DataSourceProperties.determineDriverClassName(DataSourceProperties.java:186)
        at org.springframework.boot.autoconfigure.jdbc.PropertiesJdbcConnectionDetails.getDriverClassName(PropertiesJdbcConnectionDetails.java:49)
        at org.springframework.boot.autoconfigure.jdbc.DataSourceConfiguration.createDataSource(DataSourceConfiguration.java:55)
        at org.springframework.boot.autoconfigure.jdbc.DataSourceConfiguration$Hikari.dataSource(DataSourceConfiguration.java:117)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:568)
        at org.springframework.beans.factory.support.SimpleInstantiationStrategy.instantiate(SimpleInstantiationStrategy.java:140)
        ... 48 more


<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-data-jpa</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I missed this.
Please try to replace the boot starter with the relevant data jpa dependency.
We should not use starters here, not even for the testing.
(P.S. I know that we leaked the logging starter in some places that we will have to fix)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Should be fixed now. Thanks

@rahulmitt rahulmitt requested a review from tzolov April 6, 2024 18:56
@tzolov
Copy link
Contributor

tzolov commented Apr 6, 2024

Hi @rahulmitt , thank you for the updates.
I've noticed that you have not provided auto-configuration test, so i though i can add one myself. But apparently getting a test/try account for SAP HANA Cloud is not that obvious.
Cloud you please update your documentation to explain where and how one can obtain SAP HANA Cloud access and how to configure its Vector Store capabilities?.

Also:

  • Add an Auto-configuration integration test. You can check the other Vectors Stores for sample auto-config tests.
  • run ./mvnw license:format -Plicense to fix the license headers issues
  • run ./mvnw javadoc:javadoc -pl vector-stores/spring-ai-hanadb-store -Pjavadoc to fix the javadocs issues.
  • run ./mvnw spring-javaformat:apply to fix any code formatting issues.

@rahulmitt
Copy link
Contributor Author

rahulmitt commented Apr 7, 2024

Hi @tzolov - thanks for your kind reviews. Let me work on the following:

  • Add unit test for HanaCloudVectorStoreProperties.java
  • Add IT for HanaCloudVectorStoreAutoConfiguration.java
  • Write documentation to provision a SAP BTP trial account and create SAP Hana Cloud instance
  • Run ./mvnw license:format -Plicense to fix the license headers issues
  • Run ./mvnw javadoc:javadoc -pl vector-stores/spring-ai-hanadb-store -Pjavadoc to fix the javadocs issues.
  • Run ./mvnw spring-javaformat:apply to fix any code formatting issues.

Will update the status here.

@rahulmitt
Copy link
Contributor Author

rahulmitt commented Apr 7, 2024

Hi @tzolov - I attempted to write the IT for HanaCloudVectorStoreAutoConfiguration. However, it is failing with below error. So, checked in as disabled for now.

java.io.FileNotFoundException: class path resource [org/springframework/data/jdbc/repository/config/AbstractJdbcConfiguration.class] cannot be opened because it does not exist

If I add the following dependency to spring-ai-spring-boot-autoconfigure/pom.xml, the above error is resolved.

<dependency>
	<groupId>org.springframework.boot</groupId>
	<artifactId>spring-boot-starter-data-jdbc</artifactId>
</dependency>

However, it starts complaining about:

org.springframework.beans.factory.NoSuchBeanDefinitionException: No qualifying bean of type 'org.springframework.ai.vectorstore.HanaVectorRepository<org.springframework.ai.vectorstore.HanaVectorEntity>' available: expected at least 1 bean which qualifies as autowire candidate. Dependency annotations: {}

I am looking for pointers on how to fix this. Do you mind taking a look pls?

@tzolov tzolov self-assigned this Apr 8, 2024
@tzolov tzolov added enhancement New feature or request vector store labels Apr 8, 2024
@tzolov tzolov added this to the 1.0.0-M1 milestone Apr 8, 2024
@tzolov
Copy link
Contributor

tzolov commented Apr 9, 2024

@rahulmitt thank you for the contribution.
I applied some fixes to the docs and the Autoconfiguraiton you can review (in the merged code)

@tzolov
Copy link
Contributor

tzolov commented Apr 9, 2024

Rebased, squashed and merged at 466b824

Additional changes
 - add @AutoConfiguration(after = { JpaRepositoriesAutoConfiguration.class })
 - update the handa docs structure.

@tzolov tzolov closed this Apr 9, 2024
@markpollack
Copy link
Member

@rahulmitt This is an old issue and hope you see this. We have not been able to gain access to SAP HANA to see if this functionality still works. Can you help?

@rahulmitt
Copy link
Contributor Author

Hi - see if this documentation helps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request vector store

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants