Skip to content

Commit 7e226c4

Browse files
larrysingleton007larrysingleton007
committed
fix: Guard SparkSource, Feature, and data format __eq__ against cross-type comparison
SparkSource.__eq__ passed the shared DataSource base check for any DataSource subclass and then accessed spark-only attributes, so comparing against a FileSource with a matching name raised AttributeError instead of returning False (reported on feast-dev#6636 when swapping a FeatureView's source from FileSource to SparkSource). Feature.__eq__ and the FileFormat/StreamFormat __eq__ accessed attributes of the other operand unguarded and failed the same way; they raised AttributeError rather than the TypeError pattern, which is why the original sweep missed them. Signed-off-by: Larry Singleton <[email protected]>
1 parent 36af191 commit 7e226c4

4 files changed

Lines changed: 44 additions & 5 deletions

File tree

‎sdk/python/feast/data_format.py‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@ def to_proto(self):
3333
pass
3434

3535
def __eq__(self, other):
36+
if not isinstance(other, FileFormat):
37+
return False
3638
return self.to_proto() == other.to_proto()
3739

3840
@classmethod
@@ -95,6 +97,8 @@ def to_proto(self):
9597
pass
9698

9799
def __eq__(self, other):
100+
if not isinstance(other, StreamFormat):
101+
return False
98102
return self.to_proto() == other.to_proto()
99103

100104
@classmethod

‎sdk/python/feast/feature.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,8 @@ def __init__(
5050
self._labels = labels
5151

5252
def __eq__(self, other):
53+
if not isinstance(other, Feature):
54+
return False
5355
if self.name != other.name or self.dtype != other.dtype:
5456
return False
5557
return True

‎sdk/python/feast/infra/offline_stores/contrib/spark_offline_store/spark_source.py‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -262,8 +262,13 @@ def _load_dataframe_from_path(self, spark_session):
262262
return reader.load(self.path)
263263

264264
def __eq__(self, other):
265-
base_eq = super().__eq__(other)
266-
if not base_eq:
265+
# Guard before the spark-specific attribute access below: the base
266+
# DataSource.__eq__ accepts any DataSource subclass, so a cross-type
267+
# comparison (e.g. against a FileSource with a matching name) would
268+
# otherwise raise AttributeError on `other.table` (#6636).
269+
if not isinstance(other, SparkSource):
270+
return False
271+
if not super().__eq__(other):
267272
return False
268273
return (
269274
self.table == other.table

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

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,13 @@
66
object model in one place; the per-type tests for ``DataSource``, ``Entity``,
77
``LabelView``, and ``RoleBasedPolicy`` live in their own modules.
88
9-
The contrib offline sources (athena, couchbase, mssql, oracle, postgres, ray,
9+
Most contrib offline sources (athena, couchbase, mssql, oracle, postgres, ray,
1010
trino) and the optional-dependency transformations are intentionally omitted:
1111
the unit environment does not install their drivers, so they cannot be imported
12-
here. Their ``__eq__`` follows the identical, mechanical pattern.
12+
here. Their ``__eq__`` follows the identical, mechanical pattern. SparkSource
13+
is the exception — its module imports without pyspark, and its ``__eq__``
14+
accesses spark-only attributes after the shared ``DataSource`` base check, so
15+
it gets a dedicated cross-subclass test below.
1316
"""
1417

1518
from datetime import timedelta
@@ -18,8 +21,9 @@
1821

1922
from feast import Entity, FeatureService, FeatureView, Project
2023
from feast.aggregation import Aggregation
21-
from feast.data_format import ProtoFormat
24+
from feast.data_format import ParquetFormat, ProtoFormat
2225
from feast.data_source import KafkaSource, KinesisSource, RequestSource
26+
from feast.feature import Feature
2327
from feast.field import Field
2428
from feast.infra.offline_stores.bigquery_source import BigQuerySource
2529
from feast.infra.offline_stores.file_source import FileSource
@@ -33,12 +37,16 @@
3337
RoleBasedPolicy,
3438
)
3539
from feast.types import Int64
40+
from feast.value_type import ValueType
3641

3742

3843
def _instances():
3944
"""One instance of each importable type touched by the __eq__ sweep."""
4045
return {
4146
"Entity": Entity(name="e"),
47+
"Feature": Feature(name="f", dtype=ValueType.INT64),
48+
"ParquetFormat": ParquetFormat(),
49+
"ProtoFormat": ProtoFormat("com.example.Msg"),
4250
"Project": Project(name="proj"),
4351
"Aggregation": Aggregation(column="c", function="sum"),
4452
"Permission": Permission(name="perm"),
@@ -92,3 +100,23 @@ def test_eq_cross_type_returns_false(name, obj):
92100
assert (obj != object()) is True
93101
# The isinstance guard must not break same-object equality.
94102
assert (obj == obj) is True
103+
104+
105+
def test_spark_source_vs_file_source_eq():
106+
# Reported on #6636: swapping a FeatureView's source from FileSource to
107+
# SparkSource. The two sources share the base DataSource fields, so
108+
# SparkSource.__eq__ used to pass the base check and then raise
109+
# AttributeError on FileSource's missing `table`. Both directions must
110+
# simply compare False.
111+
spark_source = pytest.importorskip(
112+
"feast.infra.offline_stores.contrib.spark_offline_store.spark_source"
113+
)
114+
SparkSource = spark_source.SparkSource
115+
116+
spark = SparkSource(name="src", table="t", timestamp_field="ts")
117+
file = FileSource(name="src", path="/tmp/x.parquet", timestamp_field="ts")
118+
119+
assert (spark == file) is False
120+
assert (file == spark) is False
121+
assert (spark == object()) is False
122+
assert (spark == SparkSource(name="src", table="t", timestamp_field="ts")) is True

0 commit comments

Comments
 (0)