fix(db): find() sort contract — dotted paths, nulls-last, and limit/sort pushdown - #31
Merged
Conversation
`_find_sort_key` looked up sort fields with a flat `record.get(field)`, so a
spec like `sort=[("context.started_at", -1)]` produced `None` for every row
and left results in arbitrary order.
The SQLite and Postgres sort pushdowns (`translate_sort`) and Mongo's native
sort already resolve dotted paths, so the same query ordered correctly on
those backends and silently did not on JsonDB/DynamoDB — or on SQLite and
Postgres whenever the query fell back to the in-memory path.
Resolve dotted paths in `_find_sort_key`; a non-dict segment along the path
yields `None` rather than raising.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nding `finalize_find_results` sorts descending via `reverse=True`, which flipped `_find_sort_key`'s `(value is None, value)` flag along with the values and floated records missing the sort field to the front. Both SQL translators emit NULLS LAST for descending — SQLite via a leading `(col IS NULL)` term that is itself sorted ASC, Postgres via an explicit `DESC NULLS LAST` — and Mongo sorts missing values last. A "newest N" `sort` + `limit` fetch therefore returned real rows on SQLite/Postgres/Mongo and a window of records missing the field on the in-memory path. Invert the None flag for descending sorts so missing values land last in both directions. All missing values share a flag, so None is never compared against a real value. Also correct the comment in `_sqlite_translate.translate_sort`, which asserted the in-memory sort already put NULLs last. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Records the two properties every adapter has to produce identically whether it pushes the sort into the backend or falls back to `finalize_find_results`: dotted paths resolve into nested documents, and missing values sort last in both directions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`translate_sort` returns None for a field path it cannot safely interpolate (e.g. `context.my-field`), so the ordering has to happen in `finalize_find_results`. The LIMIT was still pushed into the SQL, so the database returned an arbitrary N rows and the in-memory sort ordered that arbitrary subset: `find(sort=..., limit=10)` returned the top 10 of an arbitrary 10 rather than the true top 10. Withhold the LIMIT whenever the sort falls back to memory, then apply it after sorting. `SQLiteDB.find` and `DynamoDB.find` already did this. Also stop re-sorting vector (`$near`) results by the user's `sort`. The comment says the vector ORDER BY wins when both are present, but forcing `sort_sql = None` for vector queries meant the in-memory branch fired and discarded the distance ordering. Applies to both `PostgresDB.find` and the verbatim copy in `PostgresTransaction.find`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…value tail
Two defects in `GraphContext.find_page` keyset pagination.
The cursor payload was minted with a flat `last.get(primary_field)`. For a
dotted sort field the value is nested, so every cursor encoded `sort: None`
and the next page compared against it — raising `TypeError: '>' not supported
between instances of 'int' and 'NoneType'` out of `QueryEngine` on JsonDB
rather than paging. Mint the cursor with `resolve_sort_value`, the same path
walk the adapters and `finalize_find_results` use.
Records missing the sort field sort last in both directions, but the keyset
filter `{field: {"$lt": value}}` can never match a record that has no value,
so iteration stopped at the last record that had one and the trailing run was
unreachable. Add a `{field: None}` branch — which matches both an explicit
null and a missing key — and, when the cursor itself was minted inside that
run, walk it by `id` alone.
Extract the path walk from `_find_sort_key` into `resolve_sort_value` and
export it so cursor minting and adapter sorting cannot drift apart.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`ObjectPager.get_page` fetched a slice with a DB-side `sort` + `limit`, then
re-sorted that page in Python with
`item.get("context", {}).get(order_by, 0)`. A record missing `order_by`
became `0` and sorted among the real values, while the slice that produced
the page had already placed it in the trailing missing-value run. The two
orderings disagree, so a record could appear on two pages or on none.
The blanket `contextlib.suppress(KeyError, TypeError)` around it also left a
page silently in DB order whenever the key raised.
Route the safety net through `finalize_find_results` instead. It is now a
genuine no-op when the backend honored the sort, and matches the nulls-last
contract when it did not.
`test_get_page_with_ordering` was passing on the in-Python re-sort alone —
its mock ignored `sort` and `limit` entirely, so it could not have caught
this. Switch it to `mock_find_respecting_limit`, which the keyset tests in
the same file already use.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ract Three docstrings still said "NULLs sort last for ascending, first for descending, mirroring finalize_find_results" — the opposite of what the code emits and of the contract: the module docstring and `translate_sort` docstring in `_sqlite_translate.py`, and `translate_sort` in `_postgres_translate.py`. Only the inline comment was corrected when the in-memory ordering was fixed. Move the `find` sort contract from SPEC §4.2 (capability flags) to §4.1, beside the `Database` method table it governs, and extend it with the rule that `limit` must not be pushed down when the sort is not, plus a Known divergences table: MongoDB's native `cursor.sort()` places missing values first on ascending sorts (documented, not normalized — compensating needs an aggregation pipeline on every find), array-index path segments, and heterogeneous value types. Document the contract on `Database.find` for adapter authors, and correct `Database.find_iter`, which claimed a composite `(sort_value, id)` cursor it does not implement — it tracks `id` only, so a non-`id` sort drops records that sort late but carry a lower `id`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`test_find_sort_nulls_last.py` only string-matches the SQL fragments the translators emit, so the two backends the contract binds hardest had no runtime assertion. Add end-to-end `SQLiteDB` coverage that runs the same data through both branches of `find` — ORDER BY pushed into SQL, and the in-memory `finalize_find_results` fallback reached via an unsafe field path — and asserts they agree on values-then-missing ordering in both directions and on the true top N under `limit`. Also normalize a falsy `sort` to `None` in `SQLiteDB.find`. `sort=[]` failed the `sort is None` guard and took the untranslatable-sort branch, loading the whole collection and applying `limit` in memory. Same rows, needless work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Benchmark comparisonThreshold: ±25% (informational, does not block merge)
|
Resolve the CHANGELOG conflict: both branches added entries under [Unreleased] at the same spot. The two entry sets are disjoint, so keep all of them, regrouped under shared Added/Fixed/Changed/Documentation headings.
Benchmark comparisonThreshold: ±25% (informational, does not block merge)
|
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
1 participant
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.
Started as a one-function change (
_find_sort_keydid not resolve dotted paths) and grew once the behavior was checked against each backend. Three of the five fixes are pre-existing wrong-results bugs in the same area.The contract
sortis a list of(field, direction). Every adapter must produce the same ordering whether it pushes the sort into the backend or falls back tofinalize_find_results. SPEC §4.1 now states it, with a Known divergences table for the cases that are documented rather than normalized.Fixes
1. In-memory sort ignored dotted field paths (
db/database.py)_find_sort_keyused a flatrecord.get(field), sosort=[("context.started_at", -1)]producedNonefor every row and left results in arbitrary order. SQLite/Postgres pushdowns and Mongo's native sort already resolved dotted paths, so the same query ordered correctly there and silently did not on JsonDB/DynamoDB — or on SQLite/Postgres whenever the query fell back.2. Descending sorts placed missing values first (
db/database.py)finalize_find_resultssorts withreverse=True, which flipped theNoneflag along with the values:Both SQL translators and Mongo put missing last. A "newest N"
sort+limittherefore returned real rows on SQLite/Postgres/Mongo and a window of holes in memory. The comment in_sqlite_translate.translate_sortasserting the in-memory path already matched was wrong and is corrected.3. Postgres pushed
LIMITeven when the sort pushdown failed (db/postgres.py, bothfindpaths)translate_sortreturnsNonefor an unsafe field path, so ordering fell to memory — but theLIMITstill went into the SQL.find(sort=..., limit=10)returned the top 10 of an arbitrary 10.SQLiteDB.findandDynamoDB.findalready withheld it. Vector$nearqueries also no longer have their distance ordering overwritten by an in-memory re-sort.4.
find_pagekeyset cursor broke on dotted fields and could not reach the missing-value tail (core/context.py)The cursor was minted with a flat
last.get(primary_field), so a dotted sort encodedsort: Noneevery page and the next page raisedTypeError: '>' not supported between instances of 'int' and 'NoneType'fromQueryEngine. Separately,{field: {"$lt": value}}can never match a record with no value, so iteration stopped at the last record that had one. Cursors now useresolve_sort_value; the filter carries a{field: None}branch, and a cursor minted inside the tail walks it byid.5.
ObjectPagerre-sorted each page with a key that disagreed with the slice (core/pager.py)item.get("context", {}).get(order_by, 0)ordered a missing value as0— among the real values — while the DB-side slice had placed it in the trailing run. Records could appear on two pages or none. Now routed throughfinalize_find_results, so it is a genuine no-op when the backend honored the sort.Plus:
SQLiteDB.findtreatedsort=[]as untranslatable and loaded the whole collection;resolve_sort_valueextracted and exported so cursor minting and adapter sorting cannot drift apart again.Verification
Each of fixes 3–5 was verified by reverting the source file and confirming the new tests fail. New coverage: SQLite end-to-end through both branches (pushdown and in-memory fallback) — the two backends the contract binds hardest previously had only string-matched SQL fragments; stubbed-pool Postgres tests plus a DSN-gated integration case;
find_pagemulti-page walks asserting no duplicates or omissions in both directions.pytest: 2037 passed, 131 skipped.pre-commit run --all-filesgreen.Reviewer note
Commit
bb024ee(missing-values-last) changes ordering for existing flat-field descending sorts. It is split out so it can be taken or dropped independently of the dotted-path fix.🤖 Generated with Claude Code