Skip to content

fix: Compare aggregations, mode and tiling in StreamFeatureView equality - #7034

Open
vidigoat wants to merge 2 commits into
feast-dev:masterfrom
vidigoat:fix/stream-feature-view-eq
Open

vidigoat wants to merge 2 commits into
feast-dev:masterfrom
vidigoat:fix/stream-feature-view-eq

Conversation

@vidigoat

@vidigoat vidigoat commented Oct 18, 2026 •

Copy link
Copy Markdown

What this PR does / why we need it:

StreamFeatureView.__eq__ returns early when neither view has a udf:

if not self.udf:
    return not other.udf

so for UDF-less stream feature views (the common case for aggregation-only views) mode, timestamp_field, udf_string and aggregations are never compared. enable_tiling and tiling_hop_size are not compared at all, although both are serialized to the proto. As a result, two stream feature views with different aggregations compare equal:

a = StreamFeatureView(..., aggregations=[Aggregation(column="f", function="sum", time_window=timedelta(hours=1))])
b = StreamFeatureView(..., aggregations=[Aggregation(column="f", function="max", time_window=timedelta(days=7))])
a == b                         # True on master
a.to_proto() == b.to_proto()   # False

The SQL registry relies on this equality: _apply_object skips the write when existing_obj == obj, and apply_feature_view uses != to raise FeatureViewPinConflict when a pinned version is combined with a modified definition. On current master those paths happen to still work, because the registry copy of a stream feature view lists the join key in features (fixed in #7033) and that difference makes FeatureView.__eq__ return False. With #7033 applied and without this change, I reproduced end to end with a SQLite-backed SQL registry that:

  • re-applying the view with aggregations changed from sum/1h to max/7d is silently skipped (the registry keeps sum/1h), and
  • pinning version="v0" while also changing the aggregations does not raise FeatureViewPinConflict.

So this should land before (or together with) #7033.

The fix compares these fields regardless of whether a UDF is set, and adds enable_tiling and tiling_hop_size. mode and udf_string are compared in normalized form (mode_to_string(...), and None treated as ""), matching how to_proto() writes them. Otherwise a view and its from_proto(to_proto(...)) copy would compare unequal, which would make every apply look like a change and create extra version snapshots.

Which issue(s) this PR fixes:

No existing issue; found while working on #7033. I searched open and closed issues and PRs for a duplicate.

Checks

  • I've made sure the tests are passing.
  • My commits are signed off (git commit -s)
  • My PR title follows conventional commits format

Testing Strategy

  • Unit tests
  • Integration tests
  • Manual tests
  • Testing is not required for this change

test_stream_feature_view_equality_with_udf covers the UDF branches (only one view has a udf, or both have different udfs). New parametrized test test_stream_feature_view_equality_without_udf (aggregations, mode, timestamp_field, enable_tiling, tiling_hop_size): all 5 cases fail on master and pass with this change; the existing round-trip test test_stream_feature_view_initialization_with_optional_fields_omitted still passes thanks to the normalization above. Manual end-to-end check with a SQLite-backed SQL registry, both with and without #7033 applied: changed aggregations are written, the pin conflict is raised, and re-applying an unchanged view twice creates no new version snapshot. ruff check, ruff format --check and mypy feast pass.

Misc

Developed with Claude Code (AI) on my behalf.

🤖 Generated with Claude Code

StreamFeatureView.__eq__ returned early when neither view had a udf, so
mode, timestamp_field, udf_string and aggregations were never compared
for UDF-less stream feature views, and enable_tiling / tiling_hop_size
were not compared at all. The SQL registry relies on this equality to
skip unchanged writes and to detect version pin conflicts.

Compare these fields regardless of the udf, normalizing mode and
udf_string the way to_proto() writes them so that a view still equals
its from_proto(to_proto()) round trip.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Vidit Patankar <vidit.patankar16@gmail.com>
@codecov-commenter

codecov-commenter commented Oct 18, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.00%. Comparing base (c9b9d72) to head (970e2fd).
⚠️ 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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7034      +/-   ##
==========================================
+ Coverage   50.98%   51.00%   +0.01%     
==========================================
  Files         455      455              
  Lines       56964    56964              
  Branches     8313     8313              
==========================================
+ Hits        29044    29054      +10     
+ Misses      25935    25927       -8     
+ Partials     1985     1983       -2     
Flag Coverage Δ
go-feature-server 30.58% <ø> (ø)
python-unit 52.46% <100.00%> (+0.01%) ⬆️
Files with missing lines Coverage Δ
sdk/python/feast/stream_feature_view.py 74.37% <100.00%> (+4.02%) ⬆️

... 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 c9b9d72...970e2fd. 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.

Exercise the cases where only one view has a udf and where both have different udfs, which were not covered by the UDF-less equality test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Vidit Patankar <vidit.patankar16@gmail.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants