Summarise which sources recorded for which sensors

[Flix6x]

Closes #2381. Part of #2377. Based on #2378, so review that first — this targets its branch and the diff will shrink once it merges.

What

Adds a sensor_data_source (sensor_id, source_id) summary table, so the two accessors that ask "which sources recorded for which sensors" stop reading timed_belief.

Why

accessor	today
Sensor.data_sources	SELECT DISTINCT source_id FROM timed_belief WHERE sensor_id = ?
DataSource.sensors	SELECT DISTINCT sensor_id FROM timed_belief WHERE source_id = ?

Both answer a question about a relation bounded by sensors × sources — a few thousand rows at most — by reading the largest table in the database.

#2378 makes the second one worse: once the primary key leads with sensor_id, nothing leads with source_id, so it degrades to a sequential scan. An index on source_id would fix that, but at a large fraction of what #2378 reclaims, to answer a question about a few thousand rows. This is a data-modelling problem, not an indexing one.

Maintained by a trigger, and that was not the first design

The maintenance hook originally lived in TimedBelief.add_to_session, on the reasoning that every save funnels through it.

Two existing tests then failed with empty results — because they insert beliefs without going through that path. So do bulk inserts, COPY, plugins and raw SQL. A hook would have produced a summary that is silently incomplete in exactly the cases nobody checks.

So a statement-level trigger maintains it instead: unbypassable whatever the insert route, and one small upsert per insert statement rather than per row, using a transition table. A test inserts through raw SQL specifically to pin that.

The trigger and its function are created both by the migration and by a create_all DDL listener, so a schema built either way behaves identically — the same class of divergence #2378 exists to fix. Both are idempotent, since create_all runs once per test module.

The honest limitation

It is a superset. Pairs are added on insert and not removed on delete, because deciding whether a pair went stale needs exactly the scan this table avoids. A row means "this source has recorded for this sensor at some point", not "right now".

That is the right trade for the discovery and filtering these accessors feed, and Sensor.search_data_sources still reads timed_belief whenever time filters are given — so time-bounded questions stay exact. Both properties have tests.

Verification

Against PostgreSQL:

• the backfill recovers exactly the distinct pairs — 6 rows from 12,000 belief rows in the fixture;
• deleting a data source cascades its summary rows away;
• the downgrade removes trigger, function and table;
• a raw insert that bypasses save_to_db still records the pair.

flexmeasures/data: 256 passed, with the two pre-existing test_closest_sensor failures unchanged (environmental — my sandbox lacks cube/earthdistance).

Not yet measured

The real distinct-pair count at production scale. The bound is sensors × sources; worth confirming when a full dataset is next available, but it does not affect the design.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR introduces a sensor_data_source (sensor_id, source_id) summary table (maintained by a PostgreSQL statement-level trigger) so Sensor.data_sources and DataSource.sensors can answer “which sources recorded for which sensors” without scanning the large timed_belief table.

Changes:

• Add sensor_data_source model + migration with backfill and trigger/function to maintain the summary on inserts into timed_belief.
• Update Sensor.search_data_sources and DataSource.sensors to read from the summary table when no time filters are provided.
• Add dedicated tests covering trigger maintenance (including raw SQL insert), idempotency, and the “superset” semantics; add changelog entry.

Reviewed changes

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

Show a summary per file

File	Description
flexmeasures/data/tests/test_sensor_data_source.py	New tests for summary-table maintenance, superset semantics, and time-filtered correctness
flexmeasures/data/models/time_series.py	Switch unfiltered sensor→sources lookup to sensor_data_source; add create_all DDL hook to create trigger/function on PostgreSQL
flexmeasures/data/models/data_sources.py	Add SensorDataSource model and switch DataSource.sensors to query via the summary
flexmeasures/data/migrations/versions/f1c8a3d75e29_add_sensor_data_source_table.py	Migration to create/backfill summary table and install trigger/function; downgrade removes them
documentation/changelog.rst	Add Infrastructure/Support changelog entry for the new summary-table lookup

Suppressed comments (3)

flexmeasures/data/models/time_series.py:1226

• Comment wraps mid-phrase (line ends with "insert"), violating the repo’s docstring/comment line-break convention (each line must end with punctuation). See .github/instructions/docstrings.instructions.md:38.

# FOR EACH STATEMENT with a transition table costs one small upsert per insert
# statement, however many rows that statement carries, rather than one per row.

flexmeasures/data/models/data_sources.py:361

• This docstring wraps mid-phrase (line ends with source_id), violating the repo’s docstring/comment line-break convention (each line must end with punctuation). See .github/instructions/docstrings.instructions.md:38.

        Reads the ``sensor_data_source`` summary rather than ``timed_belief``.
        Answering this from the beliefs table would mean scanning it in ``source_id``
        order, which no index serves, to produce a handful of rows.

flexmeasures/data/migrations/versions/f1c8a3d75e29_add_sensor_data_source_table.py:13

• The migration module docstring wraps mid-phrase (line break between “rather than” and “by application code”), violating the repo’s docstring/comment line-break convention (each line must end with punctuation). See .github/instructions/docstrings.instructions.md:38.

The table is kept current by a statement-level trigger on ``timed_belief`` rather than
by application code.

[read-the-docs-community]

Documentation build overview

📚 flexmeasures | 🛠️ Build #33897862 | 📁 Comparing e04891c against latest (ded6de9)

  🔍 Preview build  

5 files changed · ± 5 modified

± Modified

• changelog.html
• genindex.html
• _autosummary/flexmeasures.data.models.data_sources.html
• _autosummary/flexmeasures.data.models.time_series.html
• api/v3_0.html

[Flix6x]

All four addressed in e0e29115, along with three more the review did not flag — found by scanning every added prose line against the convention rather than only the reported ones.

Worth saying plainly: this is the third review round across these branches to raise the same rule, and I had already saved it after the first. Remembering it while writing has not worked; running the scan before pushing does. That is what I will do on the remaining PRs.

flexmeasures/data: 256 passed, with the two pre-existing test_closest_sensor failures unchanged (environmental).

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

flexmeasures/data/models/time_series.py:1255

• The trigger DDL is attached to TimedBelief.__table__'s after_create event, but the DDL references sensor_data_source. Because there is no FK dependency between timed_belief and sensor_data_source, create_all() table creation order can change and could attempt to create the trigger before sensor_data_source exists, which would fail schema creation. Attaching this DDL to db.metadata after_create makes it robust by running after all tables are created.

event.listen(
    TimedBelief.__table__,
    "after_create",
    RECORD_SENSOR_DATA_SOURCES_DDL.execute_if(dialect="postgresql"),
)

flexmeasures/data/models/time_series.py:785

• Sensor.data_sources now delegates to search_data_sources(), which (without time filters) uses the sensor_data_source summary rather than scanning timed_belief. The data_sources docstring still describes the old distinct-from-timed_belief approach and claims scalability properties that no longer match the implementation.

This issue also appears on line 1251 of the same file.

                DataSource.id.in_(
                    select(SensorDataSource.source_id).where(
                        SensorDataSource.sensor_id == self.id
                    )
                )

[Flix6x]

Round 2 addressed in 5ddd23d7. Both suppressed comments were worth acting on — thanks for surfacing them.

Trigger DDL ordering. Moved the listener from timed_belief's after_create to the metadata's, so it runs once every table exists.

One correction to the reasoning, though: the failure described would not actually happen. plpgsql resolves table references lazily, so creating the function and trigger succeeds even when sensor_data_source does not exist yet — I verified that directly against PostgreSQL rather than assuming, which is also why the tests passed. But relying on late binding is a fragile reason for it to work: a function written in plain SQL would be bound eagerly and fail. So the change is right even though the mechanism isn't.

That meant turning the DDL object into a small Python listener, since guarding it in SQL would have meant nesting dollar-quoted bodies inside a DO block. It now skips quietly unless both tables are present, so a partial create_all cannot break.

Stale docstrings. Correct, and a fair hit — I had flagged that exact claim as misleading when writing the issue, then left it in place. Sensor.data_sources no longer claims to scale by not fetching every belief row, and search_data_sources now says the one thing a caller needs: where its answer comes from depends on whether time filters are given, and the filter-free path is a superset.

flexmeasures/data: 256 passed, two pre-existing test_closest_sensor failures unchanged.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

[Flix6x]

Merge conflict resolved and the base branch merged in (b35e9e30). The only conflict was documentation/changelog.rst: this branch still carried the pre-correction "three indexes" wording next to its own entry, while the base had the corrected text. Kept both entries, with the base's wording.

Also in that commit: the migration ordering fix from the outstanding review comment above.

One housekeeping note — the earlier commits on this branch show as Unverified (bad_email). That was my doing: I had been passing -c user.email=... to git commit, which made the GPG signature stop matching the committer email. This commit omits the override and verifies cleanly. Fixing the earlier ones would mean rewriting history, so I have left them.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (3)

flexmeasures/data/tests/test_sensor_data_source.py:94

• This idempotency assertion can’t detect duplicates because pairs() is a set (and even if it were a list, the table’s primary key already prevents duplicates). If the goal is to pin idempotency at the DB level, query the table directly for the row count of the specific pair.

    matching = [p for p in pairs(db) if p == (sensor.id, source.id)]
    assert len(matching) == 1

flexmeasures/data/migrations/versions/f1c8a3d75e29_add_sensor_data_source_table.py:1

• The migration module docstring’s summary line should follow the repo’s docstring convention (capitalized and ending with a period). Most other migrations use this style, and it helps keep autogenerated docs consistent.

"""add the sensor_data_source summary table

flexmeasures/data/tests/test_sensor_data_source.py:18

• pairs() currently returns a set of SQLAlchemy Row objects. The tests then compare those against plain (sensor_id, source_id) tuples; that relies on Row hashing/equality details and is easy to misread. Returning explicit (int, int) tuples makes the intent clear and robust.

This issue also appears on line 93 of the same file.

def pairs(db) -> set:
    return set(
        db.session.execute(
            select(SensorDataSource.sensor_id, SensorDataSource.source_id)
        ).all()

[nhoening]

This looks like a great idea, but I need a little help / better explanations when looking at some details.

[nhoening]

"after_create" is attached to CREATE events.

How are we sure this trigger will be installed? Which CREATE events will install it?
Is it the one in this migration, when the table sensor_data_source is created?
I miss an explicit explanation of this, and the text in lines 1264f is too technical. (What is "plpgsql"?)

[Flix6x]

Good question, and the docstring did not answer it. Rewritten in e04891c6 to name both routes explicitly:

A FlexMeasures database gets its schema in one of two ways, and each needs its own route to the trigger:

• built by Alembic migrations, as in production: migration f1c8a3d75e29 installs it there.
• built by db.create_all(), as in the test suite and some development setups: this function installs it there.

So it is not the migration that triggers this listener — the two are independent paths to the same objects. after_create fires when SQLAlchemy has created the tables in the metadata, which is why it listens on the metadata rather than on timed_belief: listening on the one table would be a bet on creation order, since the trigger's function reads sensor_data_source and nothing guarantees that table is created first.

I dropped the word plpgsql; it now says a function written in PostgreSQL's procedural language does not look up the tables it names until it first runs — which is why the wrong order would happen to work today, and why relying on that is fragile.

[nhoening]

Thanks for the improved explanations!

I see one thing to discuss - we have duplicated SQL functions now, CREATE_FUNCTION and CREATE_TRIGGER in the migration are equal to RECORD_SENSOR_DATA_SOURCES_FUNCTION and RECORD_SENSOR_DATA_SOURCES_TRIGGER i time_series.py.

You wrote that this is on purpose, as the migration should keep doing what it does.

Now, if we change the code in time_series.py one day - so that create_sensor_data_source_trigger() now has some different effect: When will that new trigger ever be installed, in an existing production system? Only when -some day- a new table is created, so that after_create happens as an event, I reckon.
This might be misleading to future developers, who would change the code and expect the trigger will work as they changed it, immeditately.