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

feat: Add support for dynamic metadata fields in Elasticsearch index creation #14431

Merged
merged 7 commits into from
Jun 28, 2024

Conversation

minglu7
Copy link
Contributor

@minglu7 minglu7 commented Jun 27, 2024

feat: Add support for dynamic metadata fields in Elasticsearch index creation

This commit enhances the flexibility of our Elasticsearch index creation by allowing dynamic metadata fields to be passed as keyword arguments during the initialization.

Key changes:

  • Updated the __init__ method to accept metadata_mappings as part of **kwargs, enabling the customization of metadata fields.
  • Merged basic metadata fields with user-defined fields to create a more customizable index mapping.
  • Modified the _create_index_if_not_exists method to incorporate the merged metadata fields.

Benefits:

  • Improved extensibility: Users can now easily add custom metadata fields without modifying the core implementation.
  • Enhanced flexibility: The solution accommodates a variety of metadata types, including text, keyword, date, and numeric fields.
  • Simplified API: Passing metadata_mappings directly in kwargs keeps the method signatures clean and intuitive.

This change ensures that our Elasticsearch indices can be tailored to specific application requirements while maintaining a straightforward and flexible API.

@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jun 27, 2024
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Jun 27, 2024
@minglu7
Copy link
Contributor Author

minglu7 commented Jun 28, 2024

@logan-markewich Actually, I didn't think that much about it. At the time, I just thought that the metadata_mappings could be extracted from **kwargs. However, I think setting it as a named parameter is a good idea; it looks more standardized.

Copy link
Contributor Author

@minglu7 minglu7 left a comment

Choose a reason for hiding this comment

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

In this example, metadata_mappings doesn't seem to be a commonly used or particularly important parameter. I think it is an optional and less frequently used parameter. Personally, I believe using **kwargs might be more appropriate.

@nerdai
Copy link
Contributor

nerdai commented Jun 28, 2024

In this example, metadata_mappings doesn't seem to be a commonly used or particularly important parameter. I think it is an optional and less frequently used parameter. Personally, I believe using **kwargs might be more appropriate.

Thanks @minglu7. I think I have I agree with Logan here. I believe parameters such as es_url and es_cloud_id would be used the same amount of times as metadata_mappings and both are parameter fields for this class.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 28, 2024
@logan-markewich logan-markewich enabled auto-merge (squash) June 28, 2024 21:44
@logan-markewich logan-markewich merged commit dd69107 into run-llama:main Jun 28, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants