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

fix: use docstore.index for get_metadata_values_by_key (OpenSearch / elasticsearch) #7562

Merged
merged 2 commits into from
Apr 22, 2024

Conversation

tstadel
Copy link
Member

@tstadel tstadel commented Apr 20, 2024

Related Issues

All docstore methods normally take self.index as index param of opensearch / elasticsearch requests if no value for index has been passed explicitly. get_metadata_values_by_key misses this behavior.

Proposed Changes:

  • get_metadata_values_by_key uses self.index if index=None
  • batch_size param added to get_metadata_values_by_key to controll efficient paging

How did you test it?

  • existing tests
  • additional test for batch_size

Notes for the reviewer

Checklist

@tstadel tstadel requested a review from a team as a code owner April 20, 2024 09:43
@tstadel tstadel requested review from vblagoje and removed request for a team April 20, 2024 09:43
@tstadel tstadel added this to the 1.25.3 milestone Apr 20, 2024
@github-actions github-actions bot added topic:tests type:documentation Improvements on the docs labels Apr 20, 2024
Copy link
Member

@vblagoje vblagoje left a comment

Choose a reason for hiding this comment

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

Looks good, would you please generate schema and verify if this change will be reflected there properly, just to make sure we don't get any hiccups when releasing :-)

@tstadel
Copy link
Member Author

tstadel commented Apr 22, 2024

Looks good, would you please generate schema and verify if this change will be reflected there properly, just to make sure we don't get any hiccups when releasing :-)

@vblagoje this change doesn't make schema changes. We only add the param to method get_metadata_values_by_key but not to the component's init.

@vblagoje vblagoje merged commit 28eda4b into v1.x Apr 22, 2024
49 checks passed
@vblagoje vblagoje deleted the fix/get_metadata_values_by_key_use_docstore_index branch April 22, 2024 14:29
vblagoje pushed a commit that referenced this pull request Apr 23, 2024
…elasticsearch) (#7562)

* fix: use docstore.index for get_metadata_values_by_key (OpenSearch / elasticsearch)

* add batch_size param
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants