Postgres graph database [ Daniel Scott - Draft ]#2504
Closed
Conversation
Problems: - Need to migrate basic graph functionality to relational DB Solutions: - New adapter implementing GraphDBInterface - Selection branch included in get_graph_engine.py Decisions: - re-use SQLAlchemy setup and configuration - Remove .query() (raw Cypher) functionality
Problems: - Cypher search retriever exposes raw cypher interface via .query() - Natural language retriever writes, uses Cypher via .query() - Temporal awareness (Graphiti) subsystem directly uses cypher, neo4j Solutions: - Raise incompatibility errors proactively across these consumers Decisions: - None of these are core functionality, cost should be low - Graphiti already unguarded hard-coding of multiple configs... - Complex cypher search cannot be replaced in postgres at present
Problems: - asyncpg misparses ::jsonb cast as named parameter - prune_system drops graph tables via delete_database() - cached adapter instance has no tables after prune - no CI coverage for postgres graph backend - no unit or e2e tests for postgres adapter Solutions: - replace ::jsonb with CAST(:param AS jsonb) throughout - add _ensure_initialized() calling idempotent CREATE IF NOT EXISTS - guard is_empty and delete_graph with _ensure_initialized - add run-postgres-tests job to graph_db_tests.yml CI workflow - add 26 unit tests against real Postgres - add shared e2e test and postgres wrapper Decisions: - _ensure_initialized over defensive checks on every method - real Postgres in tests, not SQLite (dialect differences block it) - shared test function for future backend reuse (test_graphdb_shared) - CI uses Postgres 16 service container, matching Kuzu/Neo4j pattern
Problems: - some white-space and wrapping non-compliance Solutions: - fix via ruff
Problems: - want joint graph and vector interaction in SQL to extent possible - currently, graph and vector writes are separate transactions - search requires two round-trips, vector search then graph search Solutions: - add PostgresHybridAdapter composing PostgresAdapter and PGVectorAdapter - implement combined write methods with single-transaction atomicity - implement batching to avoid loops on session.execute calls later - implement combined search by joining graph and vector tables - exceptions in get_graph_engine and create_vector_engine if given pghybrid - add pghybrid construction in get_unified_engine Decisions: - composition rather than inheritance on subclasses of SQLAlchemyAdapter - otherwise subtle configuration bugs can occur (diamond problem) - hybrid only allowed via get_unified_engine, not individual factories - per-row inserts for now, batch optimization deferred - table name validation via regex to prevent SQL injection
Problems: - pghybrid delegates query() to PostgresAdapter which raises generic error - isinstance checks only catch PostgresAdapter, not PostgresHybridAdapter - users would see NotImplementedError instead of descriptive message Solutions: - add PostgresHybridAdapter to isinstance checks in three files - cypher search, natural language, and graphiti now guard both adapters
Problems: - no test coverage for hybrid adapter Solutions: - add unit tests covering graph, vector, combined, and content paths - content integrity test verifies IDs and payloads across both tables - tests use local SQLAlchemyAdapter to avoid stale event loop issues - fixture teardown cleans up vector collection tables between tests Decisions: - use uuid5 for test IDs because pgvector tables require UUID columns - bypass PGVectorAdapter constructor to share connection pool with graph - e2e and CI deferred because cognify pipeline calls get_graph_engine directly
…rate-joint-postgres-graph-db-and
Please make sure all the checkboxes are checked:
|
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hello @dexters1, thank you for submitting a PR! We will respond as soon as possible. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Acceptance Criteria
Type of Change
Screenshots
Pre-submission Checklist
CONTRIBUTING.md)DCO Affirmation
I affirm that all code in every commit of this pull request conforms to the terms of the Topoteretes Developer Certificate of Origin.