-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: Check duplicate names for feature view across types #5999
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
Changes from 1 commit
6e256b6
e319a6f
be0e8a3
8363cd4
d7bac33
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
Signed-off-by: Prathap P <436prathap@gmail.com>
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,6 +58,7 @@ | |
| from feast.dqm.errors import ValidationFailed | ||
| from feast.entity import Entity | ||
| from feast.errors import ( | ||
| ConflictingFeatureViewNames, | ||
| DataFrameSerializationError, | ||
| DataSourceRepeatNamesException, | ||
| FeatureViewNotFoundException, | ||
|
|
@@ -3255,18 +3256,25 @@ def _print_materialization_log( | |
|
|
||
|
|
||
| def _validate_feature_views(feature_views: List[BaseFeatureView]): | ||
| """Verify feature views have case-insensitively unique names""" | ||
| fv_names = set() | ||
| """Verify feature views have case-insensitively unique names across all types. | ||
|
|
||
| This validates that no two feature views (of any type: FeatureView, | ||
| StreamFeatureView, OnDemandFeatureView) share the same case-insensitive name. | ||
| This is critical because get_online_features uses get_any_feature_view which | ||
| resolves names in a fixed order, potentially returning the wrong feature view. | ||
| """ | ||
| fv_by_name: Dict[str, BaseFeatureView] = {} | ||
| for fv in feature_views: | ||
| case_insensitive_fv_name = fv.name.lower() | ||
| if case_insensitive_fv_name in fv_names: | ||
| raise ValueError( | ||
| f"More than one feature view with name {case_insensitive_fv_name} found. " | ||
| f"Please ensure that all feature view names are case-insensitively unique. " | ||
| f"It may be necessary to ignore certain files in your feature repository by using a .feastignore file." | ||
| if case_insensitive_fv_name in fv_by_name: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ntkathole Thanks for your comment. In current Feast master, FeatureStore.apply() behaves as an incremental upsert (not a full registry refresh): it validates only the objects in the current request and writes those objects, while existing unrelated records remain untouched. Because of this, duplicate-name protection is incomplete for previously registered objects—especially when names differ only by case (for example, existing driver and new Driver). The in-request validation checks only the incoming payload, and the SQL registry uniqueness constraints are per table and case-sensitive by default, so this scenario can pass and create ambiguous registry state. Expected behavior (per my understanding) is case-insensitive uniqueness for feature view names, so this appears to be a gap.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Prathap-P I think it's fine if you want to handle it separately and we keep scope of this PR for sql registry duplicate names
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ntkathole Solving the test cases took a lot of time, finally was able to fix it. Do you have any other points to discuss?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know Devin is reporting these changes are not implemented in snowflake, but as you said to scope this pr to sql, ignoring it... |
||
| existing_fv = fv_by_name[case_insensitive_fv_name] | ||
| raise ConflictingFeatureViewNames( | ||
| fv.name, | ||
| existing_type=type(existing_fv).__name__, | ||
| new_type=type(fv).__name__, | ||
| ) | ||
| else: | ||
| fv_names.add(case_insensitive_fv_name) | ||
| fv_by_name[case_insensitive_fv_name] = fv | ||
|
|
||
|
|
||
| def _validate_data_sources(data_sources: List[DataSource]): | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.