fix: Return False from __eq__ on cross-type comparison - #6637
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6637 +/- ##
==========================================
+ Coverage 45.98% 46.35% +0.36%
==========================================
Files 414 414
Lines 50037 50052 +15
Branches 7147 7151 +4
==========================================
+ Hits 23012 23201 +189
+ Misses 25413 25229 -184
- Partials 1612 1622 +10
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
@sudohainguyen - awaiting your review please |
There was a problem hiding this comment.
Pull request overview
This PR fixes Feast registry-object equality semantics by making cross-type __eq__ comparisons return False instead of raising TypeError, preventing feast apply crashes when an object’s concrete type changes (e.g., switching a feature view’s data source type) and the registry diff logic compares old vs new objects.
Changes:
- Replaced cross-type
__eq__TypeErrorraises withreturn Falseacross the affected core object model classes (including data sources, feature views/services, projects, transformations, labeling, permissions). - Updated and added unit tests to assert cross-type comparisons return
False(and that!=remains the inverse), including updating a prior test that expectedTypeError. - Added a regression test covering the original repro scenario for data source type changes.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/python/tests/unit/test_label_view.py | Updates LabelView equality test to assert cross-type comparisons return False. |
| sdk/python/tests/unit/test_eq_cross_type.py | Adds a parameterized regression suite asserting cross-type equality returns False for multiple core types. |
| sdk/python/tests/unit/test_entity.py | Adds regression test for Entity cross-type equality returning False. |
| sdk/python/tests/unit/test_data_sources.py | Adds regression test covering cross-type equality for DataSource subclasses (e.g., FileSource vs SnowflakeSource). |
| sdk/python/tests/unit/permissions/test_policy.py | Adds regression test for policy cross-type equality returning False. |
| sdk/python/feast/transformation/substrait_transformation.py | Makes SubstraitTransformation __eq__ return False on cross-type comparisons. |
| sdk/python/feast/transformation/ray_transformation.py | Makes RayTransformation __eq__ return False on cross-type comparisons. |
| sdk/python/feast/transformation/python_transformation.py | Makes PythonTransformation __eq__ return False on cross-type comparisons. |
| sdk/python/feast/transformation/pandas_transformation.py | Makes PandasTransformation __eq__ return False on cross-type comparisons. |
| sdk/python/feast/stream_feature_view.py | Makes StreamFeatureView __eq__ return False on cross-type comparisons. |
| sdk/python/feast/saved_dataset.py | Makes SavedDataset __eq__ return False on cross-type comparisons. |
| sdk/python/feast/project.py | Makes Project __eq__ return False on cross-type comparisons. |
| sdk/python/feast/project_metadata.py | Makes ProjectMetadata __eq__ return False on cross-type comparisons. |
| sdk/python/feast/permissions/policy.py | Makes Policy implementations’ __eq__ return False on cross-type comparisons. |
| sdk/python/feast/permissions/permission.py | Makes Permission __eq__ return False on cross-type comparisons. |
| sdk/python/feast/on_demand_feature_view.py | Makes OnDemandFeatureView __eq__ return False on cross-type comparisons. |
| sdk/python/feast/labeling/label_view.py | Makes LabelView __eq__ return False on cross-type comparisons. |
| sdk/python/feast/infra/offline_stores/snowflake_source.py | Makes SnowflakeSource __eq__ return False on cross-type comparisons. |
| sdk/python/feast/infra/offline_stores/redshift_source.py | Makes RedshiftSource __eq__ return False on cross-type comparisons. |
| sdk/python/feast/infra/offline_stores/file_source.py | Makes FileSource __eq__ return False on cross-type comparisons. |
| sdk/python/feast/infra/offline_stores/contrib/trino_offline_store/trino_source.py | Makes TrinoSource __eq__ return False on cross-type comparisons. |
| sdk/python/feast/infra/offline_stores/contrib/ray_offline_store/ray_source.py | Makes RaySource __eq__ return False on cross-type comparisons. |
| sdk/python/feast/infra/offline_stores/contrib/postgres_offline_store/postgres_source.py | Makes PostgreSQLSource __eq__ return False on cross-type comparisons. |
| sdk/python/feast/infra/offline_stores/contrib/oracle_offline_store/oracle_source.py | Makes OracleSource __eq__ return False on cross-type comparisons. |
| sdk/python/feast/infra/offline_stores/contrib/mssql_offline_store/mssqlserver_source.py | Makes MsSqlServerSource __eq__ return False on cross-type comparisons. |
| sdk/python/feast/infra/offline_stores/contrib/couchbase_offline_store/couchbase_source.py | Makes CouchbaseColumnarSource __eq__ return False on cross-type comparisons. |
| sdk/python/feast/infra/offline_stores/contrib/athena_offline_store/athena_source.py | Makes AthenaSource __eq__ return False on cross-type comparisons. |
| sdk/python/feast/infra/offline_stores/bigquery_source.py | Makes BigQuerySource __eq__ return False on cross-type comparisons. |
| sdk/python/feast/feature_view.py | Makes FeatureView __eq__ return False on cross-type comparisons. |
| sdk/python/feast/feature_service.py | Makes FeatureService __eq__ return False on cross-type comparisons. |
| sdk/python/feast/entity.py | Makes Entity __eq__ return False on cross-type comparisons. |
| sdk/python/feast/data_source.py | Makes DataSource and streaming/request source __eq__ return False on cross-type comparisons. |
| sdk/python/feast/base_feature_view.py | Makes BaseFeatureView __eq__ return False on cross-type comparisons. |
| sdk/python/feast/aggregation/init.py | Makes Aggregation __eq__ return False on cross-type comparisons. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@ntkathole could you take a look at this one or reassign? The auto-assigned reviewer doesn't seem to be active lately, so it's been sitting since the 23rd. Checks are green and there are no open review threads. |
|
@ntkathole @franciscojavierarceo @jyejare @aniketpalu sorry for the broad ping, but this one still has no active reviewer. The auto-assigned reviewer hasn't reviewed anything here since October, and a second user just confirmed hitting the same bug over on #6636 (swapping a FeatureView source from FileSource to SparkSource). I've since extended the fix to cover their case as well. Checks are green, could one of you pick it up or assign someone? |
7e226c4 to
1af5c88
Compare
franciscojavierarceo
left a comment
There was a problem hiding this comment.
The cross-type guards are applied consistently across the affected domain objects, preserve same-type comparison behavior, and include broad regression coverage. I found no blocking issue in the remote diff.
Every registry object's __eq__ did
`if not isinstance(other, X): raise TypeError("Comparisons should only involve X class objects.")`.
Raising from __eq__ on an operand of a different type is wrong: comparing
objects of different types should be False, not an error. As a result, changing
a feature view's data source type and re-applying over an existing registry
crashed with `TypeError: Comparisons should only involve <X> class objects.`
Replace the raise with `return False` across all 35 affected __eq__ methods,
matching PushSource.__eq__ which already handles a cross-type comparison this
way. Update the LabelView equality test (it asserted the removed TypeError) and
add cross-type regression tests for DataSource, Entity, and policy objects, all
asserting False rather than a raise.
Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
…odel Assert that cross-type comparison returns False (never raises) for the importable core types touched by the sweep: Entity, Project, Aggregation, Permission, all four policies, the Kafka/Kinesis/Request and File/BigQuery/Redshift/Snowflake sources, FeatureView, and FeatureService. Complements the existing per-type tests (DataSource, Entity, LabelView, RoleBasedPolicy) and raises patch coverage on the sweep. Contrib sources and optional-dependency transformations are omitted since the unit env can't import them. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
…-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 <166439969+larrysingleton007@users.noreply.github.com>
1af5c88 to
d2fc30b
Compare
# [0.66.0](v0.65.0...v0.66.0) (2026-08-21) ### Bug Fixes * Add connection pre-warming for DynamoDB async client ([89240fa](89240fa)), closes [#6060](#6060) * Add remote registry client extra ([#6697](#6697)) ([b8dfcb0](b8dfcb0)) * Address review feedback on FIPS cipher suite configuration ([4a35fba](4a35fba)) * Allow remote-registry first apply for new projects ([39d408d](39d408d)) * Avoid importing feast.feature_store at mcp_server import time ([ddb2e9a](ddb2e9a)) * Bump pymssql to >=2.3.6 for macOS arm64 wheel support ([181eb35](181eb35)), closes [#5636](#5636) [#5193](#5193) [#5636](#5636) * Call ApplySavedDataset RPC instead of ApplyFeatureService in RemoteRegistry.apply_saved_dataset() ([934d341](934d341)) * Catch missing dbt parser dependency in dbt CLI commands ([#6534](#6534)) ([3c2ae3c](3c2ae3c)) * Default authentication to kubernetes auth ([6a4690a](6a4690a)) * Defer feature-freshness thread to post-fork to avoid Gunicorn deadlock ([#6648](#6648)) ([104ad10](104ad10)), closes [#6647](#6647) * Do not pass undeclared feature view columns to ODFV UDFs ([#6527](#6527)) ([75b9463](75b9463)) * downgrade mcp pin to 1.29.0 and fix CI lockfiles and unit tests ([98e5bca](98e5bca)), closes [#6706](#6706) * Feast apply silently ignoring ttl updates to None or timedelta(0) ([#6709](#6709)) ([97b0f25](97b0f25)), closes [#6703](#6703) * Fix mypy TorchTensor type alias error ([#6712](#6712)) ([34de6fa](34de6fa)), closes [#5563](#5563) * Fixed data source creation form gaps ([5d0f7d6](5d0f7d6)) * Handle parameterized and complex Trino types in type map ([326554d](326554d)) * Isolate default user permissions ([e37adbf](e37adbf)) * Isolate projection join key maps ([d1c709d](d1c709d)) * Map Postgres real to FLOAT instead of DOUBLE ([62db435](62db435)) * Merge shared ODFV source projections in feature resolution ([d269946](d269946)), closes [#6621](#6621) * More exhaustive athena types ([a9aaefc](a9aaefc)) * Normalize SQL registry read_path to the psycopg3 driver like path ([#6644](#6644)) ([996c6ea](996c6ea)), closes [#6643](#6643) * **operator:** add spec.services.onlineStore.disabled to opt out of the online store ([d81d4e3](d81d4e3)), closes [#6586](#6586) * Preinstall DuckDB delta extension for tests ([fd4d49d](fd4d49d)), closes [#6743](#6743) * Preserve event-time ordering within Redis online_write_batch ([40fb788](40fb788)), closes [#5163](#5163) * Prevent mutation of cached feature resolution results ([ea17419](ea17419)) * Remote feastRef FeatureStore fails first apply for a new feastProject ([9affee5](9affee5)) * Remove inert subjectaccessreviews and reorganize RBAC rules ([f771ea4](f771ea4)) * Report single-feature-view spark_application materialization success ([a9219d9](a9219d9)), closes [#6673](#6673) * Reset the global security manager after the permissions fixture ([7667215](7667215)) * Resolve kserve with pip --dry-run instead of installing it ([01da132](01da132)), closes [#6732](#6732) * Resolve write_to_offline_store feature view with a single registry lookup ([a42dc85](a42dc85)), closes [#4235](#4235) * Return False from __eq__ on cross-type comparison ([#6637](#6637)) ([0f149a9](0f149a9)), closes [#6636](#6636) * Reuse IdP-issued client tokens until near expiry ([602d752](602d752)) * Reuse the OIDC JWKS client across requests ([#6683](#6683)) ([a1e6fc2](a1e6fc2)) * Separate CronJob and feature-server ServiceAccounts ([398f643](398f643)) * Serialize UnixTimestamp proto values as raw int64 in remote online store transport ([1e7134f](1e7134f)) * Set FIPS cipher suites before pyarrow.flight import to prevent crash on IBM Power ([979b82a](979b82a)) * Support Entra ID (Azure AD) token claims in OIDC auth ([#6631](#6631)) ([f843c63](f843c63)) * UDF/ODFV source rehydrate (+ Postgres / online cache) ([#6655](#6655)) ([5fd7af7](5fd7af7)) * Updated projects-list.json in order to display newly added projects ([#6657](#6657)) ([3a6a103](3a6a103)) * Use correct image name in multi-arch imagetools push step ([faf85e0](faf85e0)) * Use join keys instead of entity names in ODFV materialization ([#6645](#6645)) ([abffebc](abffebc)), closes [#5965](#5965) * use matching proto class per feature view list in SqliteOnlineStore.plan() ([adb8c1c](adb8c1c)), closes [#6658](#6658) * Widen Athena integer type mapping for unsigned ints ([3425783](3425783)) ### Features * Add ConnectionRef to DataSource for pluggable external credential resolution ([28bde01](28bde01)) * Add Feature Service Create in UI ([0399380](0399380)) * Add hybrid to ValidOfflineStoreDBStorePersistenceTypes for HybridOfflineStore support ([#6707](#6707)) ([310ab51](310ab51)), closes [#6701](#6701) * Add MLflow integration support to Feast operator ([#6611](#6611)) ([52999f1](52999f1)) * Add opt-in filter_by_created_timestamp cutoff to get_historical_features ([#6617](#6617)) ([79b33ce](79b33ce)), closes [#6615](#6615) * Add optional OIDC token audience and issuer verification ([#6670](#6670)) ([ef307c6](ef307c6)) * Add packaged feature repository support to Feast Operator ([8112b1e](8112b1e)), closes [#6598](#6598) * add plan() support to DynamoDBOnlineStore ([51ce982](51ce982)), closes [#6658](#6658) [#6659](#6659) * Added optional namespace/colleciton to datasets ([165fcf2](165fcf2)) * Added SQL registry schema_mode and registry create command ([#6704](#6704)) ([037c4cd](037c4cd)) * Allow users to have protected project on shared registry ([f9923bc](f9923bc)) * Apply Intermediate TLS defaults on API fallback and handle transient errors ([#6587](#6587)) ([43ae993](43ae993)) * **cli:** Updated feast init demo by adding rag template ([#5946](#5946)) ([c8628eb](c8628eb)), closes [#5264](#5264) * Expose the OIDC JWKS tunables through the operator ([#6690](#6690)) ([fef4e78](fef4e78)), closes [#6683](#6683) * Making feast vector store with open ai search api compatible ([#6121](#6121)) ([54da19a](54da19a)) * Multi-arch publish for feast operator image ([b221036](b221036)) * OpenLineage lineage enhancements - full object coverage, richer UI, and API-level sync ([#6719](#6719)) ([120a868](120a868)) * **operator:** Add spec.services.initImage for init container image override ([#6598](#6598)) ([ca355cb](ca355cb)) * Pass optional OIDC audience and issuer through the operator ([#6677](#6677)) ([a13ed7b](a13ed7b)), closes [#6670](#6670) * **server:** Remote Materialization ([#6649](#6649)) ([b7ae488](b7ae488)), closes [#4526](#4526) * Support Lineage configs via operator ([bf1e54a](bf1e54a)) * Updated datasets UI to support grouping ([7ae64ec](7ae64ec))
What this PR does / why we need it:
Every registry object's
__eq__raisedTypeErrorwhen compared against an object of a different type:Comparing objects of different types should be
False, not an error. Because the apply/diff path compares registered objects against incoming ones, this meant changing a feature view's data source type and re-runningfeast applyover an existing registry crashed withTypeError: Comparisons should only involve <X> class objects.This replaces the raise with
return Falseacross all 35 affected__eq__methods (DataSourceand itsKafka/Request/Kinesissubclasses, the offline and contrib sources,Entity,FeatureView,BaseFeatureView,OnDemandFeatureView,StreamFeatureView,FeatureService,SavedDataset,Project,ProjectMetadata, the transformations,Aggregation,LabelView,Permission, and the policies).PushSource.__eq__already handled a cross-type comparison this way, so this makes every other__eq__consistent with it rather than introducing a new pattern (there is noreturn NotImplementedanywhere in the codebase).Which issue(s) this PR fixes:
Fixes #6636
Checks
git commit -s)Testing Strategy
Added cross-type regression tests asserting
False(not a raise) forDataSource,Entity, and a policy object, and updatedtest_label_view.py::test_equality_type_check, which previously asserted the removedTypeError. Full unit suite passes locally.Misc
Release note: Comparing two Feast objects of different types now returns
Falseinstead of raisingTypeError, so changing a feature view's data source type and re-applying no longer crashes.Scope: this sweeps all 35 sites across the object model. Happy to narrow to just the
DataSourcehierarchy (where the crash reproduces), with a follow-up for the rest, if a smaller diff is preferred.As an outside contributor I can't apply labels, so a maintainer will need to add a
kind/buglabel and runok-to-test.