diff --git a/gramps_object_query_language/proxied_query.py b/gramps_object_query_language/proxied_query.py index dee46f9..279fb07 100644 --- a/gramps_object_query_language/proxied_query.py +++ b/gramps_object_query_language/proxied_query.py @@ -67,7 +67,9 @@ `NULL` placement, for both plain sorting and keyset seeking, matches SQLite's own verified default (confirmed empirically, not assumed): `NULL` sorts as the smallest value in both directions, so `DESC` is the exact -reverse of `ASC`, `NULL`s included. Keyset seeking (`after`) mirrors +reverse of `ASC`, `NULL`s included. PostgreSQL's default is the opposite, +so the SQL path pins this order there with explicit `NULLS FIRST`/`NULLS +LAST` (`query.py`'s `_order_direction_sql`). Keyset seeking (`after`) mirrors `query.py`'s `_compile_keyset` SQL, which (after its own NULL-safety fix -- see that function's docstring) applies this identical NULL-is-smallest rule to the seek predicate too, rather than relying on SQL's raw three-valued diff --git a/gramps_object_query_language/query.py b/gramps_object_query_language/query.py index 1b32a74..1f42c5d 100644 --- a/gramps_object_query_language/query.py +++ b/gramps_object_query_language/query.py @@ -2650,8 +2650,9 @@ def _keyset_leg_sql( ) -> Optional[Tuple[str, list]]: """NULL-safe "ranks strictly after the cursor" fragment for the one `order_by` column a seek's OR-term actually advances past, matching - `ORDER BY`'s own verified default total order (`NULL` is the smallest - value in both directions -- see `_null_safe_cmp` in `proxied_query.py`, + `ORDER BY`'s total order (`NULL` is the smallest value in both + directions -- SQLite's default, pinned explicitly on PostgreSQL by + `_order_direction_sql`; see also `_null_safe_cmp` in `proxied_query.py`, which mirrors this same order for the evaluator path). A plain `col > ?`/`col < ?` against a real SQL `NULL` is always @@ -2811,6 +2812,25 @@ def _where_clauses( return clauses, params +def _order_direction_sql(ob: OrderBy, dialect: Optional[Dialect]) -> str: + """`ASC`/`DESC` for one `ORDER BY` column, pinned to the "`NULL` is + smallest" total order that `_keyset_leg_sql` and `proxied_query.py`'s + `_null_safe_cmp` both assume. + + That is SQLite's default, so SQLite gets the bare keyword. PostgreSQL's + default is the opposite (`NULL` is largest: last in `ASC`, first in + `DESC`), which both reordered results relative to SQLite and made the + keyset seek skip rows -- so it gets an explicit `NULLS FIRST`/`NULLS + LAST`. `handle` is left bare: it's the `NOT NULL` primary key, so the + clause changes nothing there, and a bare `ASC`/`DESC` keeps the + default-ordered primary-key index usable for the sort. + """ + keyword = ob.direction.upper() + if dialect != Dialect.POSTGRESQL or ob.column == "handle": + return keyword + return f"{keyword} NULLS FIRST" if ob.direction == "asc" else f"{keyword} NULLS LAST" + + def compile_query( spec: ObjectTypeSpec, query: Query, @@ -2886,7 +2906,7 @@ def compile_query( order_params: list = [] for ob in ordering: order_sql, p = _column_expr(ob.column, spec, collation, dialect, treeid) - order_parts.append(f"{order_sql} {ob.direction.upper()}") + order_parts.append(f"{order_sql} {_order_direction_sql(ob, dialect)}") order_params.extend(p) sql = f"SELECT {', '.join(select_parts)} FROM {spec.table}" diff --git a/gramps_object_query_language/tests/test_query.py b/gramps_object_query_language/tests/test_query.py index bd34a6a..1892e6b 100644 --- a/gramps_object_query_language/tests/test_query.py +++ b/gramps_object_query_language/tests/test_query.py @@ -1576,7 +1576,7 @@ def test_order_by_gets_trailing_handle_tiebreaker(): # which isn't what this test is about. query = Query(select=["handle"], order_by=[OrderBy("surname", "asc")]) sql, _ = compile_query(PERSON, query, dialect=Dialect.POSTGRESQL) - assert "ORDER BY surname ASC, handle ASC" in sql + assert "ORDER BY surname ASC NULLS FIRST, handle ASC" in sql def test_order_by_does_not_duplicate_explicit_handle(): @@ -1586,10 +1586,38 @@ def test_order_by_does_not_duplicate_explicit_handle(): ) sql, _ = compile_query(PERSON, query, dialect=Dialect.POSTGRESQL) order_by_clause = sql.split("ORDER BY", 1)[1] - assert order_by_clause.startswith(" surname ASC, handle DESC") + assert order_by_clause.startswith(" surname ASC NULLS FIRST, handle DESC") assert order_by_clause.count("handle") == 1 # not duplicated +def test_postgresql_order_by_pins_null_is_smallest(): + # Issue #4: PostgreSQL's default puts NULL last in ASC and first in DESC, + # the opposite of SQLite's and of what _keyset_leg_sql/_null_safe_cmp + # assume -- so results reordered and keyset paging dropped rows. The + # PostgreSQL dialect must pin NULL-is-smallest explicitly, both + # directions, for plain columns and JSON paths alike. + query = Query( + select=["handle"], + order_by=[OrderBy("birth.place.title", "desc"), OrderBy("surname", "asc")], + ) + sql, _ = compile_query(PERSON, query, dialect=Dialect.POSTGRESQL) + order_by_clause = sql.split("ORDER BY", 1)[1] + assert order_by_clause.count("DESC NULLS LAST") == 1 + assert "surname ASC NULLS FIRST" in order_by_clause + # The NOT NULL primary-key tiebreaker stays bare (index-friendly). + assert "handle ASC NULLS" not in order_by_clause + + +def test_sqlite_order_by_has_no_nulls_clause(): + # SQLite's default already is NULL-is-smallest; output stays unchanged. + query = Query( + select=["handle"], + order_by=[OrderBy("birth.place.title", "desc"), OrderBy("surname", "asc")], + ) + sql, _ = compile_query(PERSON, query, dialect=Dialect.SQLITE) + assert "NULLS" not in sql + + def test_default_order_by_is_handle_only(): query = Query(select=["handle"]) sql, _ = compile_query(PERSON, query, dialect=Dialect.POSTGRESQL)