Skip to content

feat: add PGVectorStore — pgvector implementation of VectorStoreInter…#377

Open
theap06 wants to merge 4 commits into
gepa-ai:mainfrom
theap06:feat/pgvector-store
Open

feat: add PGVectorStore — pgvector implementation of VectorStoreInter…#377
theap06 wants to merge 4 commits into
gepa-ai:mainfrom
theap06:feat/pgvector-store

Conversation

@theap06

@theap06 theap06 commented Jun 15, 2026

Copy link
Copy Markdown

##Summary
Adds PostgreSQL pgvector as a vector store backend for GenericRAGAdapter. Supports cosine/L2/inner-product distance, hybrid search via tsvector, JSONB metadata filtering, HNSW indexing, and upsert/delete operations.


Open in Devin Review

theap06 and others added 2 commits June 14, 2026 04:08
…face

Adds PostgreSQL pgvector as a vector store backend for GenericRAGAdapter.
Supports cosine/L2/inner-product distance, hybrid search via tsvector,
JSONB metadata filtering, HNSW indexing, and upsert/delete operations.
Install with: pip install "gepa[pgvector]"

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- hybrid_search: use self._vector_score_sql() instead of hardcoded <=> so
  l2 and inner_product distance metrics produce correct hybrid scores
- vector_search: normalize all scores to [0,1] — cosine uses GREATEST(0,1-dist),
  l2 uses 1/(1+dist), inner_product clamps dot product to [0,1]
- get_collection_info: add rollback() in except block to avoid leaving the
  connection in an aborted transaction state
- get_collection_info: add pg_namespace join to avoid wrong dimension when
  multiple schemas share the same table name
- add_documents: use uuid4() for auto-generated IDs to prevent silent overwrite
  when add_documents is called multiple times without explicit IDs
- _build_where_clause: fix bool list filter — str(True)='True' but JSONB stores
  'true'; convert bools to lowercase string explicitly
- create_local: replace f-string DSN with psycopg2.connect() keyword args to
  handle passwords/usernames containing '@', '/', '?' without URL-parsing errors

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@semanticdiff-com

semanticdiff-com Bot commented Jun 15, 2026

Copy link
Copy Markdown

Review changes with  SemanticDiff

Changed Files
File Status
  pyproject.toml Unsupported file format
  src/gepa/adapters/generic_rag_adapter/__init__.py  0% smaller
  src/gepa/adapters/generic_rag_adapter/vector_stores/pgvector_store.py  0% smaller
  uv.lock Unsupported file format

Comment thread uv.lock
@LakshyAAAgrawal

Copy link
Copy Markdown
Contributor

Reviewed the PGVectorStore implementation. Overall structure matches the sibling stores well and the SQL is parameterized correctly (good use of psycopg2.sql.Identifier / sql.Literal everywhere — no injection holes I could spot). Ruff + pyright both clean.

A few things worth fixing or discussing before merge:

Blocking

  1. uv.lock is still in the diff (1,038 / 1,562 added lines). Your earlier request to revert it isn't addressed — that's almost all of the line count of this PR. Please drop the uv.lock changes; CI will lock against the existing version.

Correctness / behavior

  1. zip(ids, documents, embeddings, strict=False) at pgvector_store.py:263. The constructor already validates len(documents) == len(embeddings) and len(ids) == len(documents) on lines 246–251, so the lengths are guaranteed equal here — strict=True would correctly assert that invariant. With strict=False, a future refactor that drops one of those checks would silently truncate writes. Cheap fix, real safety improvement.

  2. Hybrid search hard-codes 'english' at line 169 (to_tsvector('english', content), plainto_tsquery('english', %s)). Silently degrades for non-English corpora. Add a tsvector_language: str = "english" constructor parameter and thread it through. (Same pattern other stores expose for language config.)

  3. No GIN index on metadata. create_table only creates the HNSW index on embedding. The moment a user uses filters={...}, every metadata->>%s = ... predicate does a sequential scan over the metadata JSONB. For the "production RAG" use case the docstring emphasizes, this matters. Either:

    • auto-create CREATE INDEX ... USING GIN (metadata) inside create_table, or
    • document in the docstring that users should add it themselves for filterable tables.
  4. get_collection_info silently swallows errors and returns dimension=0 (lines 223–231). If the table exists but the metadata SELECT fails (e.g., permission issue, pg_catalog quirk), callers checking info["dimension"] get a misleading zero. The error field gets set, but most callers won't check it. Recommend either re-raising or replacing the broken-state dict with a dimension=None sentinel.

Design / cleanup

  1. create_local / create_remote / from_connection_string are nearly identical. create_remote is literally from_connection_string(...) (line 510). create_local is from_connection_string with kwargs instead of a DSN. Two classmethods would cover both: from_connection_string (DSN) and from_credentials (kwargs). Three feels like API surface for no extra capability.

  2. Connection ownership is undefined. The class never closes self.conn, even when it created the connection itself via the classmethods. Most sibling stores have the same issue, but adding a close() method (and __enter__/__exit__) would be a clear win — especially for the create_local/create_remote paths that opened the connection on the user's behalf.

  3. embedding_function is untyped in __init__ and the classmethods. Should be Callable[[str], list[float]] | None. Same for conn (could type as psycopg2.extensions.connection | Any via TYPE_CHECKING import to keep the lazy-import contract).

  4. Repeated from psycopg2 import sql inside every method. Other lazy-imported stores tend to do psycopg2 = None at module level inside a try/except, then from psycopg2 import sql once. The repetition here is noisy. Minor — only worth doing if you're refactoring anyway.

Smaller nits

  1. HNSW index uses pgvector defaults (m=16, ef_construction=64 ish). Reasonable, but the docstring sells this as production-ready. Either accept hnsw_m / hnsw_ef_construction parameters in create_table, or doc that users should tune via CREATE INDEX ... WITH (...) if needed.

  2. get_collection_info's dimension query depends on the column being literally named "embedding". This is fine because create_table enforces that name, but it's an implicit contract. A one-line docstring note ("Assumes the embedding column is named embedding — set by create_table") would help anyone subclassing.

  3. No tests added. Sibling stores (chroma, milvus, qdrant) likewise lack live-service tests, so this isn't a blocker — but you could add a mock-based test for _build_where_clause and _vector_score_sql since they generate non-trivial SQL fragments and would be easy to break silently. The existing tests/test_rag_adapter/test_vector_store_interface.py MockVectorStore pattern is a natural fit.

Items 1 and 2 are the ones I'd want fixed before merge. Items 3–5 are real bugs but ones you could fix in a follow-up if you're trying to land this quickly. The rest are polish.

Nice clean implementation overall — the parameterized SQL is genuinely safer than I usually see in vector-store PRs.

- Revert uv.lock to main — CI manages the lock file
- zip strict=True in add_documents — lengths are already validated above,
  strict=True correctly asserts that invariant instead of silently truncating
- Add tsvector_language parameter (default: "english") to __init__ and all
  factory methods; hybrid_search now uses it via sql.Literal so non-English
  corpora work correctly
- Add GIN index on metadata in create_table to avoid sequential scans on
  the JSONB filter predicates produced by _build_where_clause
- get_collection_info: use dimension=None instead of 0 when the column
  lookup returns nothing, so callers can distinguish missing from zero

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@theap06 theap06 requested a review from LakshyAAAgrawal June 18, 2026 00:42

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

Open in Devin Review

row = cur.fetchone()
# None signals "table exists but dimension could not be determined"
# rather than returning a misleading 0.
dimension = row[0] if row else None

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Collection info returns a non-integer dimension, breaking the inherited dimension-getter

The dimension field is set to None (pgvector_store.py:221) instead of the integer 0 expected by the interface, so the inherited get_embedding_dimension() method returns None instead of int.

Impact: Any caller relying on get_embedding_dimension() to return an integer (e.g. for arithmetic or size checks) will get None and fail.

Interface contract violation and comparison with all other stores

The VectorStoreInterface documents "dimension" as (int): Vector embedding dimension (0 if unknown) at vector_store_interface.py:170. The base-class get_embedding_dimension() at vector_store_interface.py:193-194 calls info.get("dimension", 0) — if the key exists with value None, .get() returns None rather than the default 0.

Every other store implementation (Chroma, Qdrant, Milvus, LanceDB, Weaviate) returns "dimension": 0 when the dimension is unknown. PGVectorStore returns None in both the success path (line 221) and the error path (line 235).

Suggested change
dimension = row[0] if row else None
dimension = row[0] if row else 0
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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