From 73d3956a6731db36be28fa91418999c0b2fea919 Mon Sep 17 00:00:00 2001 From: saket3395 Date: Thu, 6 Aug 2026 12:06:32 +0530 Subject: [PATCH 1/4] fix: feast apply ignores ttl updates when new ttl is None or timedelta(0) Fixes #6703. Registry._update_metadata_fields() routes ttl changes on re-apply through a truthiness check: if (... and updated_fv.ttl): None and timedelta(0) are both the documented way to express "no ttl", and both are falsy, so re-applying a FeatureView/LabelView with ttl cleared silently kept the old finite ttl -- feast apply reported the update but nothing changed in the registry. Removing that outer truthy gate isn't sufficient by itself: for the FeatureView branch, get_ttl_duration() returns Python None when self.ttl is None, and the existing inner check if ttl_duration: existing_proto.spec.ttl.CopyFrom(ttl_duration) would still silently skip CopyFrom in that case, leaving the stale ttl in place. Fixed both by explicitly writing an empty Duration() (which decodes back to timedelta(0), per FeatureView.from_proto's existing ToNanoseconds()==0 check) whenever there's no real ttl to write, instead of skipping the write. The LabelView branch is adjusted the same way, guarding FromTimedelta() against a None ttl now that the outer gate no longer prevents ttl=None from reaching this branch. Traced all three cases (None, timedelta(0), a finite value) through the new branch logic in isolation and confirmed FeatureView and LabelView now resolve identically: None and timedelta(0) both produce a zero Duration, a finite ttl passes through unchanged. Signed-off-by: saket3395 --- sdk/python/feast/infra/registry/registry.py | 22 +++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/sdk/python/feast/infra/registry/registry.py b/sdk/python/feast/infra/registry/registry.py index 62f445a5681..27b88947309 100644 --- a/sdk/python/feast/infra/registry/registry.py +++ b/sdk/python/feast/infra/registry/registry.py @@ -580,20 +580,22 @@ def _update_metadata_fields( existing_proto.spec.version = getattr(updated_fv, "version") # Configuration fields (FeatureView / LabelView TTL) - if ( - hasattr(existing_proto.spec, "ttl") - and hasattr(updated_fv, "ttl") - and updated_fv.ttl - ): + # Note: don't gate this on `updated_fv.ttl` being truthy -- None and + # timedelta(0) are both the documented way to express "no ttl", and + # are falsy, so that check would silently drop the update exactly + # when a user clears an existing ttl. + if hasattr(existing_proto.spec, "ttl") and hasattr(updated_fv, "ttl"): + from google.protobuf.duration_pb2 import Duration + if isinstance(updated_fv, FeatureView): ttl_duration = updated_fv.get_ttl_duration() - if ttl_duration: - existing_proto.spec.ttl.CopyFrom(ttl_duration) + existing_proto.spec.ttl.CopyFrom( + ttl_duration if ttl_duration is not None else Duration() + ) elif isinstance(updated_fv, LabelView): - from google.protobuf.duration_pb2 import Duration - ttl_duration = Duration() - ttl_duration.FromTimedelta(updated_fv.ttl) + if updated_fv.ttl is not None: + ttl_duration.FromTimedelta(updated_fv.ttl) existing_proto.spec.ttl.CopyFrom(ttl_duration) if hasattr(existing_proto.spec, "online") and hasattr(updated_fv, "online"): existing_proto.spec.online = getattr(updated_fv, "online") From 326215c90fee1c5e094635d77d5cfec9645d291d Mon Sep 17 00:00:00 2001 From: saket3395 Date: Thu, 6 Aug 2026 21:43:03 +0530 Subject: [PATCH 2/4] address review nitpicks: hoist Duration import, guard FromTimedelta - Move the Duration import to the top of the file with the other google.protobuf imports, consistent with how Message and RepeatedCompositeFieldContainer are already imported there. - Wrap FromTimedelta() in the LabelView branch with a try/except, re-raising as a ValueError naming the offending value, so an invalid timedelta surfaces a clear error instead of a raw protobuf exception. Signed-off-by: saket3395 --- sdk/python/feast/infra/registry/registry.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/sdk/python/feast/infra/registry/registry.py b/sdk/python/feast/infra/registry/registry.py index 27b88947309..ae8a2901bad 100644 --- a/sdk/python/feast/infra/registry/registry.py +++ b/sdk/python/feast/infra/registry/registry.py @@ -19,6 +19,7 @@ from typing import Any, Dict, List, Optional, Union from urllib.parse import urlparse +from google.protobuf.duration_pb2 import Duration from google.protobuf.internal.containers import RepeatedCompositeFieldContainer from google.protobuf.message import Message @@ -585,8 +586,6 @@ def _update_metadata_fields( # are falsy, so that check would silently drop the update exactly # when a user clears an existing ttl. if hasattr(existing_proto.spec, "ttl") and hasattr(updated_fv, "ttl"): - from google.protobuf.duration_pb2 import Duration - if isinstance(updated_fv, FeatureView): ttl_duration = updated_fv.get_ttl_duration() existing_proto.spec.ttl.CopyFrom( @@ -595,7 +594,12 @@ def _update_metadata_fields( elif isinstance(updated_fv, LabelView): ttl_duration = Duration() if updated_fv.ttl is not None: - ttl_duration.FromTimedelta(updated_fv.ttl) + try: + ttl_duration.FromTimedelta(updated_fv.ttl) + except (ValueError, OverflowError) as e: + raise ValueError( + f"Invalid TTL value: {updated_fv.ttl}" + ) from e existing_proto.spec.ttl.CopyFrom(ttl_duration) if hasattr(existing_proto.spec, "online") and hasattr(updated_fv, "online"): existing_proto.spec.online = getattr(updated_fv, "online") From cc340e87b775bae24af106a74611fead18bc5c5f Mon Sep 17 00:00:00 2001 From: saket3395 Date: Tue, 11 Aug 2026 18:52:38 +0530 Subject: [PATCH 3/4] test: cover ttl clearing in _update_metadata_fields (#6703) Adds unit coverage requested in review: - clearing a finite ttl to None or timedelta(0) now writes a zero Duration (previously silently dropped) - a finite-to-finite ttl update is preserved _update_metadata_fields uses no instance state, so it is exercised directly via the class, mirroring the FeatureView/FileSource/Field construction used elsewhere in the unit tests. Signed-off-by: saket3395 --- .../registry/test_update_metadata_fields.py | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 sdk/python/tests/unit/infra/registry/test_update_metadata_fields.py diff --git a/sdk/python/tests/unit/infra/registry/test_update_metadata_fields.py b/sdk/python/tests/unit/infra/registry/test_update_metadata_fields.py new file mode 100644 index 00000000000..60b6ed165cc --- /dev/null +++ b/sdk/python/tests/unit/infra/registry/test_update_metadata_fields.py @@ -0,0 +1,53 @@ +"""Unit tests for Registry._update_metadata_fields TTL handling (issue #6703). + +Re-applying a FeatureView with its ttl cleared to ``None`` or +``timedelta(0)`` (both the documented ways to express "no ttl") used to be +silently dropped, because the update was gated on ``updated_fv.ttl`` being +truthy -- and both of those values are falsy. + +``_update_metadata_fields`` does not use any instance state, so it is exercised +directly via the class here rather than standing up a full registry backend. +""" + +from datetime import timedelta + +import pytest + +from feast.entity import Entity +from feast.feature_view import FeatureView +from feast.field import Field +from feast.infra.offline_stores.file_source import FileSource +from feast.infra.registry.registry import Registry +from feast.types import Float32 + + +def _feature_view(ttl): + return FeatureView( + name="fv", + entities=[Entity(name="e", join_keys=["e_id"])], + schema=[Field(name="f1", dtype=Float32)], + source=FileSource(path="file://feast/*", timestamp_field="ts_col"), + ttl=ttl, + ) + + +@pytest.mark.parametrize("cleared_ttl", [None, timedelta(0)]) +def test_update_metadata_fields_clears_ttl(cleared_ttl): + existing_proto = _feature_view(timedelta(days=10)).to_proto() + # sanity: the existing view starts with a finite ttl + assert existing_proto.spec.ttl.ToNanoseconds() != 0 + + updated_fv = _feature_view(cleared_ttl) + Registry._update_metadata_fields(None, existing_proto, updated_fv) + + # the cleared ttl (None / timedelta(0)) must now be reflected as "no ttl" + assert existing_proto.spec.ttl.ToNanoseconds() == 0 + + +def test_update_metadata_fields_preserves_finite_ttl(): + existing_proto = _feature_view(timedelta(days=10)).to_proto() + + updated_fv = _feature_view(timedelta(days=3)) + Registry._update_metadata_fields(None, existing_proto, updated_fv) + + assert existing_proto.spec.ttl.ToTimedelta() == timedelta(days=3) From b34897a5cf86a3390dc203758839c1531094ec97 Mon Sep 17 00:00:00 2001 From: saket3395 Date: Fri, 14 Aug 2026 09:08:58 +0530 Subject: [PATCH 4/4] style: apply ruff format to registry.py ttl block Collapse the multi-line raise ValueError back to a single line per ruff format (it fits within the 88-char limit), fixing the format check. Signed-off-by: saket3395 --- sdk/python/feast/infra/registry/registry.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/sdk/python/feast/infra/registry/registry.py b/sdk/python/feast/infra/registry/registry.py index ae8a2901bad..5737df06881 100644 --- a/sdk/python/feast/infra/registry/registry.py +++ b/sdk/python/feast/infra/registry/registry.py @@ -597,9 +597,7 @@ def _update_metadata_fields( try: ttl_duration.FromTimedelta(updated_fv.ttl) except (ValueError, OverflowError) as e: - raise ValueError( - f"Invalid TTL value: {updated_fv.ttl}" - ) from e + raise ValueError(f"Invalid TTL value: {updated_fv.ttl}") from e existing_proto.spec.ttl.CopyFrom(ttl_duration) if hasattr(existing_proto.spec, "online") and hasattr(updated_fv, "online"): existing_proto.spec.online = getattr(updated_fv, "online")