Implement entity aliasing for feature retrieval by codyjlin · Pull Request #1868 · feast-dev/feast

codyjlin · 2021-09-15T21:18:01Z

Signed-off-by: Cody Lin codyl@twitter.com

What this PR does / why we need it:

This PR implements the Shadow Entities Mapping RFC - currently assuming we go with Option 8b (and the implementation idea at the bottom of the RFC). It will allow a user to override a FeatureView's join_key (like "user_id") to be able to join with an entity_df source that might have other column names that are spelled differently or might be "subclasses" of the entity (like "spammer_id" and "reporter_id"). This functionality is to be made available on the offline side via get_historical_features and the online side via get_online_features.

TODO:

testing to come
will add changes to redshift.py once changes to bigquery.py are reasonable/approved, and/or testing is ready

Which issue(s) this PR fixes:

Part of #1762
(Might want to add documentation after change is landed)

Does this PR introduce a user-facing change?:

User can now override the join keys of a FeatureView if the entity column name(s) are misspelled or
don't match the entity column name of the entity_df.
This can be done:
- by setting the FeatureView.join_key_map attribute
- at use-time FV in the FeatureService.features attribute via FeatureView.with_join_key_map(join_key_map)

woop · 2021-09-16T00:18:04Z

Is use_time a term people use? It wasn't immediately clear to me what this meant. I was also imagining that the logic for manipulating the feature view would be on the feature view itself, not a separate method.

Finally, you should add type annotations please.

Definitely open to alternatives of use_time - I agree that it's not clear, but I'm not sure how else to describe it or what to rename it to. Basically instead of grabbing all of the registered FeatureViews, we need to grab any changes that the user wants to specify at the time of using the FeatureView (like the join_key_map or a new name).

By "the logic for manipulating the feature view would be on the feature view itself", do you mean .override_join_keys or .with_join_key_map should manipulate the original object? I feel like we wouldn't want that behavior with this design given one might use it multiple times in different contexts.

Will do in next commit 👌

Definitely open to alternatives of use_time - I agree that it's not clear, but I'm not sure how else to describe it or what to rename it to. Basically instead of grabbing all of the registered FeatureViews, we need to grab any changes that the user wants to specify at the time of using the FeatureView (like the join_key_map or a new name).

I'd first want to see if we can avoid naming it at all. My instinct would have been to use a class like FeatureViewProjection which already supports filtering columns, and adding more options like renaming the feature view or overriding the join key. We may change it over time to be FeatureViewQuery or something. That would also prevent somebody from doing fv.with_name("blah") and then accidentally registering that FV, since we only anticipate the use of renaming to be for queries.

By "the logic for manipulating the feature view would be on the feature view itself", do you mean .override_join_keys or .with_join_key_map should manipulate the original object? I feel like we wouldn't want that behavior with this design given one might use it multiple times in different contexts.

Not override the original object, but instead produce a new object. It's just that the logic itself seems like it is better to have on the feature view class itself, basically what @mavysavydav did with with_name(). Although in his case he is returning a FV, but I think we may want to return another object that is specifically used for queries.

woop · 2021-09-16T00:18:37Z

Please remember to add documentation to this PR as well.

codyjlin · 2021-09-16T19:44:03Z

shoot, just realized these are all FeatureViewProjection objects cause it converts the features after initializing a FeatureService...

This is dependent on a PR by @mavysavydav to have FeatureService.features allow FeatureView as one of the types.

codecov-commenter · 2021-09-22T18:45:54Z

Codecov Report

Merging #1868 (44b4e49) into master (314447f) will increase coverage by 0.24%.
The diff coverage is 99.19%.

@@            Coverage Diff             @@
##           master    #1868      +/-   ##
==========================================
+ Coverage   82.02%   82.26%   +0.24%     
==========================================
  Files          96       96              
  Lines        7576     7669      +93     
==========================================
+ Hits         6214     6309      +95     
+ Misses       1362     1360       -2

Flag	Coverage Δ
integrationtests	`74.15% <99.19%> (+0.43%)`	⬆️
unittests	`59.39% <25.80%> (-0.48%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
sdk/python/feast/infra/offline_stores/redshift.py	`85.00% <ø> (ø)`
...marks/test_benchmark_universal_online_retrieval.py	`32.00% <50.00%> (ø)`
sdk/python/feast/driver_test_data.py	`100.00% <100.00%> (ø)`
sdk/python/feast/feature_store.py	`94.54% <100.00%> (+0.08%)`	⬆️
sdk/python/feast/feature_view.py	`87.50% <100.00%> (+2.03%)`	⬆️
sdk/python/feast/feature_view_projection.py	`100.00% <100.00%> (ø)`
sdk/python/feast/infra/offline_stores/file.py	`97.50% <100.00%> (+0.06%)`	⬆️
...python/feast/infra/offline_stores/offline_utils.py	`90.21% <100.00%> (+0.10%)`	⬆️
...ts/integration/feature_repos/repo_configuration.py	`100.00% <100.00%> (ø)`
...ts/integration/feature_repos/universal/entities.py	`100.00% <100.00%> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 314447f...44b4e49. Read the comment docs.