feat(mongodb_atlas): support custom schema mapping via meta_project_mapping#3424
feat(mongodb_atlas): support custom schema mapping via meta_project_mapping#3424PranavSShetty wants to merge 6 commits into
Conversation
…asDocumentStore schema mapping
…s for strict type checking
|
|
|
Heads-up for maintainers This PR is from a fork and touches integrations whose integration tests require API keys. Affected integrations:
Please run the integration tests locally ( |
github-actions
Bot
added
the
type:documentation
Coverage report (mongodb_atlas)
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
bogdankostic
left a comment
bogdankostic
left a comment
There was a problem hiding this comment.
Thanks @PranavSShetty! I added a few in-line comments that should be addressed.
|
|
||
| if self.meta_project_mapping: | ||
| meta = mongo_doc.setdefault("meta", {}) | ||
| for meta_key, mongo_field in self.meta_project_mapping.items(): | ||
| val = self._pop_nested_value(mongo_doc, mongo_field) | ||
| if val is not None: | ||
| meta[meta_key] = val |
There was a problem hiding this comment.
This raises ValueError when the stored doc has any root-level field that isn't a Haystack Document field and isn't fully consumed by the mapping, as long as at least one mapped field populated meta. Document.from_dict rejects receiving both a non-empty meta and "flattened" top-level keys:
store = MongoDBAtlasDocumentStore(..., meta_project_mapping={"source": "source"})
mongo_doc = {"id": "x1", "content": "hello", "source": "url", "category": "news", "meta": {}}
store._mongo_doc_to_haystack_doc(mongo_doc)
# ValueError: You can pass either the 'meta' parameter or flattened metadata keys ...| def _get_nested_value(self, doc: dict[str, Any], path: str) -> Any: | ||
| if path.startswith("$"): | ||
| path = path[1:] | ||
| parts = path.split(".") | ||
| val = doc | ||
| for part in parts: | ||
| if isinstance(val, dict) and part in val: | ||
| val = val[part] | ||
| else: | ||
| return None | ||
| return val | ||
|
|
||
| def _set_nested_value(self, doc: dict[str, Any], path: str, value: Any) -> None: | ||
| if path.startswith("$"): | ||
| path = path[1:] | ||
| parts = path.split(".") | ||
| val = doc | ||
| for part in parts[:-1]: | ||
| if part not in val or not isinstance(val[part], dict): | ||
| val[part] = {} | ||
| val = val[part] | ||
| val[parts[-1]] = value | ||
|
|
||
| def _pop_nested_value(self, doc: dict[str, Any], path: str) -> Any: | ||
| if path.startswith("$"): | ||
| path = path[1:] | ||
| parts = path.split(".") | ||
|
|
||
| def rec_pop(d: dict[str, Any], path_parts: list[str]) -> tuple[Any, bool]: | ||
| if not path_parts: | ||
| return None, False | ||
| key = path_parts[0] | ||
| if len(path_parts) == 1: | ||
| if key in d: | ||
| val = d.pop(key) | ||
| return val, len(d) == 0 | ||
| return None, False | ||
|
|
||
| if key in d and isinstance(d[key], dict): | ||
| val, should_delete_parent = rec_pop(d[key], path_parts[1:]) | ||
| if should_delete_parent: | ||
| d.pop(key) | ||
| return val, len(d) == 0 | ||
| return None, False |
There was a problem hiding this comment.
These should be static methods as they don't make use of self.
| self.full_text_search_index = full_text_search_index | ||
| self.embedding_field = embedding_field | ||
| self.content_field = content_field | ||
| self.meta_project_mapping = meta_project_mapping |
There was a problem hiding this comment.
The mapping values get a leading $ stripped in ~6 places, while the aggregation builders conditionally prepend one. Prepending $ where an aggregation expression references a field value is correct and necessary. But accepting values that already start with $ — and therefore stripping it everywhere else — looks like an avoidable convenience that's leaking complexity across the file.
Could we instead define mapping values as bare field paths (no $), document that in this param, and normalize once here (e.g. strip a leading $ when storing self.meta_project_mapping)? Then every downstream site can assume a clean field name, the aggregation builders are the only place that adds $, and the five startswith("$") strips go away.
There was a problem hiding this comment.
Thanks for the review and the detailed feedback. I've gone through the comments and I agree there are a few issues that need to be addressed, particularly the "Document.from_dict" case and the mapping normalization suggestions.
I'm currently unavailable for the next week, but I'll work through the feedback and submit an updated revision as soon as I'm back.
Thanks again for taking the time to review this.
3 participants
Related Issues
Proposed Changes:
This PR adds support for custom MongoDB schema mappings in MongoDBAtlasDocumentStore through a new optional meta_project_mapping configuration parameter.
When provided, keys in the Haystack Document meta dictionary can be mapped to specific root or nested MongoDB document fields.
Key additions:
Nested field helper utilities
Filter translation support
Document conversion enhancements
Metadata and aggregation support
Serialization support
Test Results:
80 passed, 133 skipped
Backward compatibility has been preserved.
Existing behaviour remains unchanged when meta_project_mapping is not provided.