Skip to content

fix: Prevent mutation of cached feature resolution results #6722

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
junwoo-choi-rapportlabs wants to merge 1 commit into
base: master
Choose a base branch
from
+38 −0
Open

fix: Prevent mutation of cached feature resolution results #6722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions sdk/python/feast/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1568,6 +1568,13 @@ def _prepare_entities_to_read_from_online_store(
entityless_case,
) = _get_cached_request_context(registry, project, features, full_feature_names)

# Online stores may append internal fields to the requested feature lists.
# Keep those request-local so the cached resolution result remains unchanged.
grouped_refs = [
(feature_view, list(requested_features))
for feature_view, requested_features in grouped_refs
]
Comment on lines +1573 to +1576

@jyejare jyejare Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Consider using copy.deepcopy for more robust copying

While the current list comprehension works for this case, using copy.deepcopy would be more explicit about the intent and handle any future changes to the data structure more robustly.

Suggested:

Suggested change
grouped_refs = [
(feature_view, list(requested_features))
for feature_view, requested_features in grouped_refs
]
import copy
grouped_refs = copy.deepcopy(grouped_refs)


# Mutable copy — downstream code adds join keys to this set.
requested_result_row_names = set(requested_result_row_names_frozen)

Expand Down
31 changes: 31 additions & 0 deletions sdk/python/tests/unit/test_feature_resolution_cache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import pytest

import feast.utils as utils
from feast.protos.feast.types.Value_pb2 import Value as ValueProto


@pytest.fixture(autouse=True)
Expand Down Expand Up @@ -140,6 +141,36 @@ def test_returned_result_row_names_is_mutable_copy(self, mock_ctx):
assert isinstance(result_row_names_1, frozenset)
assert result_row_names_1 == result_row_names_2

@patch("feast.utils._get_online_request_context")
def test_grouped_refs_are_copied_per_request(self, mock_ctx):
"""Downstream mutations must not leak into cached grouped refs."""
mock_ctx.return_value = _make_context()
registry = _make_registry(datetime.now(tz=timezone.utc))
entity_values = {"user_id": [ValueProto(int64_val=1)]}

first_result = utils._prepare_entities_to_read_from_online_store(
registry,
"proj",
["fv:feat1"],
entity_values,
native_entity_values=False,
)
first_requested_features = first_result[1][0][1]
first_requested_features.append("_ts:fv")

second_result = utils._prepare_entities_to_read_from_online_store(
registry,
"proj",
["fv:feat1"],
entity_values,
native_entity_values=False,
)
second_requested_features = second_result[1][0][1]

assert second_requested_features == ["feat1"]
assert second_requested_features is not first_requested_features
mock_ctx.assert_called_once()
Comment on lines +169 to +172

@jyejare jyejare Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Add assertion for first_requested_features content

The test verifies the second request wasn't affected but doesn't assert that the first request's mutation was successful. Adding this assertion would make the test more complete.

Suggested:

Suggested change
assert second_requested_features == ["feat1"]
assert second_requested_features is not first_requested_features
mock_ctx.assert_called_once()
+ assert first_requested_features == ["feat1", "_ts:fv"]
+ assert second_requested_features == ["feat1"]
+ assert second_requested_features is not first_requested_features
+ mock_ctx.assert_called_once()


@patch("feast.utils._get_online_request_context")
def test_feature_service_cached_separately(self, mock_ctx):
"""FeatureService objects should be cached by name."""
Expand Down
Loading