Skip to content

Commit 298c3f6

Browse files
haoxu0haoxu0
authored
fix: Include vector attributes in Field equality (#6934)
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 <hxu44@apple.com>
1 parent bc5aeef commit 298c3f6

2 files changed

Lines changed: 103 additions & 4 deletions

File tree

‎sdk/python/feast/field.py‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,13 @@ def __init__(
7777
self.tags = tags or {}
7878
self.vector_index = vector_index
7979
self.vector_length = vector_length
80-
self.vector_search_metric = vector_search_metric
80+
# The proto default for an unset metric is "", while the constructor default
81+
# is None. Normalizing here rather than in from_proto covers every
82+
# construction path, including callers that build a Field straight from a
83+
# proto message without going through from_proto. Without this, a Field and
84+
# its own deserialized copy compare unequal, which is why the vector
85+
# comparisons in __eq__ were previously disabled.
86+
self.vector_search_metric = vector_search_metric or None
8187

8288
def __eq__(self, other):
8389
if type(self) != type(other):
@@ -89,8 +95,8 @@ def __eq__(self, other):
8995
or self.description != other.description
9096
or self.tags != other.tags
9197
or self.vector_length != other.vector_length
92-
# or self.vector_index != other.vector_index
93-
# or self.vector_search_metric != other.vector_search_metric
98+
or self.vector_index != other.vector_index
99+
or self.vector_search_metric != other.vector_search_metric
94100
):
95101
return False
96102
return True

‎sdk/python/tests/unit/test_eq_cross_type.py‎

Lines changed: 94 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@
3636
NamespaceBasedPolicy,
3737
RoleBasedPolicy,
3838
)
39-
from feast.types import Int64
39+
from feast.types import Array, Float32, Int64
4040
from feast.value_type import ValueType
4141

4242

@@ -120,3 +120,96 @@ def test_spark_source_vs_file_source_eq():
120120
assert (file == spark) is False
121121
assert (spark == object()) is False
122122
assert (spark == SparkSource(name="src", table="t", timestamp_field="ts")) is True
123+
124+
125+
def test_field_eq_includes_vector_attributes():
126+
"""vector_index and vector_search_metric participate in Field equality.
127+
128+
They were previously excluded because an unset metric deserialized as "" while
129+
the constructor default was None, so a Field never compared equal to its own
130+
deserialized copy. With that normalized, a change to either attribute is a real
131+
change and must be visible to feast plan / apply.
132+
"""
133+
base = Field(
134+
name="embedding",
135+
dtype=Array(Float32),
136+
vector_index=True,
137+
vector_length=4,
138+
vector_search_metric="COSINE",
139+
)
140+
141+
assert base == Field(
142+
name="embedding",
143+
dtype=Array(Float32),
144+
vector_index=True,
145+
vector_length=4,
146+
vector_search_metric="COSINE",
147+
)
148+
149+
# Changing the metric alters retrieval semantics and must not compare equal.
150+
assert (
151+
base
152+
== Field(
153+
name="embedding",
154+
dtype=Array(Float32),
155+
vector_index=True,
156+
vector_length=4,
157+
vector_search_metric="L2",
158+
)
159+
) is False
160+
161+
# Turning the index off must not compare equal either.
162+
assert (
163+
base
164+
== Field(
165+
name="embedding",
166+
dtype=Array(Float32),
167+
vector_index=False,
168+
vector_length=4,
169+
vector_search_metric="COSINE",
170+
)
171+
) is False
172+
173+
174+
@pytest.mark.parametrize(
175+
"field",
176+
[
177+
Field(name="plain", dtype=Int64),
178+
Field(
179+
name="embedding", dtype=Array(Float32), vector_index=True, vector_length=4
180+
),
181+
Field(
182+
name="embedding",
183+
dtype=Array(Float32),
184+
vector_index=True,
185+
vector_length=4,
186+
vector_search_metric="COSINE",
187+
),
188+
],
189+
ids=["plain", "vector_without_metric", "vector_with_metric"],
190+
)
191+
def test_field_survives_a_proto_round_trip(field):
192+
"""A Field must compare equal to its own deserialized copy.
193+
194+
This is the backwards-compatibility property that previously forced the vector
195+
comparisons out of __eq__: without it, every field lacking an explicit metric
196+
would show as changed on every feast plan.
197+
"""
198+
assert Field.from_proto(field.to_proto()) == field
199+
200+
201+
def test_field_normalizes_an_empty_metric_to_none():
202+
"""An empty metric means unset, whichever construction path supplied it.
203+
204+
Normalizing in __init__ rather than from_proto matters because callers such as
205+
OnDemandFeatureView._parse_features_from_proto build a Field straight from a
206+
proto message without going through from_proto.
207+
"""
208+
assert (
209+
Field(name="x", dtype=Int64, vector_search_metric="").vector_search_metric
210+
is None
211+
)
212+
assert Field(name="x", dtype=Int64).vector_search_metric is None
213+
assert Field(name="x", dtype=Int64, vector_search_metric="") == Field(
214+
name="x", dtype=Int64
215+
)

0 commit comments

Comments
 (0)