Speed up transform_single_time_series! for many SingleTimeSeries#594
Conversation
transform_single_time_series! issued roughly 4 SQLite queries per SingleTimeSeries: three in the check phase (forecast parameters, an existing-Deterministic check, and an existing-DeterministicSingleTimeSeries check) and one INSERT per series in the add phase. At tens of thousands of series this dominated runtime. Check phase (_check_transform_single_time_series): - Memoize get_forecast_parameters by (resolution, interval); it is nearly constant across series. - Fetch once the set of owner UUIDs that actually own a Deterministic (and, when skip_existing, a DeterministicSingleTimeSeries) forecast, and only run the per-component metadata query for owners in those sets. When no such forecasts exist the per-component queries are skipped entirely. This is exact: an owner not in the set could not have matched the old query. Add phase: - Add a vectorized add_metadata!(store, owners, all_metadata) that issues a single executemany INSERT instead of one INSERT per series, and use it from _transform_single_time_series!. executemany uses a savepoint when already inside begin_time_series_update's transaction, so nesting is safe. - Factor the associations column list into ASSOCIATIONS_TABLE_COLUMNS. Behavior is preserved (conflict detection and skip_existing idempotency are unchanged); only provably non-matching queries are avoided. Benchmark with 10,000 SingleTimeSeries: ~3.67s -> ~0.40s (~9x), with better scaling as the count grows. Full test suite passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR improves the scalability of transform_single_time_series! when converting large numbers of SingleTimeSeries by reducing per-series SQLite query overhead and batching inserts into the metadata associations table.
Changes:
- Added
ASSOCIATIONS_TABLE_COLUMNSand reused it to keep insert column ordering consistent across insert paths. - Introduced a batched
add_metadata!overload that performs a singleexecutemanyinsert for many metadata rows. - Optimized
_check_transform_single_time_seriesby memoizing forecast parameters and precomputing owner UUID sets to avoid unnecessary per-component metadata queries.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/time_series_metadata_store.jl | Adds a shared associations column list and a new batched add_metadata! that uses executemany to insert many metadata rows efficiently. |
| src/system_data.jl | Switches transform add-phase to the batched insert API and reduces check-phase query volume via caching and owner prefiltering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot review: typing the batched add_metadata! to Vector needlessly excluded views/SubArray/StaticArrays and other AbstractVector types with no correctness benefit. The body only uses length/eachindex/indexing, so it stays fully type-specialized per concrete input and does not become ambiguous with the scalar owner method. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`MetadataStore::list` fetched matching rows in one query, then hydrated each row's features with a separate `fetch_features` call that used `prepare()` rather than `prepare_cached()`, re-parsing the SQL per row. `transform_single_time_series` calls `list` three times, so a transform over 50k series issued ~150k feature queries — the same shape as the InfrastructureSystems.jl bug fixed in Sienna-Platform/InfrastructureSystems.jl#594, one layer below the transform rather than in it. The transform's own structure was already sound: the check phase was bulk, and the writes already batched into one transaction with `prepare_cached`. - `list` now hydrates every matched row's features in a single query, reusing the row query's predicate as a subquery. An `id IN (...)` list would hit SQLite's bound-parameter ceiling on a large store. - Add `list_identities`, which reads the stored `features_hash` column directly. The existing-DST and existing-Deterministic dedup sets only test identity, so they no longer hydrate and re-hash the features of every forecast in the store to rebuild a hash the catalog already has. - Push the transform's `owner_category` and `resolution` arguments into SQL instead of listing every SingleTimeSeries and dropping the misses. Both dedup sets compare periods in their stored ISO-8601 encoding, which is what the `uq_assoc` unique index itself keys on, so dedup and the constraint that fires on INSERT now agree by construction. Named `AssociationIdentity` / `SeriesFamily` replace two six-field tuples, which drops the `clippy::type_complexity` allows. Both filter arguments had no test coverage — every call site passed `None, None` — so add a test that pins the narrowing and idempotency. 50k series x 5 features, file-backed: first transform 1.44s -> 1.10s, idempotent re-transform 1.01s -> 0.18s (5.7x). Scaling stays linear. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
josephmckinsey
left a comment
josephmckinsey
left a comment
There was a problem hiding this comment.
Looks good enough as is, but there is some minor code duplication and potential future problems.
| _get_owner_category(owner), | ||
| _convert_ts_type_to_string(time_series_metadata_to_data(metadata)), | ||
| make_features_string(metadata.features), | ||
| isnothing(sfm) ? missing : JSON3.write(serialize(sfm)), |
There was a problem hiding this comment.
Should we extract out this logic since it's duplicated between both add_metadata! functions?
There was a problem hiding this comment.
This is now obsolete. The bulk add_metadata! method added by Claude in round 1 was unnecessary. The SQLite.jl implementation of executemany calls execute in a for loop, so there was no point.
| ) | ||
| existing = list_metadata( | ||
| data.time_series_manager.metadata_store, | ||
| store, |
There was a problem hiding this comment.
Since this still keeps a potential O(n) number of SQL queries, this could still bite us badly if someone actually calls transform_single_time_series! twice with skip_existing=true, since it would hang just as badly. Should that just be user error or would a warning be appropriate?
An alternative implementation would be to write a query joining all single time series to all "compatible" deterministic/deterministic single time series instead of searching each time, or creating a dict to search instead. I imagine that we will be replacing this once the new time series library is available, in which case, maybe just a warning?
There was a problem hiding this comment.
Good catch — that was a real hole in the first version. The owner-UUID pre-filter only helped when few owners had forecasts, and after one transform_single_time_series! every owner is in the DeterministicSingleTimeSeries set, so a second call fell straight back to N per-component queries. The idempotent re-run was the worst case, not the best one.
I took the dict approach you suggested rather than a warning. _check_transform_single_time_series now fetches all Deterministic/DeterministicSingleTimeSeries metadata in a single list_metadata_with_owner_uuid call and indexes it by (owner_uuid, name, resolution); both the conflict check and the skip_existing check run in memory against that lookup. The per-iteration list_metadata calls are gone.
There is now no SQL per series at all in that loop: get_forecast_parameters is memoized on (resolution, interval), get_component is an in-memory component_uuids lookup, and _check_single_time_series_transformed_parameters is pure metadata arithmetic. The check phase is two bulk queries plus one get_forecast_parameters per distinct (resolution, interval), regardless of series count — so a second call costs the same as the first and a warning doesn't seem warranted. check_transform_single_time_series gets the same benefit, since it runs the check with skip_existing = true. (For anyone reading this later: the public spelling of this path is delete_existing = false.)
Two trade-offs worth naming. Memory: the check now materializes all forecast metadata up front, alongside the SingleTimeSeries list it already held, so peak metadata during a re-run roughly doubles. Semantics: the skip_existing predicate is now an exact feature match in memory (_features_contain) instead of SQL LIKE, which is stricter — it can skip fewer entries but never more. Copilot raised that same point on line 714 and I'm treating it as a separate thread.
And agreed that this is a stopgap until the new time-series library lands.
|
@daniel-thom what's the status of this? |
The batched add_metadata! provided no benefit: DBInterface.executemany is a per-row execute loop in a transaction, identical to looping the single-row method with cached statements inside begin_time_series_update. Benchmarks showed it equal or slower (50k rows: ~370 ms loop vs ~580 ms batched), so remove it and revert the call site to the loop. Address review feedback on repeated transform_single_time_series! calls with skip_existing=true: instead of prefiltering by owner UUID sets and issuing a per-component list_metadata query (O(n) queries once owners have forecasts), fetch all Deterministic/DeterministicSingleTimeSeries metadata with one query, index by (owner_uuid, name, resolution), and run the conflict and skip_existing checks in memory. 10k series: first transform 0.27s -> 0.12s, second call with delete_existing=false 0.15s -> 0.06s. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The _features_contain helper is an exact per-pair match, stricter than the SQL LIKE-based partial feature filter it replaced, which could over-match on substrings (key=1 matching key=10) and on LIKE wildcard characters in string values. Reword the comment to state that intent and use === instead of isequal so Bool and Int feature values do not cross-match (isequal(true, 1) is true in Julia). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem
transform_single_time_series!was very slow with tens of thousands ofSingleTimeSeriesinstances. It issued roughly 4 SQLite queries per series:_check_transform_single_time_series), per series:get_forecast_parameters— even though it depends only on(resolution, interval), which is nearly constant across series.list_metadatafor an existingDeterministicforecast — run on every component, including those with no forecasts.list_metadatafor an existingDeterministicSingleTimeSeries(whenskip_existing).add_metadata!.At 50k series that's ~200k queries.
Changes
Check phase
get_forecast_parametersby(resolution, interval).Deterministic(and, whenskip_existing, aDeterministicSingleTimeSeries) forecast, and only run the per-component metadata query for owners in those sets. When no such forecasts exist, all per-component queries are skipped. This is exact — an owner not in the set could not have matched the old query.Add phase
add_metadata!(store, owners, all_metadata)that issues a singleexecutemanyINSERT instead of one INSERT per series, and use it from_transform_single_time_series!.executemanyuses a savepoint when already insidebegin_time_series_update's transaction, so nesting is safe.ASSOCIATIONS_TABLE_COLUMNS.Behavior is preserved: conflict detection and
skip_existingidempotency are unchanged; only provably non-matching queries are avoided.Verification
SingleTimeSeries, post-warmup): ~3.67s → ~0.40s (~9×), with better scaling as the count grows (old per-component cost was linear).🤖 Generated with Claude Code