GeezerVison

Good day @franciscojavierarceo
I would appreciate it if my PR is looked at and reviewed if there is any comment you'd want to give in relations to the PR would be highly appreciated.

Good day @franciscojavierarceo
I would appreciate it if my PR is looked at and reviewed.

Hello,
Good day @franciscojavierarceo
Just a quick reminder of my PR getting reviewed.

Can you explain why you need this argument?

Can you explain why you need this argument?

The new on_demand_feature_views argument is needed because FeatureStore.get_historical_features() already resolves which OnDemandFeatureViews are actually requested (via _group_feature_refs), but historically we were not passing that information to the Provider.
Without this argument, the Provider only sees feature_views + feature_refs and has to:

• either re-resolve ODFVs from the feature refs (i.e. duplicating logic), or
• conservatively treat ODFVs as part of the full graph and potentially execute transformations for ODFVs that aren’t requested, which can add unnecessary joins/transforms and overhead.

Passing on_demand_feature_views makes the API explicit so provider implementations that can optimize or need this for correct planning like compute-engine based retrieval can do so without re-deriving it.

Can you explain why you need this argument?

The new on_demand_feature_views argument is needed because FeatureStore.get_historical_features() already resolves which OnDemandFeatureViews are actually requested (via _group_feature_refs), but historically we were not passing that information to the Provider. Without this argument, the Provider only sees feature_views + feature_refs and has to:

• either re-resolve ODFVs from the feature refs (i.e. duplicating logic), or
• conservatively treat ODFVs as part of the full graph and potentially execute transformations for ODFVs that aren’t requested, which can add unnecessary joins/transforms and overhead.

Passing on_demand_feature_views makes the API explicit so provider implementations that can optimize or need this for correct planning like compute-engine based retrieval can do so without re-deriving it.

Although I don’t fully support this ad hoc change, since it’s not user-facing, I’m okay with approving it. Could you please add a TODO and create a follow-up issue to flag it for a proper long-term fix?

Although I don’t fully support this ad hoc change, since it’s not user-facing, I’m okay with approving it. Could you please add a TODO and create a follow-up issue to flag it for a proper long-term fix?

Okay, I'm on it now

Although I don’t fully support this ad hoc change, since it’s not user-facing, I’m okay with approving it. Could you please add a TODO and create a follow-up issue to flag it for a proper long-term fix?

Added a TODO in FeatureStore.get_historical_features to flag this as an interim interface change. I’ve also open a follow-up issue to track a proper long-term cleanup of the historical retrieval Provider API (request object / clearer contract) and will link it here once created.

Hello,
Good day @franciscojavierarceo
Just a quick reminder of my PR getting reviewed or processed

Hello,
Good day
Please can my PR get reviewed?