Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Validate entities when running get_online_features #5031

Merged
merged 1 commit into from
Feb 8, 2025

Conversation

franciscojavierarceo
Copy link
Member

@franciscojavierarceo franciscojavierarceo commented Feb 7, 2025

What this PR does / why we need it:

This PR addresses the issue of unexpected crashes when using feature_store.get_online_features with incorrect or missing join keys.

Specifically, it introduces validation logic in the _get_unique_entities function to ensure that all expected join keys are present and non-empty before proceeding.

If any required join keys are missing or have empty values, a KeyError is raised with a descriptive error message. Additionally, new unit tests have been added to cover various edge cases, ensuring that the function behaves as expected under different scenarios.

Changes:

  • sdk/python/feast/utils.py:
    • Added validation for missing and empty join keys in _get_unique_entities.
    • Updated the logic to convert column-oriented table_entity_values into row-wise data and handle edge cases.
  • sdk/python/tests/unit/online_store/test_online_retrieval.py:
    • Added a test case to verify that a KeyError is raised when required join keys are missing.
  • sdk/python/tests/unit/test_unit_feature_store.py:
    • Imported pytest for testing.
    • Renamed test_get_unique_entities to test_get_unique_entities_success for clarity.
    • Added test cases for missing join keys and handling errors appropriately.

Which issue(s) this PR fixes:

#3270

Misc

It's worth noting, when one key is present and one is not, the retrieval will still succeed as this is expected behavior.

Copy link
Collaborator

@HaoXuAI HaoXuAI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@franciscojavierarceo franciscojavierarceo merged commit 3bb0dca into master Feb 8, 2025
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants