Skip to content

Commit 2583c45

Browse files
MohammadHijjawi97ntkathole
authored andcommitted
fix: Limit tiled max and min aggregations to the window
The tiling orchestrator ran cumsum over the per-hop max/min values, so tiled max/min added the maxima (or minima) of all hops together, and the tile subtraction step then used that cumulative value as is. The results were wrong as soon as the data spanned more than one hop. max and min cannot be undone by subtraction, so keep their tiles as per-hop values (empty hops stay NaN instead of 0) and compute the windowed value as the max/min over the hops inside the window. Signed-off-by: Mohammad Hijjawi <[email protected]>
1 parent 9340545 commit 2583c45

3 files changed

Lines changed: 72 additions & 13 deletions

File tree

‎sdk/python/feast/aggregation/tiling/orchestrator.py‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
"""
77

88
from datetime import timedelta
9-
from typing import Any, Callable, Dict, List, Tuple, Union
9+
from typing import Any, Callable, Dict, List, Set, Tuple, Union
1010

1111
import pandas as pd
1212

@@ -61,6 +61,9 @@ def apply_sawtooth_window_tiling(
6161
# Step 2: Group by entity keys + hop interval and aggregate
6262
agg_dict: Dict[str, Tuple[str, Union[str, Callable[[Any], Any]]]] = {}
6363
ir_metadata_dict = {}
64+
# max/min cannot be undone by subtraction, so these columns keep per-hop
65+
# values instead of cumulative ones (reduced over the window later).
66+
per_hop_columns: Set[str] = set()
6467

6568
for agg in aggregations:
6669
feature_name = agg.resolved_name(window_size)
@@ -76,8 +79,10 @@ def apply_sawtooth_window_tiling(
7679
agg_dict[f"_tail_{feature_name}"] = (agg.column, "count")
7780
elif agg.function == "max":
7881
agg_dict[f"_tail_{feature_name}"] = (agg.column, "max")
82+
per_hop_columns.add(f"_tail_{feature_name}")
7983
elif agg.function == "min":
8084
agg_dict[f"_tail_{feature_name}"] = (agg.column, "min")
85+
per_hop_columns.add(f"_tail_{feature_name}")
8186

8287
elif metadata.type in ("holistic", "avg", "std", "var"):
8388
# Holistic aggregations: compute IRs
@@ -141,15 +146,11 @@ def apply_sawtooth_window_tiling(
141146
group_df, on=group_by_keys + ["_hop_interval"], how="left"
142147
)
143148

144-
# Fill NaN with 0 for IR columns
149+
# Fill NaN with 0 and compute cumulative sum for IR columns
150+
# (empty hops stay NaN for per-hop max/min columns)
145151
for col in ir_columns:
146-
if col in merged.columns:
147-
merged[col] = merged[col].fillna(0)
148-
149-
# Compute cumulative sum for IR columns
150-
for col in ir_columns:
151-
if col in merged.columns:
152-
merged[col] = merged[col].cumsum()
152+
if col in merged.columns and col not in per_hop_columns:
153+
merged[col] = merged[col].fillna(0).cumsum()
153154

154155
cumulative_results.append(merged)
155156

‎sdk/python/feast/aggregation/tiling/tile_subtraction.py‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,9 @@ def convert_cumulative_to_windowed(
3434
1. Subtracts intermediate representation (IR) components
3535
2. Recomputes the final value from windowed IRs
3636
37+
max and min cannot be undone by subtraction, so their tiles hold per-hop
38+
values and the windowed value is the max/min over the hops in the window.
39+
3740
Args:
3841
tiles_df: DataFrame with cumulative tiles from orchestrator.
3942
Must contain: entity_keys, _tile_end, IR columns, feature columns
@@ -88,11 +91,17 @@ def convert_cumulative_to_windowed(
8891
current_val = float(row[feature_name])
8992

9093
if agg.function.lower() in ("max", "min"):
91-
if has_prev_tile and feature_name in prev_tile.index:
92-
prev_val = float(prev_tile[feature_name])
93-
windowed_row[feature_name] = current_val
94+
# max/min cannot be subtracted: the tiles hold
95+
# per-hop values, so reduce the hops in the window
96+
in_window = group_df.loc[
97+
(group_df["_tile_end"] > window_start)
98+
& (group_df["_tile_end"] <= tile_end),
99+
feature_name,
100+
]
101+
if agg.function.lower() == "max":
102+
windowed_row[feature_name] = float(in_window.max())
94103
else:
95-
windowed_row[feature_name] = current_val
104+
windowed_row[feature_name] = float(in_window.min())
96105
else:
97106
# For sum and count: subtract previous from current
98107
if has_prev_tile and feature_name in prev_tile.index:

‎sdk/python/tests/unit/test_aggregation_ops.py‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,3 +190,52 @@ def test_tiled_holistic_aggregation_matches_pandas(function, expected_fn):
190190
& (df["event_timestamp"] < tile_end)
191191
]["trips"]
192192
assert row[feature_name] == pytest.approx(expected_fn(in_window), nan_ok=True)
193+
194+
195+
@pytest.mark.parametrize("function", ["max", "min"])
196+
def test_tiled_max_min_aggregation_matches_pandas(function):
197+
"""Tiled max/min only consider the hops inside the sliding window."""
198+
from feast.aggregation.tiling.orchestrator import apply_sawtooth_window_tiling
199+
from feast.aggregation.tiling.tile_subtraction import (
200+
convert_cumulative_to_windowed,
201+
)
202+
203+
start = pd.Timestamp("2024-01-01")
204+
# No events between minutes 20 and 35, so two hops are empty.
205+
minutes = [0, 5, 10, 15, 20, 35, 40, 45]
206+
values = [-1.0, -2.0, 30.0, -10.0, 4.0, -7.0, 5.0, -8.0]
207+
df = pd.DataFrame(
208+
{
209+
"driver_id": [1] * len(values),
210+
"event_timestamp": [start + timedelta(minutes=m) for m in minutes],
211+
"trips": values,
212+
}
213+
)
214+
window = timedelta(minutes=20)
215+
agg = Aggregation(column="trips", function=function, time_window=window)
216+
217+
tiles = apply_sawtooth_window_tiling(
218+
df=df.copy(),
219+
aggregations=[agg],
220+
group_by_keys=["driver_id"],
221+
timestamp_col="event_timestamp",
222+
window_size=window,
223+
hop_size=timedelta(minutes=5),
224+
)
225+
result = convert_cumulative_to_windowed(
226+
tiles_df=tiles,
227+
entity_keys=["driver_id"],
228+
timestamp_col="event_timestamp",
229+
window_size=window,
230+
aggregations=[agg],
231+
)
232+
233+
feature_name = agg.resolved_name(window)
234+
assert len(result) == 10
235+
for _, row in result.iterrows():
236+
tile_end = row["event_timestamp"]
237+
in_window = df[
238+
(df["event_timestamp"] >= tile_end - window)
239+
& (df["event_timestamp"] < tile_end)
240+
]["trips"]
241+
assert row[feature_name] == getattr(in_window, function)()

0 commit comments

Comments
 (0)