From 470e64045abbb60318dcedb0adc012b3e7c6e359 Mon Sep 17 00:00:00 2001 From: hao-xu5 Date: Fri, 2 Oct 2026 17:48:16 -0700 Subject: [PATCH] fix: Include vector attributes in Field equality Re-enables the vector_index and vector_search_metric comparisons in Field.__eq__, which have been commented out since #4855. Addresses item 5 of #6907. They were disabled deliberately, for backwards compatibility: an unset metric deserializes as "" while the constructor default is None, so a Field never compared equal to its own deserialized copy and every field lacking an explicit metric would have shown as changed on every feast plan. Fixing that asymmetry removes the reason for the exclusion. The normalization goes in __init__ rather than from_proto, because from_proto is not the only deserialization path: OnDemandFeatureView._parse_features_from_proto builds a Field straight from a proto message. Normalizing only from_proto left the two paths disagreeing, which surfaced as a projection for an aliased on-demand feature view failing to apply, since the comparison is also what BaseFeatureView.with_projection uses to check a projection's fields. With both attributes compared, a change to vector_search_metric from COSINE to L2, or flipping vector_index, is now visible to feast plan and apply. Previously such a change altered retrieval semantics while producing no diff. Verified no regressions by diffing failure names across the full unit suite before and after: 41 pre-existing failures on both sides, none introduced. Signed-off-by: hao-xu5 --- sdk/python/feast/field.py | 12 ++- sdk/python/tests/unit/test_eq_cross_type.py | 95 ++++++++++++++++++++- 2 files changed, 103 insertions(+), 4 deletions(-) diff --git a/sdk/python/feast/field.py b/sdk/python/feast/field.py index 3055bd70830..d7fddcd53d7 100644 --- a/sdk/python/feast/field.py +++ b/sdk/python/feast/field.py @@ -77,7 +77,13 @@ def __init__( self.tags = tags or {} self.vector_index = vector_index self.vector_length = vector_length - self.vector_search_metric = vector_search_metric + # The proto default for an unset metric is "", while the constructor default + # is None. Normalizing here rather than in from_proto covers every + # construction path, including callers that build a Field straight from a + # proto message without going through from_proto. Without this, a Field and + # its own deserialized copy compare unequal, which is why the vector + # comparisons in __eq__ were previously disabled. + self.vector_search_metric = vector_search_metric or None def __eq__(self, other): if type(self) != type(other): @@ -89,8 +95,8 @@ def __eq__(self, other): or self.description != other.description or self.tags != other.tags or self.vector_length != other.vector_length - # or self.vector_index != other.vector_index - # or self.vector_search_metric != other.vector_search_metric + or self.vector_index != other.vector_index + or self.vector_search_metric != other.vector_search_metric ): return False return True diff --git a/sdk/python/tests/unit/test_eq_cross_type.py b/sdk/python/tests/unit/test_eq_cross_type.py index 7a87dd68f10..f8981155562 100644 --- a/sdk/python/tests/unit/test_eq_cross_type.py +++ b/sdk/python/tests/unit/test_eq_cross_type.py @@ -36,7 +36,7 @@ NamespaceBasedPolicy, RoleBasedPolicy, ) -from feast.types import Int64 +from feast.types import Array, Float32, Int64 from feast.value_type import ValueType @@ -120,3 +120,96 @@ def test_spark_source_vs_file_source_eq(): assert (file == spark) is False assert (spark == object()) is False assert (spark == SparkSource(name="src", table="t", timestamp_field="ts")) is True + + +def test_field_eq_includes_vector_attributes(): + """vector_index and vector_search_metric participate in Field equality. + + They were previously excluded because an unset metric deserialized as "" while + the constructor default was None, so a Field never compared equal to its own + deserialized copy. With that normalized, a change to either attribute is a real + change and must be visible to feast plan / apply. + """ + base = Field( + name="embedding", + dtype=Array(Float32), + vector_index=True, + vector_length=4, + vector_search_metric="COSINE", + ) + + assert base == Field( + name="embedding", + dtype=Array(Float32), + vector_index=True, + vector_length=4, + vector_search_metric="COSINE", + ) + + # Changing the metric alters retrieval semantics and must not compare equal. + assert ( + base + == Field( + name="embedding", + dtype=Array(Float32), + vector_index=True, + vector_length=4, + vector_search_metric="L2", + ) + ) is False + + # Turning the index off must not compare equal either. + assert ( + base + == Field( + name="embedding", + dtype=Array(Float32), + vector_index=False, + vector_length=4, + vector_search_metric="COSINE", + ) + ) is False + + +@pytest.mark.parametrize( + "field", + [ + Field(name="plain", dtype=Int64), + Field( + name="embedding", dtype=Array(Float32), vector_index=True, vector_length=4 + ), + Field( + name="embedding", + dtype=Array(Float32), + vector_index=True, + vector_length=4, + vector_search_metric="COSINE", + ), + ], + ids=["plain", "vector_without_metric", "vector_with_metric"], +) +def test_field_survives_a_proto_round_trip(field): + """A Field must compare equal to its own deserialized copy. + + This is the backwards-compatibility property that previously forced the vector + comparisons out of __eq__: without it, every field lacking an explicit metric + would show as changed on every feast plan. + """ + assert Field.from_proto(field.to_proto()) == field + + +def test_field_normalizes_an_empty_metric_to_none(): + """An empty metric means unset, whichever construction path supplied it. + + Normalizing in __init__ rather than from_proto matters because callers such as + OnDemandFeatureView._parse_features_from_proto build a Field straight from a + proto message without going through from_proto. + """ + assert ( + Field(name="x", dtype=Int64, vector_search_metric="").vector_search_metric + is None + ) + assert Field(name="x", dtype=Int64).vector_search_metric is None + assert Field(name="x", dtype=Int64, vector_search_metric="") == Field( + name="x", dtype=Int64 + )