feat: add graph filter when searching to PostgreSQL graph#3059
feat: add graph filter when searching to PostgreSQL graph#3059dexters1 wants to merge 3 commits into
Conversation
|
Hello @dexters1, thank you for submitting a PR! We will respond as soon as possible. |
|
Adds graph filtering with the confidence of someone who's never heard of unit tests — the PR template's untouched and the new method ships with zero coverage. 🟡 0 blockers, 1 major, 2 minor — please address
See inline comments for details. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| data.update(row[3] if isinstance(row[3], dict) else json.loads(row[3])) | ||
| nodes.append((row[0], data)) | ||
|
|
||
| return nodes, edges |
There was a problem hiding this comment.
major — Missing tests for this new method.
The Neo4j and Ladybug implementations both have integration test coverage, but this Postgres implementation has zero tests. At minimum, add a test in cognee/tests/e2e/postgres/test_postgres_adapter.py that:
- Adds nodes and edges to the graph
- Calls
get_id_filtered_graph_datawith a subset of node IDs - Verifies it returns only edges touching those IDs and their endpoint nodes
- Verifies the edge-driven contract (nodes with no edges aren't returned)
Why: Without tests, regressions won't be caught, and future refactors (e.g., table schema changes) could silently break this method.
How to apply: Before merging, add at least one integration test covering the happy path and the empty-result case.
| for row in edge_result.fetchall(): | ||
| props = {} | ||
| if row[3]: | ||
| props = row[3] if isinstance(row[3], dict) else json.loads(row[3]) |
There was a problem hiding this comment.
minor — Missing error handling for malformed JSON.
Neo4j and Ladybug implementations wrap JSON parsing in try/except with logger.warning (see ladybug:2324, neo4j:1356). If row[3] is a string but not valid JSON, json.loads() raises JSONDecodeError and crashes the entire query, potentially hiding partial results.
Suggested fix:
| props = row[3] if isinstance(row[3], dict) else json.loads(row[3]) | |
| if row[3]: | |
| try: | |
| props = row[3] if isinstance(row[3], dict) else json.loads(row[3]) | |
| except (json.JSONDecodeError, TypeError): | |
| logger.warning(f"Failed to parse edge properties for {row[0]}->{row[1]}") | |
| props = {} |
Why: Database corruption or migration bugs can leave malformed JSON; graceful degradation (warn + skip) beats a stack trace.
|
|
||
| return nodes, edges | ||
|
|
||
| async def get_id_filtered_graph_data( |
There was a problem hiding this comment.
nit — Add timing and result-count logging to match Neo4j/Ladybug.
Both Neo4j (line 1349) and Ladybug (line 2348) log retrieval time and result counts. Postgres should do the same for observability and performance debugging.
Suggested pattern:
import time
start_time = time.time()
# ... query logic ...
logger.info(
f"ID-filtered retrieval: {len(nodes)} nodes and {len(edges)} edges in {time.time() - start_time:.2f}s"
)
| register = getattr(module, "register", None) | ||
| if callable(register): | ||
| register() | ||
| print(f"Registered adapter module: {module_name}") |
There was a problem hiding this comment.
minor — Adapter registration in the benchmark script feels misplaced.
This pattern (dynamic adapter registration via env var) is useful but belongs in core infrastructure, not buried in a performance benchmark. If COGNEE_REGISTER_ADAPTERS is a general feature, it should live in the adapter factory or a dedicated bootstrap module so all entry points (API, CLI, tests) can use it, not just this one benchmark script.
Why: Duplicating this block across test files when other tests need it is tech debt. If it's temporary scaffolding for this PR only, add a comment explaining that.
How to apply: Either (a) move it to a shared utility (e.g., cognee/infrastructure/databases/graph/register_adapters.py), or (b) add a comment explaining this is benchmark-specific and why.
1 participant
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.