-
Notifications
You must be signed in to change notification settings - Fork 677
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
AzureAiSearchEmbeddingStore - add "indexName" to the builder #1084
Conversation
this.indexName = indexName; | ||
return this; | ||
} | ||
|
||
public AzureAiSearchEmbeddingStore build() { | ||
ensureNotNull(endpoint, "endpoint"); |
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 assume the builder will ensure that either indexName or index is set but never both. Otherwise, it could be a bit hard to predict the implementation behavior.
If you agree then the test should verify this rule as well.
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.
Please have a look at my latest commit at ec7fd32 where I add a test for this
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.
looks good!
Since another comment is resolved now, I'll just write here my thoughts, however, you decide :)
I think it would be great to think about the following: What if someone else asks you to add one more option, e.g. 'eTag' or anything else?
- how much documentation will you update regarding what erases what and so on?
- how many NULLs / IFs / logical operators will appear in your code?
- will the complexity of the API increase or not?
If you think the builder API will be still OK, then this PR should be merged.
I got another idea of how it could look like from the client perspective.
AzureAiSearchEmbeddingStore.builder()
.apiKey(properties.apiKey())
.endpoint(properties.endpoint())
.dimensions(1000)
//defaultSearchIndex() is a static method provided by AbstractAzureAiSearchEmbeddingStore
.index(defaultSearchIndex().setName("myName"))
.build();
However, the code above will probably struggle supporting dimensions.
One more approach:
AzureAiSearchEmbeddingStore.builder()
.apiKey(properties.apiKey())
.endpoint(properties.endpoint())
.dimensions(1000)
//your implementation provides a template (pre-built SearchIndex) with all fields set, so developers will be able to modify it in the way they want by passing a Consumer<SearchIndex>
//As a result, the implementation won't suffer from additional NULLs/IF-checks/exceptions thrown.
.index(indexTemplate -> indexTemplate.setName("myName"))
.build();
```
.../src/main/java/dev/langchain4j/store/embedding/azure/search/AzureAiSearchEmbeddingStore.java
Show resolved
Hide resolved
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.
@jdubois thank you a lot!
Fix #1062