mirror of
https://github.com/semantica-agi/semantica.git
synced 2026-08-29 04:26:20 +00:00
fix: address code review findings in backend metadata filtering
- pinecone_store: call self.index.describe_index_stats() instead of the nonexistent self.describe_index_stats(), and use a unit query vector instead of an all-zero vector so filter_by_metadata() works on cosine-metric indexes (the library's own default) - pgvector_store: apply the existing lowercase true/false bool handling to the list-filter branch too, and use the jsonb ?| operator so list-valued metadata fields match on intersection instead of being compared as a single JSON-text blob - sqlite_vec_store: use json_each() with a json_type guard so list-valued metadata fields match on intersection, mirroring the in-memory backend's set-intersection semantics - faiss_store: filter_by_metadata(limit=0) now returns [] instead of one result - milvus_store: reject NaN/Infinity filter values up front with a clear ValidationError instead of building an invalid expression that gets silently swallowed - update the #848 FAISS NotImplementedError test to reflect that FAISS now implements real filter_by_metadata() (this PR's whole point) - add regression tests for each fix; sqlite tests run against the real sqlite-vec extension
This commit is contained in:
@@ -9,6 +9,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Fixed
|
||||
|
||||
- **`VectorStore._filter_by_metadata()` `AttributeError` on all persistent backends** (#857, closes #849) by @TaherTadpatri
|
||||
- `_filter_by_metadata()` iterated `self.metadata` directly, which only exists on the `inmemory` backend — any persistent backend (`faiss`, `qdrant`, `pinecone`, `milvus`, `pgvector`, `sqlite`, `weaviate`) crashed with `AttributeError` on `filter_decisions(query=None, ...)` / metadata-only filtering. Filtering is now delegated to a native `filter_by_metadata()` implemented on each backend store, using backend-native payload/SQL/JSON filtering (Qdrant `scroll()`, Pinecone `query()`, Milvus expression filters, PostgreSQL JSONB, SQLite `json_extract()`, Weaviate collection filters)
|
||||
- **Fixed along the way**: `PineconeStore.get_index()` and `filter_by_metadata()` called a nonexistent `self.describe_index_stats()` on the store itself (the method only exists on the `PineconeIndex` wrapper returned by `self.index`); the resulting `AttributeError` was silently swallowed, so dimension auto-detection always failed quietly. Now correctly calls `self.index.describe_index_stats()`
|
||||
- **Fixed along the way**: `PineconeStore.filter_by_metadata()` probed for filter-only matches using an all-zero dummy query vector, which Pinecone rejects for cosine-metric indexes — the library's own default — making metadata-only filtering silently non-functional out of the box. Now uses a unit vector instead
|
||||
- **Fixed along the way**: `PgVectorStore.filter_by_metadata()`'s list-filter branch formatted boolean values with `str(v)` (`'True'`/`'False'`), never matching PostgreSQL JSONB's lowercase `'true'`/`'false'` text rendering, even though the equivalent scalar-filter branch already handled this correctly
|
||||
- **Fixed along the way**: list-valued metadata fields (e.g. `{"tags": ["python", "js"]}`) could never match a list filter on the SQLite or PostgreSQL backends, because both extracted the whole array as its JSON/text representation instead of matching individual elements — silently diverging from the in-memory backend's set-intersection semantics. SQLite now uses `json_each()` over a `json_type`-guarded array/scalar wrapper; PostgreSQL now uses the `?|` "any array element" operator alongside the existing scalar `= ANY(...)` path
|
||||
- **Fixed along the way**: `FAISSStore.filter_by_metadata(limit=0)` returned one result instead of zero, because the limit check ran after appending the current match
|
||||
- **Fixed along the way**: `MilvusStore`'s metadata expression builder rendered `NaN`/`Infinity` filter values as bare unquoted tokens, producing an invalid Milvus expression whose server-side rejection was then swallowed by a broad `except`, indistinguishable from "no matches"; these values are now rejected up front with a clear `ValidationError`
|
||||
- New/expanded test coverage in `tests/vector_store/test_backend_metadata_filtering.py` (all 7 backends, including the Pinecone dimension/zero-vector, PgVector boolean-list, FAISS `limit=0`, and Milvus `NaN` regressions) and `tests/vector_store/test_sqlite_vec_store.py` (new `TestSQLiteVecStoreFilterByMetadata`, run against the real `sqlite-vec` extension, including the array-vs-scalar intersection case)
|
||||
|
||||
## [0.6.5] - 2026-08-11
|
||||
|
||||
### Added
|
||||
|
||||
@@ -508,6 +508,9 @@ class FAISSStore:
|
||||
|
||||
from .vector_store import _matches_filter
|
||||
|
||||
if limit <= 0:
|
||||
return []
|
||||
|
||||
results = []
|
||||
for vector_id, metadata in self.index.metadata.items():
|
||||
if _matches_filter(metadata, filters):
|
||||
|
||||
@@ -35,6 +35,7 @@ Author: Semantica Contributors
|
||||
License: MIT
|
||||
"""
|
||||
|
||||
import math
|
||||
import re
|
||||
from typing import Any, Dict, List, Optional, Union
|
||||
|
||||
@@ -57,6 +58,11 @@ def _format_milvus_value(val: Any) -> str:
|
||||
if isinstance(val, bool):
|
||||
return "true" if val else "false"
|
||||
elif isinstance(val, (int, float)):
|
||||
if isinstance(val, float) and not math.isfinite(val):
|
||||
raise ValidationError(
|
||||
f"Invalid metadata filter value: {val!r}. NaN/Infinity are not "
|
||||
"valid Milvus expression literals."
|
||||
)
|
||||
return str(val)
|
||||
elif isinstance(val, str):
|
||||
escaped = (
|
||||
|
||||
@@ -697,10 +697,25 @@ class PgVectorStore:
|
||||
))
|
||||
filter_values.append(value["max"])
|
||||
elif isinstance(value, list):
|
||||
filter_conditions.append(psycopg_sql.SQL("metadata->>{} = ANY(%s)").format(
|
||||
psycopg_sql.Literal(key)
|
||||
))
|
||||
filter_values.append([str(v) for v in value])
|
||||
# Same lowercase-bool rule as the scalar branch below: ->> renders
|
||||
# JSON booleans as 'true'/'false', not str()'s 'True'/'False'.
|
||||
str_values = [
|
||||
('true' if v else 'false') if isinstance(v, bool) else str(v)
|
||||
for v in value
|
||||
]
|
||||
# If the metadata value at this key is itself a JSON array, match on
|
||||
# intersection (mirrors the in-memory backend's set-intersection
|
||||
# semantics) via the jsonb `?|` "any array element matches" operator;
|
||||
# otherwise fall back to plain scalar membership. `->>` renders an
|
||||
# array as its whole text representation, so it cannot be reused for
|
||||
# the array case.
|
||||
filter_conditions.append(psycopg_sql.SQL(
|
||||
"(CASE WHEN jsonb_typeof(metadata->{0}) = 'array' "
|
||||
"THEN metadata->{0} ?| %s "
|
||||
"ELSE metadata->>{0} = ANY(%s) END)"
|
||||
).format(psycopg_sql.Literal(key)))
|
||||
filter_values.append(str_values)
|
||||
filter_values.append(str_values)
|
||||
elif isinstance(value, bool):
|
||||
# PostgreSQL JSONB ->> returns lowercase 'true'/'false' for JSON booleans.
|
||||
# str(True)='True' and str(False)='False' would never match; use the
|
||||
|
||||
@@ -435,7 +435,7 @@ class PineconeStore:
|
||||
self.search_engine = PineconeSearch(self.index)
|
||||
if self.dimension is None:
|
||||
try:
|
||||
stats = self.describe_index_stats()
|
||||
stats = self.index.describe_index_stats()
|
||||
if stats and isinstance(stats, dict) and stats.get("dimension"):
|
||||
self.dimension = int(stats["dimension"])
|
||||
except Exception as e:
|
||||
@@ -676,7 +676,7 @@ class PineconeStore:
|
||||
dimension = self.dimension
|
||||
if dimension is None:
|
||||
try:
|
||||
stats = self.describe_index_stats()
|
||||
stats = self.index.describe_index_stats()
|
||||
if stats and isinstance(stats, dict) and stats.get("dimension"):
|
||||
dimension = int(stats["dimension"])
|
||||
self.dimension = dimension
|
||||
@@ -705,7 +705,12 @@ class PineconeStore:
|
||||
else:
|
||||
pinecone_filter[key] = value
|
||||
|
||||
dummy_vector = [0.0] * dimension
|
||||
# A literal zero vector is rejected by Pinecone for cosine-metric indexes
|
||||
# ("Query vector must not be the zero vector"). Use a unit vector instead so
|
||||
# this works regardless of the index's distance metric; since this call only
|
||||
# cares about which vectors match `filter`, not similarity ranking, any
|
||||
# fixed non-zero query vector is an equally valid probe.
|
||||
dummy_vector = [1.0 / (dimension ** 0.5)] * dimension
|
||||
|
||||
try:
|
||||
response = self.index.index.query(
|
||||
|
||||
@@ -648,8 +648,21 @@ class SQLiteVecStore:
|
||||
filter_conditions.append(f"CAST(json_extract(metadata, '$.{key}') AS NUMERIC) <= ?")
|
||||
filter_params.append(value["max"])
|
||||
elif isinstance(value, list):
|
||||
# If the metadata value at this key is itself a JSON array, match on
|
||||
# intersection (mirrors the in-memory backend's set-intersection
|
||||
# semantics); otherwise fall back to plain scalar membership. Both
|
||||
# cases are handled uniformly via json_each: a non-array value is
|
||||
# wrapped in a one-element array first so json_each always sees a
|
||||
# valid JSON array to iterate.
|
||||
placeholders = ", ".join(["?"] * len(value))
|
||||
filter_conditions.append(f"json_extract(metadata, '$.{key}') IN ({placeholders})")
|
||||
filter_conditions.append(
|
||||
f"EXISTS (SELECT 1 FROM json_each("
|
||||
f" CASE WHEN json_type(metadata, '$.{key}') = 'array'"
|
||||
f" THEN json_extract(metadata, '$.{key}')"
|
||||
f" ELSE json_array(json_extract(metadata, '$.{key}'))"
|
||||
f" END"
|
||||
f") je WHERE je.value IN ({placeholders}))"
|
||||
)
|
||||
filter_params.extend([str(v) if not isinstance(v, (int, float, bool)) else v for v in value])
|
||||
else:
|
||||
filter_conditions.append(f"json_extract(metadata, '$.{key}') = ?")
|
||||
|
||||
@@ -156,8 +156,8 @@ class TestBackendMetadataFiltering(unittest.TestCase):
|
||||
def test_pinecone_store_filter_by_metadata_unknown_dimension_raises(self):
|
||||
store = PineconeStore()
|
||||
mock_index_wrapper = MagicMock()
|
||||
mock_index_wrapper.describe_index_stats = MagicMock(return_value={})
|
||||
store.index = mock_index_wrapper
|
||||
store.describe_index_stats = MagicMock(return_value={})
|
||||
with self.assertRaises(ProcessingError):
|
||||
store.filter_by_metadata({"status": "active"}, limit=5)
|
||||
|
||||
@@ -314,6 +314,90 @@ class TestBackendMetadataFiltering(unittest.TestCase):
|
||||
self.assertNotIn('False', params_passed,
|
||||
"str(False)='False' must NOT appear in SQL params")
|
||||
|
||||
@patch('semantica.vector_store.pgvector_store.PSYCOPG3_AVAILABLE', True)
|
||||
@patch('semantica.vector_store.pgvector_store.psycopg_sql')
|
||||
def test_pgvector_store_filter_by_metadata_bool_list(self, mock_sql):
|
||||
"""List-valued boolean filters must use lowercase 'true'/'false', not
|
||||
str(True)/str(False), matching the scalar branch's handling.
|
||||
"""
|
||||
store = object.__new__(PgVectorStore)
|
||||
store.table_name = "test_vectors"
|
||||
store._is_safe_identifier = lambda k: True
|
||||
|
||||
mock_conn = MagicMock()
|
||||
mock_cur = MagicMock()
|
||||
mock_cur.fetchall.return_value = [
|
||||
("pg4", [0.7, 0.8], {"active": True})
|
||||
]
|
||||
mock_conn.cursor.return_value = mock_cur
|
||||
|
||||
with patch.object(
|
||||
PgVectorStore,
|
||||
'_get_connection',
|
||||
return_value=MagicMock(
|
||||
__enter__=MagicMock(return_value=mock_conn),
|
||||
__exit__=MagicMock(),
|
||||
),
|
||||
):
|
||||
results = store.filter_by_metadata({"active": [True, False]}, limit=10)
|
||||
|
||||
self.assertEqual(len(results), 1)
|
||||
execute_call_args = mock_cur.execute.call_args
|
||||
params_passed = execute_call_args[0][1]
|
||||
flat_params = [v for p in params_passed for v in (p if isinstance(p, list) else [p])]
|
||||
self.assertIn('true', flat_params)
|
||||
self.assertIn('false', flat_params)
|
||||
self.assertNotIn('True', flat_params)
|
||||
self.assertNotIn('False', flat_params)
|
||||
|
||||
def test_faiss_store_filter_by_metadata_limit_zero(self):
|
||||
"""limit=0 must return no results, not the first match."""
|
||||
store = FAISSStore(dimension=2)
|
||||
mock_index = MagicMock()
|
||||
mock_index.metadata = {
|
||||
"v1": {"category": "finance", "score": 10},
|
||||
}
|
||||
mock_index.get_vector.return_value = np.array([1.0, 0.0])
|
||||
store.index = mock_index
|
||||
|
||||
results = store.filter_by_metadata({"category": "finance"}, limit=0)
|
||||
self.assertEqual(results, [])
|
||||
|
||||
@patch('semantica.vector_store.milvus_store.MILVUS_AVAILABLE', True)
|
||||
def test_milvus_store_filter_by_metadata_nan_raises(self):
|
||||
"""NaN/Infinity are not valid Milvus expression literals and must be
|
||||
rejected up front rather than silently producing an invalid expression
|
||||
that gets swallowed by the broad except around the query() call.
|
||||
"""
|
||||
store = MilvusStore()
|
||||
mock_coll_wrapper = MagicMock()
|
||||
store.collection = mock_coll_wrapper
|
||||
|
||||
with self.assertRaises(ValidationError):
|
||||
store.filter_by_metadata({"score": {"min": float("nan")}}, limit=5)
|
||||
|
||||
@patch('semantica.vector_store.pinecone_store.PINECONE_AVAILABLE', True)
|
||||
def test_pinecone_store_get_index_sets_dimension_from_stats(self):
|
||||
"""get_index() must read stats from the returned PineconeIndex wrapper
|
||||
(self.index), not from a nonexistent method on the store itself.
|
||||
"""
|
||||
store = PineconeStore()
|
||||
mock_client = MagicMock()
|
||||
mock_pinecone_index = MagicMock()
|
||||
mock_client.get_index.return_value = mock_pinecone_index
|
||||
store.client = mock_client
|
||||
|
||||
with patch(
|
||||
'semantica.vector_store.pinecone_store.PineconeIndex'
|
||||
) as mock_index_cls:
|
||||
mock_index_instance = MagicMock()
|
||||
mock_index_instance.describe_index_stats.return_value = {"dimension": 42}
|
||||
mock_index_cls.return_value = mock_index_instance
|
||||
|
||||
store.get_index("my-index")
|
||||
|
||||
self.assertEqual(store.dimension, 42)
|
||||
|
||||
def test_weaviate_store_filter_by_metadata(self):
|
||||
store = WeaviateStore()
|
||||
mock_coll = MagicMock()
|
||||
|
||||
@@ -773,19 +773,21 @@ class TestBuildDecisionContextFAISSBackend:
|
||||
|
||||
class TestFilterByMetadataBackendBehavior:
|
||||
"""
|
||||
Requirement (issue #848 follow-up): verify the chosen behavior of
|
||||
Requirement (issue #848, superseded by #857): verify the behavior of
|
||||
_filter_by_metadata when a non-inmemory backend is active.
|
||||
|
||||
The decision: raise NotImplementedError (matching get_vector / get_metadata
|
||||
from #843) rather than silently returning [].
|
||||
#848's original decision was to raise NotImplementedError (matching
|
||||
get_vector / get_metadata from #843) rather than silently return [],
|
||||
because at the time zero backend wrappers implemented
|
||||
filter_by_metadata(filters, limit).
|
||||
|
||||
Rationale documented in the production comment:
|
||||
- Zero backend wrappers implement filter_by_metadata(filters, limit).
|
||||
- The only codebase hit (HybridSearch.filter_by_metadata) has a completely
|
||||
different signature and is never stored in _backend_store.
|
||||
- Returning [] would make filter_decisions(query=None, category="loan")
|
||||
report "zero matches" when the truth is "capability not available" —
|
||||
indistinguishable from a real empty result and therefore wrong.
|
||||
#857 gave every persistent backend (FAISS, Qdrant, Pinecone, Milvus,
|
||||
PgVector, SQLiteVec, Weaviate) a real filter_by_metadata() implementation,
|
||||
so FAISS-backed filter_decisions(query=None, ...) now returns actual
|
||||
filtered results instead of raising. The NotImplementedError path itself
|
||||
is still correct and still covered (see
|
||||
test_filter_by_metadata_backend_not_implemented in test_vector_store.py)
|
||||
for a backend that genuinely lacks the method.
|
||||
"""
|
||||
|
||||
def _make_faiss_store(self):
|
||||
@@ -809,34 +811,26 @@ class TestFilterByMetadataBackendBehavior:
|
||||
)
|
||||
return vs, ids
|
||||
|
||||
# ── FAISS backend: NotImplementedError, not AttributeError, not [] ── #
|
||||
# ── FAISS backend: real results, not AttributeError, not [] ── #
|
||||
|
||||
def test_filter_by_metadata_faiss_raises_not_implemented(self):
|
||||
def test_filter_by_metadata_faiss_returns_real_results(self):
|
||||
"""
|
||||
filter_decisions(query=None, category='loan') on a FAISS-backed store
|
||||
must raise NotImplementedError, not AttributeError (old crash) and not
|
||||
silently return [] (the wrong silent-failure fix).
|
||||
|
||||
This test pins the chosen behavior: explicit NotImplementedError matching
|
||||
the get_vector/get_metadata precedent set by issue #843.
|
||||
must return the actual matching decisions, not raise AttributeError
|
||||
(old crash) and not silently return [] (the old NotImplementedError
|
||||
stand-in from #848, superseded once #857 gave FAISSStore a real
|
||||
filter_by_metadata()).
|
||||
"""
|
||||
vs, _ids = self._make_faiss_store()
|
||||
|
||||
with pytest.raises(NotImplementedError) as exc_info:
|
||||
vs.filter_decisions(query=None, category="loan")
|
||||
results = vs.filter_decisions(query=None, category="loan")
|
||||
|
||||
# Message must name the backend and point to the correct alternative
|
||||
msg = str(exc_info.value)
|
||||
assert "FAISSStore" in msg, (
|
||||
f"Error message should name the backend class, got: {msg!r}"
|
||||
)
|
||||
assert "filter_decisions" in msg or "filter_by_metadata" in msg, (
|
||||
f"Error message should mention the failing method, got: {msg!r}"
|
||||
)
|
||||
assert "search_decisions" in msg, (
|
||||
f"Error message should suggest search_decisions() as the alternative, "
|
||||
f"got: {msg!r}"
|
||||
assert isinstance(results, list)
|
||||
assert len(results) == 2, (
|
||||
f"Expected 2 loan decisions, got {len(results)}: {results}"
|
||||
)
|
||||
for r in results:
|
||||
assert r["metadata"]["category"] == "loan"
|
||||
|
||||
def test_filter_by_metadata_faiss_not_attribute_error(self):
|
||||
"""
|
||||
|
||||
@@ -413,3 +413,51 @@ class TestSQLiteVecStoreStats:
|
||||
|
||||
stats = store.get_stats()
|
||||
assert stats["vector_count"] == 4
|
||||
|
||||
|
||||
class TestSQLiteVecStoreFilterByMetadata:
|
||||
"""Test filter_by_metadata, including list-valued metadata handling."""
|
||||
|
||||
def test_filter_exact_match(self, store):
|
||||
vectors = [np.random.rand(128).astype(np.float32) for _ in range(2)]
|
||||
metadata = [{"category": "finance"}, {"category": "tech"}]
|
||||
ids = store.add(vectors, metadata, ids=["v1", "v2"])
|
||||
|
||||
results = store.filter_by_metadata({"category": "finance"}, limit=10)
|
||||
|
||||
assert [r["id"] for r in results] == ["v1"]
|
||||
|
||||
def test_filter_scalar_field_against_list_filter(self, store):
|
||||
"""A scalar metadata value should match via plain IN-list membership."""
|
||||
vectors = [np.random.rand(128).astype(np.float32) for _ in range(2)]
|
||||
metadata = [{"category": "finance"}, {"category": "tech"}]
|
||||
store.add(vectors, metadata, ids=["v1", "v2"])
|
||||
|
||||
results = store.filter_by_metadata({"category": ["finance", "ops"]}, limit=10)
|
||||
|
||||
assert [r["id"] for r in results] == ["v1"]
|
||||
|
||||
def test_filter_array_field_intersects_list_filter(self, store):
|
||||
"""A list-valued metadata field must match on set intersection with the
|
||||
filter list, mirroring the in-memory backend's semantics -- not on a
|
||||
literal comparison of the whole array's JSON text against each candidate.
|
||||
"""
|
||||
vectors = [np.random.rand(128).astype(np.float32) for _ in range(3)]
|
||||
metadata = [
|
||||
{"tags": ["python", "js"]},
|
||||
{"tags": ["go"]},
|
||||
{"tags": ["python", "ml"]},
|
||||
]
|
||||
store.add(vectors, metadata, ids=["v1", "v2", "v3"])
|
||||
|
||||
results = store.filter_by_metadata({"tags": ["python", "ml"]}, limit=10)
|
||||
|
||||
assert {r["id"] for r in results} == {"v1", "v3"}
|
||||
|
||||
def test_filter_limit_zero_returns_empty(self, store):
|
||||
vectors = [np.random.rand(128).astype(np.float32)]
|
||||
store.add(vectors, [{"category": "finance"}], ids=["v1"])
|
||||
|
||||
results = store.filter_by_metadata({"category": "finance"}, limit=0)
|
||||
|
||||
assert results == []
|
||||
|
||||
Reference in New Issue
Block a user