From cab995dc9770bc1cd3e6756e0ccabc4b0f109b6e Mon Sep 17 00:00:00 2001 From: KaifAhmad1 Date: Wed, 12 Aug 2026 12:46:23 +0530 Subject: [PATCH] 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 --- CHANGELOG.md | 12 +++ semantica/vector_store/faiss_store.py | 3 + semantica/vector_store/milvus_store.py | 6 ++ semantica/vector_store/pgvector_store.py | 23 ++++- semantica/vector_store/pinecone_store.py | 11 ++- semantica/vector_store/sqlite_vec_store.py | 15 +++- .../test_backend_metadata_filtering.py | 86 ++++++++++++++++++- .../test_decision_embedding_pipeline.py | 54 ++++++------ tests/vector_store/test_sqlite_vec_store.py | 48 +++++++++++ 9 files changed, 219 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8efdcd18..c5c574b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/semantica/vector_store/faiss_store.py b/semantica/vector_store/faiss_store.py index 2e39266f..54df70a4 100644 --- a/semantica/vector_store/faiss_store.py +++ b/semantica/vector_store/faiss_store.py @@ -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): diff --git a/semantica/vector_store/milvus_store.py b/semantica/vector_store/milvus_store.py index 2a213859..f467eeb4 100644 --- a/semantica/vector_store/milvus_store.py +++ b/semantica/vector_store/milvus_store.py @@ -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 = ( diff --git a/semantica/vector_store/pgvector_store.py b/semantica/vector_store/pgvector_store.py index bfed4512..e1ce8ed0 100644 --- a/semantica/vector_store/pgvector_store.py +++ b/semantica/vector_store/pgvector_store.py @@ -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 diff --git a/semantica/vector_store/pinecone_store.py b/semantica/vector_store/pinecone_store.py index f58f61dc..068cb4bb 100644 --- a/semantica/vector_store/pinecone_store.py +++ b/semantica/vector_store/pinecone_store.py @@ -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( diff --git a/semantica/vector_store/sqlite_vec_store.py b/semantica/vector_store/sqlite_vec_store.py index 646353ec..9fbdaed5 100644 --- a/semantica/vector_store/sqlite_vec_store.py +++ b/semantica/vector_store/sqlite_vec_store.py @@ -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}') = ?") diff --git a/tests/vector_store/test_backend_metadata_filtering.py b/tests/vector_store/test_backend_metadata_filtering.py index 5401f0fd..cffe422c 100644 --- a/tests/vector_store/test_backend_metadata_filtering.py +++ b/tests/vector_store/test_backend_metadata_filtering.py @@ -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() diff --git a/tests/vector_store/test_decision_embedding_pipeline.py b/tests/vector_store/test_decision_embedding_pipeline.py index ed65236d..9293f726 100644 --- a/tests/vector_store/test_decision_embedding_pipeline.py +++ b/tests/vector_store/test_decision_embedding_pipeline.py @@ -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): """ diff --git a/tests/vector_store/test_sqlite_vec_store.py b/tests/vector_store/test_sqlite_vec_store.py index 786257f0..0c72888e 100644 --- a/tests/vector_store/test_sqlite_vec_store.py +++ b/tests/vector_store/test_sqlite_vec_store.py @@ -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 == []