FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: Preserve an unset aggregation time window across a proto round-trip by Rodrigo-Palma · Pull Request #6859 · feast-dev/feast · GitHub

Repository navigation

fix: Preserve an unset aggregation time window across a proto round-trip - #6859

Merged
ntkathole merged 1 commit into
feast-dev:masterfrom
Rodrigo-Palma:fix-aggregation-unset-time-window
Sep 26, 2026
Merged

ntkathole merged 1 commit into
feast-dev:masterfrom
Rodrigo-Palma:fix-aggregation-unset-time-window

Conversation

Copy link
Copy Markdown
Contributor

What this PR does / why we need it

Aggregation.__init__ types time_window and slide_interval as Optional, and to_proto honours that: it only writes the Duration when the value is not None. from_proto tests the value instead of the presence:

time_window = (
    timedelta(days=0)
    if agg_proto.time_window.ToNanoseconds() == 0
    else agg_proto.time_window.ToTimedelta()
)

An absent Duration also reports ToNanoseconds() == 0, so an aggregation defined without a window comes back as timedelta(0).

That difference is load bearing. aggregation_specs_to_agg_ops rejects a windowed aggregation in online serving with:

if getattr(agg, "time_window", None) is not None:
    raise ValueError(time_window_unsupported_error_message)

So an on demand feature view written the way the existing tests write it:

@on_demand_feature_view(..., aggregations=[Aggregation(column="trips", function="sum")], mode="python")

works in process, and then fails on the first get_online_features after feast apply, with "Time window aggregation is not supported in online serving.", even though no window was ever requested. Any registry reaches this, since they all round-trip through the proto. StreamFeatureView runs the same conversion.

Aggregation.__eq__ compares both attributes, so a registry diff also sees an unchanged feature view as modified.

Fix

Read the fields through HasField, which is available because both are google.protobuf.Duration message fields and therefore have presence.

Testing

Two tests in test_on_demand_feature_view_aggregation.py: a plain round-trip that asserts None survives, and an end to end one that takes an ODFV through to_proto/from_proto and then serves it. Both fail before the change and pass after. ruff check, ruff format and mypy are clean on the touched files.

Rodrigo-Palma requested a review from a team as a code owner September 24, 2026 00:34

codecov-commenter commented Sep 25, 2026 •
edited
Loading

Copy link
Copy Markdown

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 47.66%. Comparing base (139704f) to head (1bd6944).
⚠️ Report is 1 commits behind head on master.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6859   +/-   ##
=======================================
  Coverage   47.65%   47.66%           
=======================================
  Files         422      422           
  Lines       52396    52396           
  Branches     7606     7606           
=======================================
+ Hits        24970    24973    +3     
  Misses      25632    25632           
+ Partials     1794     1791    -3     
Flag Coverage Δ
go-feature-server 30.58% <ø> (ø)
python-unit 49.00% <ø> (+<0.01%) ⬆️
Files with missing lines Coverage Δ
sdk/python/feast/aggregation/__init__.py 86.66% <ø> (+3.33%) ⬆️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 139704f...1bd6944. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ntkathole force-pushed the fix-aggregation-unset-time-window branch from debce0a to 175e64e Compare September 26, 2026 14:39
ntkathole force-pushed the fix-aggregation-unset-time-window branch from 175e64e to cedef11 Compare September 26, 2026 15:05

Copy link
Copy Markdown
Contributor Author

The lint-python failure is coming from master, not from this PR. detect-secrets flags build_deck.py:10 in the Chronon deck added by #6850, where REV is a commit SHA read as a Hex High Entropy String. #6870 fails on the same hook.

Happy to send a one-line fix adding a pragma: allowlist secret there, or regenerate .secrets.baseline, whichever you prefer.

Aggregation.__init__ types time_window and slide_interval as Optional and
to_proto honours that: it only writes the Duration when the value is not
None. from_proto tests the value instead of the presence, and an absent
Duration also reports ToNanoseconds() == 0, so an aggregation defined
without a window comes back with timedelta(0).

That difference is load bearing. aggregation_specs_to_agg_ops rejects a
windowed aggregation in online serving with 'if getattr(agg,
"time_window", None) is not None', so an on demand feature view with a
plain Aggregation(column=..., function=...) works in process and then
fails with 'Time window aggregation is not supported in online serving.'
on the first get_online_features after feast apply, whatever the
registry. The same round-trip runs for StreamFeatureView.

Read the field through HasField, which is available because both are
google.protobuf.Duration message fields. Aggregation.__eq__ compares
these two attributes, so this also stops a registry diff from seeing an
unchanged feature view as modified.

Signed-off-by: Rodrigo-Palma <email.rodrigopalma@gmail.com>
ntkathole force-pushed the fix-aggregation-unset-time-window branch from cedef11 to 1bd6944 Compare September 26, 2026 16:51
ntkathole merged commit 2000379 into feast-dev:master Sep 26, 2026
19 of 23 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL