Skip to content

Adding Oracle Database Vector Store Connector #327

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

Open
wants to merge 62 commits into
base: main
Choose a base branch
from

Conversation

psilberk
Copy link

@psilberk psilberk commented Jul 8, 2025

Motivation and Context

Addressing #321

  1. Why is this change required?: adding Oracle implementation.
  2. What problem does it solve?: NA
  3. What scenario does it contribute to?: NA
  4. If it fixes an open issue, please link to the issue here.

Description

We added the Oracle implementation. Following the similar approach of other vendors.

Contribution Checklist

psilberk and others added 30 commits April 22, 2025 09:33
@karianna karianna requested a review from johnoliver July 8, 2025 23:20
*
* @return the mapping between the supported Java key types and the Oracle database type.
*/
public static HashMap<Class<?>, String> getSupportedKeyTypes() {
Copy link
Member

Choose a reason for hiding this comment

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

I would return an unmodifiable Map<Class<?>, String> if possible

* @return the mapping between the supported Java data types and the Oracle database type.
*/
public static Map<Class<?>, String> getSupportedVectorTypes() {
return supportedVectorTypes;
Copy link
Member

Choose a reason for hiding this comment

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

make unmodifiable

*/
public static String getCreateVectorIndexStatement(VectorStoreRecordVectorField field, String collectionTableName) {
switch (field.getIndexKind()) {
case IVFFLAT:
Copy link
Member

Choose a reason for hiding this comment

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

worth adding name checks to make sure we have no sql injections?

.getField(options.getVectorFieldName());
DistanceFunction distanceFunction = vectorField == null ? null : vectorField.getDistanceFunction();
if (options.getVectorFieldName() != null && vectorField == null) {
throw new SKException("");
Copy link
Member

Choose a reason for hiding this comment

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

Would be more descriptive here

}

return new OracleVectorStoreRecordMapper<>(
(resultSet, options) -> {
Copy link
Member

Choose a reason for hiding this comment

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

I would pull this out into its own method, a very large lambda

pom.xml Outdated
@@ -71,13 +71,24 @@
<module>semantickernel-bom</module>
<module>semantickernel-api</module>
<module>semantickernel-experimental</module>
<module>samples</module>
Copy link
Member

Choose a reason for hiding this comment

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

We only add samples to build in the with-samples maven profile, since the samples require a higher version of Java

Copy link
Author

Choose a reason for hiding this comment

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

Addressed in latest commit.

<dependency>
<groupId>com.microsoft.semantic-kernel</groupId>
<artifactId>semantickernel-data-jdbc</artifactId>
<version>1.4.4-SNAPSHOT</version>
Copy link
Member

Choose a reason for hiding this comment

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

should these be ${project.version} ?

Copy link
Author

Choose a reason for hiding this comment

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

Addressed in latest commit.

@@ -0,0 +1,30 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

This does proliferate the number of maven modules we publish to create a single class module, is there some dependency restriction that requires this module?

<dependency>
<groupId>io.projectreactor</groupId>
<artifactId>reactor-core</artifactId>
<version>3.4.38</version>
Copy link
Member

Choose a reason for hiding this comment

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

do we need to specify this version, is it not already defined?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, we need to define it:
[ERROR] [ERROR] Some problems were encountered while processing the POMs:
[ERROR] 'dependencies.dependency.version' for io.projectreactor:reactor-core:jar is missing. @ line 41, column 21

@@ -193,7 +194,7 @@ public static VectorStoreRecordDefinition fromRecordClass(Class<?> recordClass)
dataFields.add(VectorStoreRecordDataField.builder()
.withName(field.getName())
.withStorageName(storageName)
.withFieldType(field.getType())
.withFieldType(field.getType(), List.class.equals(field.getType()) ? (Class<?>)((ParameterizedType) field.getGenericType()).getActualTypeArguments()[0] : null)
Copy link
Member

Choose a reason for hiding this comment

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

should this be List.class.isInstance?

</parent>

<artifactId>semantickernel-data-oracle</artifactId>
<name>Semantic Kernel Oracle connector</name>

Choose a reason for hiding this comment

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

Please add version 0.0.1 and indicate that this is a preview version.

@psilberk
Copy link
Author

psilberk commented Jul 9, 2025 via email

@psilberk the command you issued was incorrect. Please try again.

Examples are:

@microsoft-github-policy-service agree

and

@microsoft-github-policy-service agree company="your company"

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.

5 participants