Skip to content

feat: adding new operations to ValkeyDocumentStore#2910

Open
davidsbatista wants to merge 4 commits intomainfrom
feat/add-more-operations-valkey
Open

feat: adding new operations to ValkeyDocumentStore#2910
davidsbatista wants to merge 4 commits intomainfrom
feat/add-more-operations-valkey

Conversation

@davidsbatista
Copy link
Contributor

@davidsbatista davidsbatista commented Mar 4, 2026

Related Issues

Proposed Changes:

Adding the following new operations, both sync and async:

  • count_unique_metadata_by_filter
  • count_documents_by_filter
  • delete_by_filter
  • get_metadata_field_min_max
  • get_metadata_fields_info
  • update_by_filter

How did you test it?

  • new sync and async tests for each

Checklist

@github-actions github-actions bot added integration:valkey type:documentation Improvements or additions to documentation labels Mar 4, 2026
@davidsbatista davidsbatista marked this pull request as ready for review March 5, 2026 10:28
@davidsbatista davidsbatista requested a review from a team as a code owner March 5, 2026 10:28
@davidsbatista davidsbatista requested review from anakin87 and removed request for a team March 5, 2026 10:28
Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

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

Good work overall.

I left some comments and questions.

try:
docs_to_update = self.filter_documents(filters)
for doc in docs_to_update:
doc.meta = dict(doc.meta or {})
Copy link
Member

Choose a reason for hiding this comment

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

this triggers a warning due to deepset-ai/haystack#10650
let's find a way to avoid mutating the attribute in-place

Warning: Mutating attribute 'meta' on an instance of 'Document' can lead to unexpected behavior by affecting other parts of the pipeline that use the same dataclass instance. Use dataclasses.replace(instance, meta=new_value) instead. See https://docs.haystack.deepset.ai/docs/custom-components#requirements for details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch!

try:
docs_to_update = await self.filter_documents_async(filters)
for doc in docs_to_update:
doc.meta = dict(doc.meta or {})
Copy link
Member

Choose a reason for hiding this comment

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

same issue here (in-place mutation)

Comment on lines +1409 to +1414
@staticmethod
def _get_doc_meta_value(doc: Document, field_name: str) -> Any:
"""Get metadata value from a document; field_name can be 'category', 'meta.category', or 'meta_category'."""
key = ValkeyDocumentStore._normalize_metadata_field_name(field_name)
meta_key = key[5:] if key.startswith("meta_") else key # key for doc.meta
return (doc.meta or {}).get(meta_key)
Copy link
Member

Choose a reason for hiding this comment

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

since we already normalize fields names, is this method needed?

counts: dict[str, int] = {}
for field_name in metadata_fields:
meta_key = ValkeyDocumentStore._normalize_metadata_field_name(field_name)
user_name = meta_key[5:] if meta_key.startswith("meta_") else meta_key
Copy link
Member

Choose a reason for hiding this comment

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

nit: user_name seems a bit misleading variable name

assert doc.meta["score"] >= 0.7

# --- delete_by_filter, update_by_filter, count_documents_by_filter ---

Copy link
Member

Choose a reason for hiding this comment

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

Here in tests, could we use the tests you added in haystack/testing/document_store.py?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration:valkey type:documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants