Repository navigation
fix: Compare the source table in PostgreSQL, SQL Server and Couchbase source equality - #7080
NopeCharon wants to merge 2 commits into
Conversation
… source equality PostgreSQLSource.__eq__ compared the query but not the table, MsSqlServerSource.__eq__ compared the connection string but not the table_ref, and CouchbaseColumnarSource.__eq__ compared the query but not the database, scope or collection. A table-backed source that was moved to another table therefore compared equal to the registered one. The file registry's apply_data_source returns early when the incoming source equals the stored one, so re-running feast apply after changing only the table left the registered data source on the old table, while feature views built on it (whose protos embed their batch source) moved to the new table. The registry ended up describing the same source two different ways. Compare the location fields too. Each of them is already serialized into the source's custom options and restored by from_proto, so a source read back from the registry still equals itself and unchanged sources are not re-applied. Signed-off-by: NopeCharon <[email protected]>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7080 +/- ##
==========================================
+ Coverage 50.97% 51.08% +0.11%
==========================================
Files 455 455
Lines 56994 57000 +6
Branches 8321 8322 +1
==========================================
+ Hits 29055 29121 +66
+ Misses 25954 25887 -67
- Partials 1985 1992 +7
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
patelchaitany
left a comment
There was a problem hiding this comment.
Thanks for this, the root cause and repro look right to me! Two inline comments below, plus one note on the description:
- The impact is bigger than "described two ways". With the remote offline store, the client sends only
data_source_name(remote.py#L296), and the server looks it up withget_data_source()(offline_server.py#L551). So remote materialization quietly keeps reading the old table. That might be worth adding to the description.
| }, | ||
| ), | ||
| ], | ||
| ids=["postgres", "mssql", "couchbase"], |
There was a problem hiding this comment.
ClickhouseSource has the same bug: it has no __eq__, so it falls back to the base one, which never compares table or query (clickhouse_source.py#L59). But get_table_query_string() builds the SQL from them (#L153).
Could you add the same fix there (compare _table and _query and call super().__eq__()), plus a clickhouse case here? ClickhouseOptions already round-trips both fields, so it should be a small change.
There was a problem hiding this comment.
Done in dbf8643. ClickhouseSource now compares _table and _query on top of super().__eq__(), and there's a clickhouse case in this test.
| @@ -177,6 +177,8 @@ def __eq__(self, other): | |||
|
|
|||
| return ( | |||
| self.name == other.name | |||
There was a problem hiding this comment.
Since you're already in this __eq__: it never calls super().__eq__(), so changes to tags, description, owner and connection_ref are also ignored on re-apply. tags matter because permissions match on required_tags (permission.py#L62), and the offline server checks them against the registry copy.
Maybe switch this to super().__eq__(other) and ... like Postgres and Couchbase? OracleSource has the same pattern (oracle_source.py#L109). A follow-up issue is fine too if you'd rather keep this PR small.
There was a problem hiding this comment.
Switched it to super().__eq__(other) and ... in dbf8643. The proto didn't carry description, tags or owner for this source, so I added those too so a registry round trip still compares equal. I left OracleSource for a follow-up to keep this PR focused.
ClickhouseSource had no __eq__ of its own, so the base comparison ignored its table and query and a moved source compared equal to the registered one. Compare both options on top of the base fields. MsSqlServerSource.__eq__ did not call the base comparison, so changes to tags, description, owner and connection_ref were ignored on re-apply. Use super().__eq__() and serialize description, tags and owner in the proto so a source read back from the registry still equals itself. Signed-off-by: NopeCharon <[email protected]>
patelchaitany
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround and for catching the missing proto fields in MSSQL. LGTM!
What this PR does / why we need it:
Four table-backed sources leave their location out of
__eq__:PostgreSQLSourcecomparesquerybut nottable.MsSqlServerSourcecomparesconnection_strbut nottable_ref. It also skippedsuper().__eq__(), sotags,description,ownerandconnection_refchanges were ignored as well.CouchbaseColumnarSourcecomparesquerybut notdatabase,scopeorcollection.ClickhouseSourcehas no__eq__of its own, so neithertablenorqueryis compared.So a source pointed at another table compares equal to the one already in the registry.
Registry.apply_data_sourcereturns early when the stored source is equal to the incoming one. As a result, re-runningfeast applyafter changing only the table leaves the registered data source on the old table. Feature views built on that source embed it in their own proto, so they do move to the new table, and the registry ends up describing the same source two ways. With the remote offline store this is worse: the client sends onlydata_source_nameand the offline server resolves it withget_data_source(), so remote materialization keeps reading the old table.Reproduced with a file registry and a
PostgreSQLSourcefeature view: apply withtable="driver_stats", then apply again withtable="driver_stats_v2".get_data_source("driver_stats")get_feature_view("driver_fv").batch_sourcedriver_statsdriver_stats_v2driver_stats_v2driver_stats_v2The fix compares these location fields in
__eq__. Each one is already written to the source's custom options and restored byfrom_proto(for SQL Server,description,tagsandownerare now serialized too), so a source read back from the registry still equals itself, and unchanged sources are not re-applied on everyfeast apply.Which issue(s) this PR fixes:
No existing issue. Found while reading the data source equality checks.
Checks
git commit -s)Testing Strategy
test_data_source_eq_compares_the_source_tableintests/unit/test_data_sources.pycovers all four sources, andtest_mssqlserver_source_eq_compares_base_fieldscovers the SQL Server base fields. For each one it checks that a moved source compares unequal, that a proto round trip still compares equal, and that a file registry stores the moved table on re-apply. The test fails on master for all three and passes with this change.tests/unit/test_data_sources.py,tests/unit/infra/registry/test_file_registry.pyand the Postgres offline store unit tests pass, and ruff check and ruff format are clean. The manual test is theFeatureStore.applyreproduction above.Misc
SparkSource.__eq__also skipsfile_formatandtable_format. It isn't changed here because its options don't round-trip unset values cleanly (Nonecomes back as""), so comparing them needs a separate change.