use window approach by hafarooki · Pull Request #1502 · IMAP-Science-Operations-Center/sds-data-manager

hafarooki · 2026-07-18T06:39:42Z

Preface: This is a WIP change that is intended to fix #1493 . At this time, I would just like @tech3371 to test if this fixes the performance issue for the queries mentioned in #1493 in a production-like environment.

Change Summary

Overview

Replaces the correlated-subquery approach for resolving "latest version" of science
files with a single window-function (RANK) query, shared between the query API and
the release API. The old approach re-scanned the science table per candidate row and
got very slow at realistic data volumes; the release API additionally had its own
~180-line copy of the same logic. The release path now also applies releases as a
single bulk UPDATE instead of loading ORM objects and flipping them one at a time.

File changes

latest_version_query.py (new): build_latest_version_query(), which ranks rows
by version within each file series (partitioned on the columns identifying one
logical file) and keeps rank 1. Handles both "newest file" and "all minors of the
latest major".
query_api.py: uses the shared builder. Some incidental cleanup while in here:
table/valid-parameter lookups are now module-level dicts, the version mode is a
StrEnum, and per-parameter filter construction moved into a helper.
release_api.py: deletes query_latest_science_files (including the repointing
special-casing — the window query is NULL-safe on repointing so it no longer needs
two code paths). Ancillary resolution now returns a query used as an IN subquery,
so releasing stays a single UPDATE statement.
models.py: new FILE_ID_COLUMNS constant, also reused for the version index.
tests/: the in-memory SQLite session fixture moved up to the top-level conftest
so it can be shared with a new tests/performance/ directory.

Testing

7 new unit tests in test_version_resolution.py covering per-series resolution,
NULL-safe repointing grouping, filter/ranking interaction, and released-file
visibility for unauthenticated users.
3 new performance tests (query, release, SPICE query) over ~2 years of synthetic
data, with a 2s per-statement budget that fails fast if a correlated-subquery-style
regression sneaks back in.
Removed test_release_repoint_files_date_range since the function it exercised is
gone; its scenarios are covered by the new version-resolution tests.

hafarooki · 2026-07-20T16:42:01Z

(FYI the change summary was generated by Claude since I was in a hurry, sorry if it reads poorly...)

Copilot

Pull request overview

This PR is a WIP performance-focused refactor for “latest version” resolution of science files, replacing correlated-subquery logic with a shared window-function query builder and reusing it across query and release APIs. It also introduces performance tests and consolidates SQLite session test scaffolding so regressions can be caught early.

Changes:

Introduces a shared build_latest_version_query() using a window function to resolve “latest” science files.
Updates query and release lambdas to use the shared builder; release now performs bulk UPDATE operations instead of per-row ORM updates.
Adds unit and performance tests plus shared in-memory SQLite session fixtures.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`sds_data_manager/lambda_code/SDSCode/api_lambdas/latest_version_query.py`	New shared window-function “latest version” query builder for science files.
`sds_data_manager/lambda_code/SDSCode/api_lambdas/query_api.py`	Refactors query parameter handling and uses the shared latest-version builder.
`sds_data_manager/lambda_code/SDSCode/api_lambdas/release_api.py`	Removes duplicated latest-resolution logic; releases via bulk UPDATE and builds ancillary “latest” as an `IN` subquery.
`sds_data_manager/lambda_code/SDSCode/database/models.py`	Adds `FILE_ID_COLUMNS` constant and reuses it for the version index.
`tests/conftest.py`	Adds shared `in_memory_session()` contextmanager for DB-backed tests.
`tests/lambda_endpoints/conftest.py`	Switches endpoint tests to the shared in-memory session helper.
`tests/lambda_endpoints/test_version_resolution.py`	Adds unit tests validating latest-version semantics (per-series, NULL-safe repointing grouping, filter ordering, released visibility).
`tests/lambda_endpoints/test_release_api.py`	Updates ancillary latest-version test to use the new query builder; removes repointing-specific release test.
`tests/performance/conftest.py`	Adds time-constrained SQLite session fixture to fail fast on slow statements.
`tests/performance/test_query_performance.py`	New performance regression test for query API “latest” resolution at scale.
`tests/performance/test_release_performance.py`	New performance regression test for release API latest-resolution + bulk release update at scale.
`tests/performance/test_spice_query_performance.py`	New performance regression test for SPICE query API latest-version resolution at scale.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

hafarooki · 2026-07-20T18:02:25Z

+# Valid query parameters per table: its columns (minus "id"), plus "end_date"
+# for every table but "ancillary", plus the ingestion date range params.
+_VALID_PARAMETERS = {
+    table: [
+        *(column.key for column in model.__table__.columns if column.key != "id"),
+        *(["end_date"] if table != "ancillary" else []),
+        "ingestion_start_date",
+        "ingestion_end_date",
+    ]
+    for table, model in _TABLE_MODELS.items()
+}


Valid issue, I added a test to avoid regressions on this and pushed a change to fix it. I also added a comment clarifying the differences across tables to avoid future confusion. I also removed the id exclusion since none of the included tables have an id column (@tmplummer please confirm)

hafarooki · 2026-07-21T15:27:29Z

    # We want to order the query returns by the filename
    # This will implicitly sort by: instrument, data level, descriptor, start_date, ...
    # Default for the table is by the ascending id so by insertion order
    # This fails for the SPICE table because it uses 'file_name'
-    query = query.order_by(model.file_path)
+    query = query.order_by(cols.file_path)


The problematic comment is from May 2025. It doesn't appear to be true of the latest codebase. I think I need input here on what the expected behavior should be.

hafarooki · 2026-07-20T18:09:23Z

@tmplummer @tech3371 Question: What is the expected behavior for end_date for ancillary files? The ancillary table defines an end date, but the contract for the endpoint is that end_date defines the final start_date. I think this ambiguity may be pre-existing and out of scope for this PR but if I'm modifying that portion of the code it may not hurt to iron out that behavior here.

tech3371 · 2026-07-21T20:59:55Z

@tmplummer @tech3371 Question: What is the expected behavior for end_date for ancillary files? The ancillary table defines an end date, but the contract for the endpoint is that end_date defines the final start_date. I think this ambiguity may be pre-existing and out of scope for this PR but if I'm modifying that portion of the code it may not hurt to iron out that behavior here.

As initial comment, would you like to remove changes in release_api.py to reduce scope of this PR? I am working on release updates and I can add updates for that in separate PR. I just took suggested changes you added as base for my work.

About the ancillary end_date behavior. Ancillary's end_date is optional. So, if no end_date is given, then we take start_date of next ancillary as end_date of previous delivery. Eg. if imap_swapi_some-cal_20260601_v001.csv and imap_swapi_some-cal_20260701_v001.csv. In DB query, imap_swapi_some-cal_20260601_v001.csv's end_date is 20260701. and imap_swapi_some-cal_20260701_v001.csv's end_date is valid now and in the future until next delivery.

hafarooki · 2026-07-21T22:24:17Z

I don't mind removing the changes to release_api.

tmplummer · 2026-07-22T20:06:44Z

@hafarooki, this is still on my radar, but some other pressing matters are keeping me from doing a thorough review of this.

hafarooki · 2026-07-22T20:09:31Z

No rush on my end! I'm happy to work on other things in the meanwhile as well ;)

tech3371 · 2026-07-24T22:22:53Z

I wanted to take some of your code into release work I was working on. I have to get that work in asap for next week. Tagging you for awareness that I used some of your code in this PR already, #1533.

Thank you for these improvements.

tmplummer

This all looks good to me. One comment about how long the new performance tests take to run.

tmplummer · 2026-07-24T22:49:53Z

+        case "start_date":
+            return cols.start_date >= datetime.datetime.strptime(value, "%Y%m%d")
+        case "end_date":
+            # TODO: Need to discuss as a team how to handle date queries. For now,


Is this specific to ancillary files that can have a start and end date? I would think that the desired results would be to provide any ancillary files that would be used for the full range that is queried.

Not sure tbh. That TODO comment was in the code from before my changes

I think we can define our own desired behavior here since it was not defined previously

tmplummer · 2026-07-24T22:52:19Z

I'm curious about how long this takes to run? I have noticed that our sds-data-manager tests are taking close to 10 minutes now and want to be mindful of not adding tests that take too long.

About 300ms

hafarooki · 2026-07-24T22:55:44Z

After my optimizations the performance test takes a few hundred ms to run. Before with no index it took longer than i had the patience to find out

tmplummer

This is a nice improvement with some really useful added testing.

tmplummer · 2026-07-30T16:44:12Z

Sorry for the slow review. There are now some merge conflicts and pre-commit checks that need to be addressed.

hafarooki · 2026-07-30T16:44:20Z

Thanks! I suggest we go ahead and merge it, and open a separate issue for handling of end date for ancillary files, since that behavior was never defined and it's out of scope for this PR.

hafarooki · 2026-07-30T16:44:48Z

I'll resolve the merge conflicts...

Upstream IMAP-Science-Operations-Center#1533 already landed the window-function version resolution as api_lambdas/utils.build_latest_version_query, so this branch's duplicate latest_version_query.py and FILE_ID_COLUMNS definition are dropped in favor of upstream's. release_api.py takes upstream's manifest-line release wholesale; the release performance test now exercises latest_science_release directly since the old lambda params are gone. Assisted-by: Claude

Assisted-by: Claude

supersedes them Assisted-by: Claude

hafarooki · 2026-07-30T17:17:44Z

Should be good to go now

use window approach

0487ea3

hafarooki mentioned this pull request Jul 18, 2026

Science data querying times out and fails for relatively small queries when not specifying a major version #1493

Closed

tmplummer requested review from Copilot and tmplummer July 20, 2026 16:39

Copilot started reviewing on behalf of tmplummer July 20, 2026 16:39 View session

hafarooki marked this pull request as ready for review July 20, 2026 16:40

Copilot AI reviewed Jul 20, 2026

View reviewed changes

fix _VALID_PARAMETERS setup

16773a1

hafarooki mentioned this pull request Jul 20, 2026

BUG - Latest version filter does not appear to work #1506

Closed

hafarooki added 3 commits July 21, 2026 15:05

revert ancillary file handling

886dd15

handle invalid table with error code 400

b160dcd

handle invalid table with error code 400

7dae958

tmplummer reviewed Jul 24, 2026

View reviewed changes

tmplummer approved these changes Jul 30, 2026

View reviewed changes

hafarooki added 3 commits July 30, 2026 12:51

style: apply ruff docstring and format fixes

2c2212c

Assisted-by: Claude

revert release_api changes; upstream IMAP-Science-Operations-Center#1533

b2032cd

supersedes them Assisted-by: Claude

tmplummer merged commit 132f243 into IMAP-Science-Operations-Center:dev Jul 30, 2026
4 checks passed

hafarooki deleted the use-window-instead-of-subquery branch July 30, 2026 21:18

Uh oh!

Conversation

hafarooki commented Jul 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Summary

Overview

File changes

Testing

Uh oh!

hafarooki commented Jul 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

hafarooki Jul 20, 2026

Choose a reason for hiding this comment

Uh oh!

hafarooki Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hafarooki commented Jul 20, 2026

Uh oh!

tech3371 commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hafarooki commented Jul 21, 2026

Uh oh!

tmplummer commented Jul 22, 2026

Uh oh!

hafarooki commented Jul 22, 2026

Uh oh!

tech3371 commented Jul 24, 2026

Uh oh!

tmplummer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tmplummer Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

hafarooki Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

hafarooki Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

tmplummer Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

hafarooki Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

hafarooki commented Jul 24, 2026

Uh oh!

tmplummer left a comment

Choose a reason for hiding this comment

Uh oh!

tmplummer commented Jul 30, 2026

Uh oh!

hafarooki commented Jul 30, 2026

Uh oh!

hafarooki commented Jul 30, 2026

Uh oh!

hafarooki commented Jul 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hafarooki commented Jul 18, 2026 •

edited

Loading

tech3371 commented Jul 21, 2026 •

edited

Loading