Skip to content

Fix iter_rows_keyset NULL hang and silent row loss on ties#4

Merged
varjoranta merged 1 commit into
mainfrom
fix/iter-keyset-null-and-ties
May 13, 2026
Merged

Fix iter_rows_keyset NULL hang and silent row loss on ties#4
varjoranta merged 1 commit into
mainfrom
fix/iter-keyset-null-and-ties

Conversation

@varjoranta
Copy link
Copy Markdown
Owner

Summary

Replaces the narrow ValueError fix from #2 with proper compound-cursor pagination. iter_rows_keyset now correctly paginates through NULL values in `by` and through tied non-NULL values; both failure modes are gone.

Re #1 (already closed via #2; this PR replaces that narrow fix with the proper one).

What changed

  • Cursor columns extended from `[by]` to `[by] + [p for p in pk if p != by]`. PK tail breaks ties and lets the cursor advance through NULL values.
  • `ORDER BY by NULLS FIRST` is now explicit so NULL rows iterate first on every backend (Postgres default would be NULLS LAST; SQLite default already NULLS FIRST).
  • WHERE cursor has three shapes, Python-side branched on cursor state:
    • first page -- no predicate
    • NULL region -- `(by IS NULL AND <pk_cursor>) OR by IS NOT NULL`
    • past NULL -- `(by, *pk_tail) > (last_by, *last_pk_tail)`
  • OR-group fully parenthesized so filters compose correctly.
  • Single-column cursor (by IS pk, single PK) keeps the scalar form `by > $k` -- no one-element row-value syntax.
  • The ValueError introduced in Fix iter_rows_keyset infinite loop on NULL by column (#1) #2 for NULL-at-page-boundary is removed.

API change

`sql.select_keyset`'s `last_seen` parameter shape changes from scalar to `Sequence` aligned with the compound cursor. This is a breaking change for direct emitter consumers / `db.compose` previewers; `db.iter_rows_keyset`'s public API is unchanged.

Behavior change for nullable `by` columns

NULL rows now iterate first on Postgres (was implicit NULLS LAST). Non-nullable `by` is unaffected. Users who were hitting the ValueError now get correct pagination instead. No previously-working iteration becomes broken.

SQLite requirement

`NULLS FIRST` needs SQLite 3.30+ (2019). All mainstream environments comply (macOS 3.43, Ubuntu 22.04 3.37, Ubuntu 24.04 3.45+). Documented in README.

Test plan

  • 7 new integration tests parametrized x 3 backends = 21 runs:
  • 11 unit tests covering all three cursor states x single/composite PK x filter precedence x placeholder numbering
  • `uv run pytest`: 507 passed, 1 skipped (was 484)
  • `uv run ruff check`: clean
  • simplify pass applied

The 0.6 keyset cursor used `WHERE by > last_seen` which broke in
two ways: NULL at a page boundary stalled the cursor (#1), and
tied non-NULL values were silently skipped because `WHERE by > X`
excludes everything with `by = X`.

Replace with a compound cursor: `[by] + [p for p in __pk__ if p
!= by]`. PK tail breaks ties and lets the cursor paginate through
NULL values. ORDER BY uses explicit NULLS FIRST so iteration
order is deterministic across backends (Postgres default would be
NULLS LAST; SQLite default is already NULLS FIRST).

WHERE has three shapes branched on cursor state:
- first page  -- no cursor predicate
- NULL region -- (by IS NULL AND <pk_cursor>) OR by IS NOT NULL
- past NULL   -- (by, *pk_tail) > (last_by, *last_pk_tail)

The OR-group is fully parenthesized so filters compose correctly.
Single-column cursor (by IS pk, single PK) degenerates to scalar
`by > $k` so no one-element row-value syntax is emitted.

API change: sql.select_keyset's `last_seen` parameter shape goes
from scalar to Sequence aligned with the compound cursor. The
ValueError introduced in 0.6 for NULL-at-page-boundary is gone --
the cursor handles NULL properly now. SQLite 3.30+ is required
for the NULLS FIRST keyword; mainstream environments comply.

Fixes #1.
@varjoranta varjoranta merged commit 65a13a7 into main May 13, 2026
7 checks passed
@varjoranta varjoranta deleted the fix/iter-keyset-null-and-ties branch May 13, 2026 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant