Fix statistic_during_period wrongly prioritizing ST statistics over LT

homeassistant/components/recorder/statistics.py

@@ -1263,6 +1263,7 @@ def _get_oldest_sum_statistic(
     main_start_time: datetime | None,
     tail_start_time: datetime | None,
     oldest_stat: datetime | None,
+    oldest_5_min_stat: datetime | None,
     tail_only: bool,
     metadata_id: int,
 ) -> float | None:
@@ -1307,6 +1308,15 @@ def _get_oldest_sum_statistic_in_sub_period(
 
     if (
         head_start_time is not None
+        and oldest_5_min_stat is not None
+        and (
+            # If we want stats older than the short term purge window, don't lookup
+            # the oldest sum in the short term table, as it would be prioritized
+            # over older LongTermStats.
+            (oldest_stat is None)
+            or (oldest_5_min_stat < oldest_stat)
+            or (oldest_5_min_stat <= head_start_time)
+        )
         and (
             oldest_sum := _get_oldest_sum_statistic_in_sub_period(
                 session, head_start_time, StatisticsShortTerm, metadata_id
@@ -1518,6 +1528,7 @@ def statistic_during_period(
                     main_start_time,
                     tail_start_time,
                     oldest_stat,
+                    oldest_5_min_stat,
                     tail_only,
                     metadata_id,
                 )

tests/components/recorder/test_websocket_api.py

@@ -794,6 +794,217 @@ def next_id():
     }
 
 
+@pytest.mark.freeze_time(datetime.datetime(2022, 10, 21, 7, 25, tzinfo=datetime.UTC))
+async def test_statistic_during_period_partial_overlap(
+    recorder_mock: Recorder,
+    hass: HomeAssistant,
+    hass_ws_client: WebSocketGenerator,
+) -> None:
+    """Test statistic_during_period."""
+    now = dt_util.utcnow()
+
+    await async_recorder_block_till_done(hass)
+    client = await hass_ws_client()
+
+    zero = now
+    start = zero.replace(hour=0, minute=0, second=0, microsecond=0)
+
+    # Sum shall be tracking a hypothetical sensor that is 0 at midnight, and grows by 1 per minute.
+    # The test will have 4 hours of LTS-only data (0:00-3:59:59), followed by 2 hours of overlapping STS/LTS (4:00-5:59:59), followed by 30 minutes of STS only (6:00-6:29:59)
+    # similar to how a real recorder might look after purging STS.
+
+    # The datapoint at i=0 (start = 0:00) will be 60 as that is the growth during the hour starting at the start period
+    imported_stats_hours = [
+        {
+            "start": (start + timedelta(hours=i)),
+            "sum": (i + 1) * 60,
+        }
+        for i in range(6)
+    ]
+
+    # The datapoint at i=0 (start = 4:00) would be the sensor's value at t=4:05, or 245
+    imported_stats_5min = [
+        {
+            "start": (start + timedelta(hours=4, minutes=5 * i)),
+            "sum": 4 * 60 + (i + 1) * 5,
+        }
+        for i in range(36)
+    ]
+
+    statId = "sensor.test_overlapping"
+    imported_metadata = {
+        "has_mean": False,
+        "has_sum": True,
+        "name": "Total imported energy overlapping",
+        "source": "recorder",
+        "statistic_id": statId,
+        "unit_of_measurement": "kWh",
+    }
+
+    recorder.get_instance(hass).async_import_statistics(
+        imported_metadata,
+        imported_stats_hours,
+        Statistics,
+    )
+    recorder.get_instance(hass).async_import_statistics(
+        imported_metadata,
+        imported_stats_5min,
+        StatisticsShortTerm,
+    )
+    await async_wait_recording_done(hass)
+
+    metadata = get_metadata(hass, statistic_ids={statId})
+    metadata_id = metadata[statId][0]
+    run_cache = get_short_term_statistics_run_cache(hass)
+    # Verify the import of the short term statistics
+    # also updates the run cache
+    assert run_cache.get_latest_ids({metadata_id}) is not None
+
+    # Get all the stats, should consider all hours and 5mins
+    await client.send_json_auto_id(
+        {
+            "type": "recorder/statistic_during_period",
+            "statistic_id": statId,
+            "types": ["change"],
+        }
+    )
+    response = await client.receive_json()
+    assert response["success"]
+    assert response["result"] == {
+        "change": imported_stats_5min[-1]["sum"] - imported_stats_hours[0]["sum"],
+    }
+
+    async def assert_change_during_fixed(client, start_time, end_time, expected):
+        json = {
+            "type": "recorder/statistic_during_period",
+            "types": ["change"],
+            "statistic_id": statId,
+            "fixed_period": {},
+        }
+        if start_time:
+            json["fixed_period"]["start_time"] = start_time.isoformat()
+        if end_time:
+            json["fixed_period"]["end_time"] = end_time.isoformat()
+
+        await client.send_json_auto_id(json)
+        response = await client.receive_json()
+        assert response["success"]
+        assert response["result"] == {
+            "change": expected,
+        }
+
+    # One hours worth of growth in LTS-only
+    start_time = start.replace(hour=1)
+    end_time = start.replace(hour=2)
+    expect = 60
+    await assert_change_during_fixed(client, start_time, end_time, expect)
+
+    # Five minutes of growth in STS-only
+    start_time = start.replace(hour=6, minute=15)
+    end_time = start.replace(hour=6, minute=20)
+    expect = 5
+    await assert_change_during_fixed(client, start_time, end_time, expect)
+
+    # Six minutes of growth in STS-only
+    start_time = start.replace(hour=6, minute=14)
+    end_time = start.replace(hour=6, minute=20)
+    expect = 5
+    await assert_change_during_fixed(client, start_time, end_time, expect)
+
+    # Six minutes of growth in STS-only
+    # 5-minute Change includes start times exactly on or before a statistics start, but end times are not counted unless they are greater than start.
+    start_time = start.replace(hour=6, minute=15)
+    end_time = start.replace(hour=6, minute=21)
+    expect = 10
+    await assert_change_during_fixed(client, start_time, end_time, expect)
+
+    # Five minutes of growth in overlapping LTS+STS
+    start_time = start.replace(hour=5, minute=15)
+    end_time = start.replace(hour=5, minute=20)
+    expect = 5
+    await assert_change_during_fixed(client, start_time, end_time, expect)
+
+    # Five minutes of growth in overlapping LTS+STS (start of hour)
+    start_time = start.replace(hour=5, minute=0)
+    end_time = start.replace(hour=5, minute=5)
+    expect = 5
+    await assert_change_during_fixed(client, start_time, end_time, expect)
+
+    # Five minutes of growth in overlapping LTS+STS (end of hour)
+    start_time = start.replace(hour=4, minute=55)
+    end_time = start.replace(hour=5, minute=0)
+    expect = 5
+    # FIXME: this returns None, I might have expected 5 given the behavior of other 5 minute intervals
+    # await assert_change_during_fixed(client, start_time, end_time, expect)
+
+    # Five minutes of growth in STS-only, with a minute offset. Despite that this does not cover the full period, result is still 5
+    start_time = start.replace(hour=6, minute=16)
+    end_time = start.replace(hour=6, minute=21)
+    expect = 5
+    await assert_change_during_fixed(client, start_time, end_time, expect)
+
+    # 7 minutes of growth in STS-only, spanning two intervals
+    start_time = start.replace(hour=6, minute=14)
+    end_time = start.replace(hour=6, minute=21)
+    expect = 10
+    await assert_change_during_fixed(client, start_time, end_time, expect)
+
+    # One hours worth of growth in LTS-only, with arbitrary minute offsets
+    # Since this does not fully cover the hour, result is None?
+    start_time = start.replace(hour=1, minute=40)
+    end_time = start.replace(hour=2, minute=12)
+    expect = None
+    await assert_change_during_fixed(client, start_time, end_time, expect)
+
+    # One hours worth of growth in LTS-only, with arbitrary minute offsets, covering a whole 1-hour period
+    start_time = start.replace(hour=1, minute=40)
+    end_time = start.replace(hour=3, minute=12)
+    expect = 60
+    # Pre-115291 says "-65"
+    await assert_change_during_fixed(client, start_time, end_time, expect)
+
+    # 90 minutes of growth in window overlapping LTS+STS/STS-only (4:41 - 6:11)
+    start_time = start.replace(hour=4, minute=41)
+    end_time = start_time + timedelta(minutes=90)
+    expect = 90
+    await assert_change_during_fixed(client, start_time, end_time, expect)
+
+    # 4 hours of growth in overlapping LTS-only/LTS+STS (2:01-6:01)
+    start_time = start.replace(hour=2, minute=1)
+    end_time = start_time + timedelta(minutes=240)
+    expect = 185  # 60 from LTS (3:00-3:59), 125 from STS (25 intervals) (4:00-6:01)
+    # Pre-115291 says "120", which is too little
+    await assert_change_during_fixed(client, start_time, end_time, expect)
+
+    # 4 hours of growth in overlapping LTS-only/LTS+STS (1:31-5:31)
+    start_time = start.replace(hour=1, minute=31)
+    end_time = start_time + timedelta(minutes=240)
+    expect = 215  # 120 from LTS (2:00-3:59), 95 from STS (19 intervals) 4:00-5:31
+    # Pre-115291 says "90", which is only the STS window (4:00-5:31) ?
+    await assert_change_during_fixed(client, start_time, end_time, expect)
+
+    # 5 hours of growth, start time only (1:31-end)
+    start_time = start.replace(hour=1, minute=31)
+    end_time = None
+    # will be actually 2:00 - end
+    expect = 4 * 60 + 30
+    # Pre-115291 says "115" (1 hour and 55 minutes)? which I do not understand
+    # FIXME: Postfix says 240, which doesn't seem right either, I would have expected 270 or 275? for 2:00 to the end
+    # await assert_change_during_fixed(client, start_time, end_time, expect)
+
+    # 5 hours of growth, end_time_only (0:00-5:00)
+    start_time = None
+    end_time = start.replace(hour=5)
+    expect = 60 * 5
+    await assert_change_during_fixed(client, start_time, end_time, expect)
+
+    # 5 hours 1 minute of growth, end_time_only (0:00-5:01)
+    start_time = None
+    end_time = start.replace(hour=5, minute=1)
+    expect = 60 * 5 + 5  # 4 hours LTS, 1 hour and 5 minutes STS (4:00-5:01)
+    await assert_change_during_fixed(client, start_time, end_time, expect)
+
+
 @pytest.mark.freeze_time(datetime.datetime(2022, 10, 21, 7, 25, tzinfo=datetime.UTC))
 @pytest.mark.parametrize(
     ("calendar_period", "start_time", "end_time"),