perf(db): resolve the observed-objects cache in one query

[mrosseel]

Follow-on to #564, from the other side of the same stall.

What is left after #564

#564 indexed catalog_objects, so each listing lookup is now an index probe instead of a ~151k-row scan, and the ~1.4 s stall is gone. Two things from #528 remain:

1. load_observed_objects_cache() still resolves one lookup per logged listing, so the cache build grows with the size of the log.
2. UIObjectDetails.__init__ still builds a new ObservationsDatabase — and therefore a new cache — on every entry into the object details screen.

Measured on a Pi 4 with the indexed catalog DB and 138 logged listings:

	time
138 single indexed lookups	7.2 ms
one bulk query	1.6 ms

Small in absolute terms today, but it is paid on every screen entry and scales with the log. On the same 138 listings against a catalog DB that has no indexes yet — one built before #564, before _ensure_catalog_object_indexes() backfills — the loop costs 5.5 s and the single query 0.075 s. That is how I found this: a device in the field took 6.5–8.5 s to open object details.

Change

• ObjectsDatabase.get_object_ids_by_listings() — maps many listings to object ids in one query, chunked at 400 pairs to stay inside SQLite's variable limit, written as row values so the (catalog_code, sequence) index resolves the whole chunk.
• load_observed_objects_cache() calls it once instead of looping.
• UIObjectDetails shares one ObservationsDatabase per process, the same pattern as the existing _catalog_db() handle, so opening the screen stops rebuilding the cache.

No schema change, no catalog rebuild, no behaviour change: the resulting observed_object_ids set is identical.

Tests

• tests/test_objects_db_listings.py (new): bulk lookup matches single lookups, unresolved listings are omitted, chunking works past one chunk.
• tests/test_observed_identity.py: the test double stubs the bulk seam too, and a new test asserts the cache resolves in exactly one call.

ruff check and ruff format --check pass. Full unit run on macOS: 1232 passed; the only failures are 5 in tests/test_comets.py, from skyfield version skew in my local environment, untouched by this change.

🤖 Generated with Claude Code

[brickbots]

Pushed a commit onto this branch (179230c) changing the shape of the bulk query, plus restoring a None guard. The _obs_db() sharing is untouched — that half is sound, and it's doing more than the description claims (see the last note). Reasoning for the query change:

The row-value form doesn't use the index

The docstring said the listings were written as row values "so the (catalog_code, sequence) index resolves the whole chunk." SQLite won't do that. It cannot drive a two-column index from a row-value IN (VALUES …) list, so it constrains on the leading column only and filters the rest:

SEARCH catalog_objects USING INDEX idx_catalog_objects_code_sequence (catalog_code=?)
LIST SUBQUERY 2
SCAN 2 CONSTANT ROWS

The cost is therefore Σ(rows in every catalog named in the chunk) — independent of how many listings you actually passed. WDS holds 131,303 of catalog_objects' 151,170 rows, so one logged double star drags the entire WDS index range into every chunk that mentions it.

Measured against the shipped pifinder_objects.db, all three implementations returning byte-identical dicts:

scenario	n	loop (before #623)	row-value (this PR)	grouped (pushed)
110 Messier only	110	0.63 ms	0.12 ms	0.11 ms
110 Messier + 1 WDS double	111	0.67 ms	21.83 ms	0.12 ms
138 M/NGC/IC mix	138	0.89 ms	2.38 ms	0.24 ms
500 M/NGC/IC mix	500	3.19 ms	3.07 ms	0.62 ms
2000 random listings	2000	14.07 ms	157.48 ms	3.16 ms

So the 1.6 ms you measured is real, but it's the shape of a log confined to small catalogs. As soon as a big catalog appears the row-value form is slower than the per-listing loop it replaces — a regression on exactly the path this set out to speed up. Nothing wrong with the diagnosis or the goal, just that one SQL form.

What I changed it to

Group by catalog code, then an IN list on sequence within each:

WHERE catalog_code = ? AND sequence IN (?, ?, ?, …)

which SQLite plans as a real probe per listing:

SEARCH catalog_objects USING INDEX idx_catalog_objects_code_sequence (catalog_code=? AND sequence=?)

There are only 21 distinct catalog codes, so this is bounded at a handful of queries no matter how large the log grows — which preserves your actual objective (cache-build cost decoupled from log size). Strictly speaking it's now one query per catalog rather than literally one, but the cache builder still makes a single call, and test_cache_resolves_all_listings_in_one_query passes unchanged.

Chunking is kept at 400 per catalog. FWIW I checked the headroom: 800 bound parameters is under the 999 default SQLITE_MAX_VARIABLE_NUMBER, and the compound-select limit isn't reached even at 1000 rows, so 400 is comfortable either way.

Regression test

test_bulk_listing_lookup_uses_an_index in test_catalog_object_indexes.py asserts the query plan rather than the result — it captures the statements the method issues and requires each to use idx_catalog_objects_code_sequence with no SCAN. I verified it fails against the previous implementation. The existing test_objects_db_listings.py fixture has 3 rows in 2 catalogs, so it could never have caught this; plans need asserting explicitly.

The None guard

load_observed_objects_cache() dropped the loop's object_id is not None check and kept only object_id >= 0. catalog_objects.object_id is nullable, so a resolved listing can carry a NULL, and None >= 0 raises TypeError. The shipped DB has zero such rows so it's latent, but log_object() still carries the guard, so the two paths had drifted apart. Restored it, and changed both return types to Dict[Tuple[str, int], Optional[int]] so the guard isn't sitting there looking like dead code.

One note on the _obs_db() half

Worth stating outright in the description, because it's a stronger result than the millisecond count: every ObservationsDatabase() also lazily opens an ObjectsDatabase via _get_objects_db(), and neither was ever closed — so the old code leaked two sqlite connections on every entry into the object-details screen. That's the bigger fix here.

It's also safe against staleness for a non-obvious reason worth recording: object_details.py touches this handle only through get_logs_for_object() and get_observation_count(), both live queries. It never reads observed_objects_cache / observed_object_ids, so the cache that construction builds was pure waste for this consumer and sharing it can't serve a stale answer. That does mean the cache is now built once per process, so anyone later adding a check_logged() call against this handle would get a stale result — probably worth a line in the comment above _obs_db().

Verified on the branch: 1240 unit tests and 5 smoke tests pass, ruff check, ruff format --check and mypy clean. (Your 5 test_comets failures are local skyfield skew — they pass here.)