Skip to content

fix(backends): wrap bare-str OneOrMany inputs before embedding - #1707

Merged
igorls merged 2 commits into
developfrom
fix/embedding-wrapper-oneormany
Jun 6, 2026
Merged

fix(backends): wrap bare-str OneOrMany inputs before embedding#1707
igorls merged 2 commits into
developfrom
fix/embedding-wrapper-oneormany

Conversation

@igorls

@igorls igorls commented Jun 6, 2026

Copy link
Copy Markdown
Member

Fixes the one real correctness bug from the #1706 review (Gemini + Copilot, HIGH).

EmbeddingCollection did _embed_texts(list(documents)). ChromaDB's OneOrMany allows a single document as a bare str — and list("abc")['a','b','c'], so each character got embedded, producing the wrong number of vectors and a length mismatch vs ids/metadatas on the explicit-vector backends (pgvector, sqlite_exact).

Fix: a _as_list() helper normalizes str[str] (and leaves real sequences alone), applied at all four sites — documents in add/upsert/update and query_texts in query — and the normalized list is passed to the inner backend too, so it's length-aligned with ids.

Tests: tests/test_embedding_wrapper.py — bare-string add/upsert/update/query each embed one whole document, the backend receives a list, and list inputs are unaffected. Backend conformance + pgvector + sqlite_exact suites still pass.

Note: the other two "HIGH" findings on #1706 were false positivesmigrate.py imports tempfile at module level, and collision_scan.py's GetResult does implement __getitem__ (via _DictCompatMixin). The Qdrant text_any finding is an opt-in perf concern (fallback scan), tracked separately. The default chroma backend is unaffected by any of these.

EmbeddingCollection did _embed_texts(list(documents)). For ChromaDB's
OneOrMany shape, a bare str document splits into per-character 'docs'
(list("abc") -> ['a','b','c']), embedding each character and breaking
length alignment with ids/metadatas on explicit-vector backends
(pgvector, sqlite_exact). Normalize str -> [str] via _as_list() at all
four sites (add/upsert/update documents, query query_texts) and pass the
normalized list to the inner backend too. Addresses PR #1706 review
(Gemini + Copilot, HIGH).
@igorls
igorls requested a review from milla-jovovich as a code owner June 6, 2026 07:35
Copilot AI review requested due to automatic review settings June 6, 2026 07:35

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a helper function _as_list to normalize ChromaDB's OneOrMany shape (specifically handling bare strings) to prevent character-by-character iteration and ensure length alignment with IDs and metadata. This helper is integrated into the add, upsert, query, and update methods of EmbeddingCollection, and corresponding unit tests are added. The review feedback suggests extending _as_list to also wrap single dictionaries (which can be passed as metadata) to prevent data loss when calling list(), and adding tests to verify the wrapping of bare string IDs and dictionary metadata.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +21 to +31
def _as_list(value):
"""Normalize ChromaDB's ``OneOrMany`` shape (``str`` | sequence) to a list.

A bare ``str`` must be *wrapped*, not iterated: ``list("abc")`` yields
``['a', 'b', 'c']``, which would embed per character and break length
alignment with ``ids``/``metadatas`` on explicit-vector backends
(pgvector, sqlite_exact). See PR #1706 review.
"""
if isinstance(value, str):
return [value]
return list(value)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Currently, _as_list only handles str by wrapping it in a list. However, ChromaDB's OneOrMany shape also allows a single dictionary for metadatas (e.g., {"source": "web"}). If a single dictionary is passed, calling list(value) on it will extract only its keys (e.g., ["source"]), which leads to data loss and schema mismatch.

By extending _as_list to also check for dict, we can safely wrap single metadata dictionaries as well.

Suggested change
def _as_list(value):
"""Normalize ChromaDB's ``OneOrMany`` shape (``str`` | sequence) to a list.
A bare ``str`` must be *wrapped*, not iterated: ``list("abc")`` yields
``['a', 'b', 'c']``, which would embed per character and break length
alignment with ``ids``/``metadatas`` on explicit-vector backends
(pgvector, sqlite_exact). See PR #1706 review.
"""
if isinstance(value, str):
return [value]
return list(value)
def _as_list(value):
"""Normalize ChromaDB's ``OneOrMany`` shape (``str`` | ``dict`` | sequence) to a list.
A bare ``str`` or ``dict`` must be *wrapped*, not iterated: ``list("abc")`` yields
``['a', 'b', 'c']``, and ``list({"a": 1})`` yields ``['a']``, which would break
length alignment with ``ids``/``metadatas`` on explicit-vector backends
(pgvector, sqlite_exact). See PR #1706 review.
"""
if isinstance(value, (str, dict)):
return [value]
return list(value)

Comment on lines +90 to +95
def test_list_inputs_unaffected(monkeypatch):
seen = _patch_embed(monkeypatch)
inner = _FakeInner()
ew.EmbeddingCollection(inner).add(documents=["one", "two"], ids=["a", "b"])
assert seen["texts"] == ["one", "two"]
assert len(inner.calls["add"]["embeddings"]) == 2

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Let's add a unit test to verify that bare string ids and single dictionary metadatas are correctly wrapped and normalized before delegating to the inner backend.

Suggested change
def test_list_inputs_unaffected(monkeypatch):
seen = _patch_embed(monkeypatch)
inner = _FakeInner()
ew.EmbeddingCollection(inner).add(documents=["one", "two"], ids=["a", "b"])
assert seen["texts"] == ["one", "two"]
assert len(inner.calls["add"]["embeddings"]) == 2
def test_list_inputs_unaffected(monkeypatch):
seen = _patch_embed(monkeypatch)
inner = _FakeInner()
ew.EmbeddingCollection(inner).add(documents=["one", "two"], ids=["a", "b"])
assert seen["texts"] == ["one", "two"]
assert len(inner.calls["add"]["embeddings"]) == 2
def test_add_wraps_bare_string_ids_and_dict_metadatas(monkeypatch):
seen = _patch_embed(monkeypatch)
inner = _FakeInner()
ew.EmbeddingCollection(inner).add(
documents="hello world",
ids="d1",
metadatas={"source": "web"}
)
assert inner.calls["add"]["ids"] == ["d1"]
assert inner.calls["add"]["metadatas"] == [{"source": "web"}]

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes a correctness issue in the explicit-embedding wrapper where ChromaDB-style OneOrMany inputs could be passed as a bare str and inadvertently treated as an iterable of characters, producing the wrong number of embeddings and misaligning documents vs ids/metadatas for explicit-vector backends (e.g., pgvector, sqlite_exact).

Changes:

  • Add _as_list() normalization to wrap bare str inputs as [str] and apply it to documents (add/upsert/update) and query_texts (query).
  • Ensure normalized lists are forwarded to the inner backend so batch lengths remain aligned.
  • Add a new test suite verifying bare-string add/upsert/update/query behavior and that list inputs remain unaffected.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
mempalace/backends/embedding_wrapper.py Introduces _as_list() and uses it to prevent per-character embedding when inputs are a bare string.
tests/test_embedding_wrapper.py Adds tests covering bare-string and list input behavior for the embedding wrapper.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread mempalace/backends/embedding_wrapper.py Outdated
Comment on lines +29 to +31
if isinstance(value, str):
return [value]
return list(value)
):
if query_texts is not None and query_embeddings is None:
query_embeddings = _embed_texts(list(query_texts))
query_embeddings = _embed_texts(_as_list(query_texts))
Address #1707 review:
- _as_list also wraps a bare dict (single metadata) — list({'k':1}) -> ['k']
  would drop the values — and returns list inputs as-is (no copy, Copilot
  perf note); other iterables are materialized once.
- Normalize ids and metadatas (not just documents) in add/upsert/update so
  a scalar id/metadata stays length-aligned with documents/embeddings.
- Widen query_texts annotation to list[str] | str to match the behavior.
- Tests: bare-str ids + dict metadatas, dict wrapping, list-returned-as-is.
@igorls
igorls merged commit 72a69e2 into develop Jun 6, 2026
8 checks passed
@igorls
igorls deleted the fix/embedding-wrapper-oneormany branch June 6, 2026 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants