Skip to content
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

Merged
merged 3 commits into from
May 22, 2024

Conversation

jdubois
Copy link
Contributor

@jdubois jdubois commented May 10, 2024

Fix #1062

this.indexName = indexName;
return this;
}

public AzureAiSearchEmbeddingStore build() {
ensureNotNull(endpoint, "endpoint");

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.

Copy link
Contributor Author

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

Copy link

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();
```

@langchain4j langchain4j added the P2 High priority label May 10, 2024
Copy link
Owner

@langchain4j langchain4j left a 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!

@langchain4j langchain4j merged commit 495344e into langchain4j:main May 22, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 High priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] AzureAiSearchEmbeddingStore - add "indexName" to the builder
3 participants