-
Notifications
You must be signed in to change notification settings - Fork 2k
refactor: move PgVectorStore to dedicated package and update builder … #1889
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
refactor: move PgVectorStore to dedicated package and update builder … #1889
Conversation
|
|
||
| var initializeSchema = properties.isInitializeSchema(); | ||
|
|
||
| return new PgVectorStore.Builder(jdbcTemplate, embeddingModel).withSchemaName(properties.getSchemaName()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General thought: Given every vector store requires a client (jdbctemplate in this case) and an embedding model, can we make both of these as the default argument to the vector store builder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the design we are using is to have zero arg builder consistent throughout the project. Reason is imagine a class that need 5 strings as required, if the builder takes 5 args, we haven't achieved any simplification by adding a fluent api.
| BatchingStrategy batchingStrategy, int maxDocumentBatchSize) { | ||
|
|
||
| super(observationRegistry, customObservationConvention); | ||
| // @Deprecated(forRemoval = true, since = "1.0.0-M5") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the plan was to keep the methods by deprecating instead of the removing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
| this.removeExistingVectorStoreTable, this.indexType, this.initializeSchema, | ||
| this.observationRegistry, this.searchObservationConvention, this.batchingStrategy, | ||
| this.maxDocumentBatchSize); | ||
| return PgVectorStore.builder(this.jdbcTemplate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both observationRegistry and searchObservationConvention are missing in this builder setup?
…pattern - Move PgVectorStore and related classes to org.springframework.ai.pg.vectorstore package - Update builder pattern to use more idiomatic method names (e.g. withSchemaName -> schemaName) - Deprecate existing constructors and old Builder class in favor of new static builder() method - Update tests to reflect the new builder style usage
d3498a1 to
94bf663
Compare
|
The postgresql library for vector store support uses |
|
updated reference docs and javadoc. change to use Merged in 677a18e |
…pattern