Fix sso duplication issue and allow for bulk reading of sso file - #39
Open
axf295 wants to merge 3 commits into
Open
Fix sso duplication issue and allow for bulk reading of sso file#39axf295 wants to merge 3 commits into
axf295 wants to merge 3 commits into
Conversation
When a sky box crosses RA=0/360 (lower_left.ra > upper_right.ra), get_box_sso split the query into two RA sub-ranges combined with union_all(). Neither sub-query deduplicated its own outerjoin against moving_sources, so an object with N matching ephemeris points came back as N duplicate rows. The outer .distinct() didn't help either, since it was applied before .from_statement(), which replaces the executed SQL wholesale and silently drops it. Fixed by deduplicating each RA sub-range individually and switching the outer combine from union_all to union, so an object with ephemeris points on both sides of RA=0 within the same window isn't double-counted either. Found via a real production DB (~50M ephemeris rows, 641 SSOs) where a wraparound box for a field straddling RA=0 returned one object duplicated ~83 times, and the resulting per-object get_source_generator fan-out (once per duplicate) took get_box() from ~1s to ~118s. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
RegisteredMovingSourceTable (the ORM model, used by SQLModel.metadata.create_all() -- which is what every Client(db_url=...) call uses when provisioning a fresh database, including the initial Alembic migration's own table) had no index beyond its primary key. The Alembic migration path separately indexed `time` but never `sso_id`, so the two schema-provisioning paths in this repo disagreed with each other, and the one actually used in practice (create_all()) was the worse of the two. Adds a (sso_id, time) composite index to the ORM model (serves both get_ephem_points()/get_source_generator()'s sso_id-first lookups and, via leftmost-prefix, plain sso_id lookups) and makes `time` itself Field(index=True) for get_box_sso()/get_monitored_ssos()/ get_pointing_ssos(), which have no sso_id to filter on. A companion Alembic migration adds the same composite index for databases already provisioned via `alembic upgrade head`, which already had the time index from the initial migration. Verified both provisioning paths (SQLModel.metadata.create_all() and `alembic upgrade head`, including an upgrade/downgrade/upgrade round-trip) now produce the identical index set. Without these, every per-object ephemeris lookup and every get_box_sso call is a full table scan -- fine at test scale, not at the tens of millions of rows a real ephemeris table holds (see simonsobs/sotrplib's scripts/solar_system/build_socat_db.py). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…files socat.ingest.actfits.ingest_fits_file/jplparquet.ingest_jpl_parquet_file go through a ClientBase (one INSERT + COMMIT per row via create_source()/ create_sso()/create_ephem()), which is right for adding a handful of sources to an existing catalog but not for building a database from scratch: a real ephemeris ingest at these files' ~2-hour cadence over 2015-2033 is tens of millions of rows, and one-commit-per-row would take on the order of days. New socat.ingest.bulk_sqlite (CLI: socat-build-db) bulk-loads an ACT FITS catalog and JPL Horizons ephemeris parquet files directly via raw sqlite3.executemany with SQLite tuned for bulk writes, building on local disk before copying to the requested destination (SQLite doesn't behave reliably over network filesystems like Lustre/NFS, which is usually where the final database needs to live). No manual index-creation step is needed -- create_schema() uses socat's own SQLModel metadata, so the indexes added to RegisteredMovingSourceTable in the previous commit are applied automatically. Verified against real production inputs (a 90k-row ACT catalog, a 50M+-row two-file JPL ephemeris set) as well as new synthetic-fixture tests covering schema/index creation, fixed-source loading, designation parsing, ephemeris loading, and a full build() round-trip through a real Client query. Also declares numpy and tqdm as direct dependencies -- both are imported directly (tqdm already was, by the existing jplparquet ingest module, but was undeclared and not actually installable from a clean checkout). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Asked claude how to make it more efficient to read in the ephemerides, and this is what it came up with. also allow indexing by time and by sso_id so that it's much quicker to query.
also include a single script to bulk load act catalog and ssos, since we'll want to do that every time.