Repository navigation
Python: Reject skip with key lookup in PostgreSQL and DuckDB collections - #10207
Wilson_Jiang (Wilson0511) wants to merge 1 commit into
Conversation
get(keys=..., skip=N) silently dropped the explicit skip in the PostgreSQL and DuckDB _inner_get key branches — the unshifted key query was built and run, so the same portable call succeeded here while MongoDB, Qdrant, and Oracle raise ValueError for it (and core itself raises for keys+filter as alternate retrieval modes). Both key branches now reject skip with the same message shape as their siblings, next to their existing order_by guard. Co-Authored-By: ZCode <zcode@bigmodel.ai>
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are recorded for the change aligning PostgreSQL and DuckDB key-lookup behavior with other connectors.
No open issues 🎉
What changed in this PR
Addresses #10206 by making PostgreSQL and DuckDB reject nonzero skip during key lookup, aligning their behavior with MongoDB, Qdrant, and Oracle.
Changes:
- Add
ValueErrorguards for key retrieval combined withskip. - Add regression tests for both connectors, including a DuckDB check that listing without keys still accepts
skip. - Leave
keys+topbehavior unchanged.
| File | Description |
|---|---|
python/packages/postgres/tests/test_postgres.py |
Adds an offline regression test for rejecting keys with skip. |
python/packages/postgres/agent_framework_postgres/_vector_store.py |
Rejects nonzero skip in PostgreSQL key lookups. |
python/packages/duckdb/tests/duckdb/test_collection.py |
Tests rejection and preserves listing support for skip. |
python/packages/duckdb/agent_framework_duckdb/_vector_store.py |
Rejects nonzero skip in DuckDB key lookups. |
Review effort: Balanced
|
Wilson_Jiang (@Wilson0511), the current-head code review is clean and checks are green. Before approval, please update the PR description to include the repository template's Contribution Checklist, including contribution-guideline compliance, the agreed-issue/no-competing-PR attestation, and breaking-change status. The prose AI disclosure is sufficient for review; missing the checkbox alone is not a blocker. |
Python: Reject skip with key lookup in PostgreSQL and DuckDB collections
Description
get(keys=..., skip=N)silently dropped the explicitskipin the PostgreSQL and DuckDB_inner_getkey branches — the unshifted key query was built and run, so the same portable call succeeded here while MongoDB, Qdrant, and Oracle raiseValueErrorfor it (and core itself raises for keys+filter as alternate retrieval modes). Both key branches now rejectskipwith the same message shape as their siblings, matching their existingorder_byguard.keys+topremains as-is everywhere except Oracle; that divergence is a separate design call (noted in the linked issue).Fixes #10206
Tests
test_key_retrieval_rejects_skipin both packages:get(["key"], skip=1)raisesValueErrormatching "cannot be combined with skip";get(skip=1)without keys still works. Red on main (DuckDB repro returns the record with skip silently dropped —Failed: DID NOT RAISE), green with the guards. DuckDB runs against the existing in-memory fixture; PostgreSQL's guard fires before any connection so the test is fully offline.Attribution
Developed with AI assistance (model: GLM-5.3 via ZCode; Co-Authored-By trailer on the commit). Found by census-ing the key-lookup guard sets across all vector-store connectors after the stores family sweep; diff reviewed in the authoring context, no separate fresh-context pass. Integration tests requiring live PostgreSQL: not run locally.