Skip to content

fix: Keep unmapped join keys in ibis point-in-time joins - #7159

Open
iammanoj807 wants to merge 1 commit into
feast-dev:masterfrom
iammanoj807:fix/ibis-partial-join-key-map
Open

iammanoj807 wants to merge 1 commit into
feast-dev:masterfrom
iammanoj807:fix/ibis-partial-join-key-map

Conversation

@iammanoj807

Copy link
Copy Markdown

What this PR does / why we need it:

get_historical_features_ibis, which the DuckDB offline store and the contrib Oracle and MSSQL stores use, took a projection's join_key_map as the whole list of join keys whenever the map was not empty. When with_join_key_map renamed only some keys of a feature view with several entities, the other keys were left out of the point-in-time join predicates and of entity_row_id. Each entity row then got the latest value for the renamed key alone, taken from any value of the other keys.

Feature view keyed on user_id and merchant_id, used as .with_join_key_map({"user_id": "sender_id"}). User 1 spent 10 at merchant 100 and 99 at merchant 200:

Before:
 sender_id  merchant_id  spend
         1          100     99
         1          200     99
After:
 sender_id  merchant_id  spend
         1          100     10
         1          200     99

get_online_features on the same store already returns 10 and 99.

The map is now built from the feature view's entity columns, and every key the projection does not rename keeps its own name (join_key_map.get(name, name)), as in the Dask store and offline_utils.get_feature_view_query_context. The new _join_key_map() is used for both the join predicates and entity_row_id.

Adds test_get_historical_features_keeps_join_keys_left_out_of_join_key_map to test_ibis.py. It fails on master ({100: 99, 200: 99}) and passes with this change.

Which issue(s) this PR fixes:

Fixes #7158

Checks

  • I've made sure the tests are passing.
  • My commits are signed off (git commit -s)
  • My PR title follows conventional commits format

Testing Strategy

  • Unit tests
  • Integration tests
  • Manual tests
  • Testing is not required for this change

Misc

Run locally:

  • test_ibis.py: 11 passed.
  • The other related unit tests give the same results as on master, plus the new test (103 passed): the Dask, created-timestamp, SQL entity-row-id and hybrid offline store tests, plus test_feature_service.py, test_utils_entity_maps.py and test_unit_feature_store.py. test_offline_store.py was not collected locally because psycopg is not installed.
  • ruff 0.16.10 and mypy 1.11.2 pass on the changed files.
  • Manual check: the reproducer from the issue.

get_historical_features_ibis, used by the DuckDB offline store and the
contrib Oracle and MSSQL stores, took a projection's join_key_map as the
full list of join keys. When with_join_key_map renamed only some keys of
a feature view with several entities, the other keys were left out of
the join and of entity_row_id. Each entity row then got the latest value
for the renamed key alone, taken from any value of the other keys.

Build the map from the feature view's entity columns instead and keep
the name of every key the projection does not rename, as the Dask and
SQL offline stores and the online read path already do.

Signed-off-by: Manoj Kumar Thapa <[email protected]>
@iammanoj807
iammanoj807 requested a review from a team as a code owner November 13, 2026 22:51

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

get_historical_features on DuckDB ignores join keys that with_join_key_map does not rename

1 participant