feat: Adding 'SortedFeatureView' type to support Range Queries #168

Manisha4 · 2025-02-11T02:02:45Z

What this PR does / why we need it:

Updating the feast protos with a new FeatureView type 'SortedFeatureView' used specifically for range series support.

kpulipati29 · 2025-02-11T14:02:42Z

protos/feast/core/SortedFeatureView.proto

+    // System-populated metadata for this feature view.
+    FeatureViewMeta meta = 2;
+}
+


This empty line can be removed.

kpulipati29 · 2025-02-11T14:03:07Z

protos/feast/core/SortedFeatureView.proto

+import "feast/core/DataSource.proto";
+import "feast/core/Entity.proto";
+import "feast/core/Feature.proto";
+import "feast/core/FeatureView.proto";


~~Unused import FeatureView can be removed?~~

kpulipati29 · 2025-02-11T14:04:59Z

protos/feast/core/SortedFeatureView.proto

+    SortedFeatureViewSpec spec = 1;
+
+    // System-populated metadata for this feature view.
+    FeatureViewMeta meta = 2;


~~FeatureViewMeta definition is missing, this needs to be defined?~~

FeatureViewMeta is defined in FeatureView.proto which is why it is imported

kpulipati29 · 2025-02-11T14:07:10Z

protos/feast/core/SortedFeatureView.proto

+
+    // Batch/Offline DataSource where this view can retrieve offline feature data.
+    DataSource batch_source = 7;
+    // Streaming DataSource from where this view can consume "online" feature data.


Need an empty line here? (between batch and stream source definitions)

kpulipati29 · 2025-02-11T14:14:18Z

protos/feast/core/SortedFeatureView.proto

+    repeated FeatureSpecV2 entity_columns = 12;
+
+    // List of sort keys for this feature view.
+    repeated SortKey sort_keys = 31;


Hmm I was thinking we decided not to allow more than one field as a sort key, we need to double check this about whether we want to allow multiple fields as sort keys or not.

We don't need to buffer the index for this field, it is a critical field for this feature view type, so just use index 13

+1 on using index 13. This is completely new implementation.

kpulipati29 · 2025-02-11T14:21:07Z

protos/feast/core/SortedFeatureView.proto

+
+// Defines the sorting criteria for range-based feature queries.
+message SortKey {
+    // Name of the feature or entity used for sorting.


Name of the feature or entity used for sorting.

Entities in feature view maps to partition key in Scylla db and not used as a sort key right? You think we should remove entity here?

Yep, will remove

kpulipati29 · 2025-02-11T14:58:43Z

protos/feast/registry/RegistryServer.proto

@@ -40,6 +41,12 @@ service RegistryServer{
  rpc GetFeatureView (GetFeatureViewRequest) returns (feast.core.FeatureView) {}
  rpc ListFeatureViews (ListFeatureViewsRequest) returns (ListFeatureViewsResponse) {}

+  // SortedFeatureView RPCs


Need to check but are you sure that you need to have ApplySortedFeatureView and DeleteSortedFeatureView here? Because for example if you look at StreamFeatureView or OnDemandFeatureView RPCs below, they only have Get and List. I think it should be similar for SortedFeatureView too, isn't it?

The generic Apply and Delete rpcs should handle all feature view types, we don't need specific ones for SortedFeatureView.

kpulipati29 · 2025-02-11T14:59:37Z

protos/feast/registry/RegistryServer.proto

@@ -277,6 +286,36 @@ message ListOnDemandFeatureViewsResponse {
    repeated feast.core.OnDemandFeatureView on_demand_feature_views = 1;
 }

+// SortedFeatureView for range queries
+
+message ApplySortedFeatureViewRequest {


This can be removed if we know that Apply and Delete is not needed for SortedFeatureView.

Needed for remote registry. We should implement it for remote registry.

kpulipati29 · 2025-02-11T14:59:48Z

protos/feast/registry/RegistryServer.proto

+  repeated feast.core.SortedFeatureView sorted_feature_views = 1;
+}
+
+message DeleteSortedFeatureViewRequest {


Same here, this can be removed if we know that Apply and Delete is not needed for SortedFeatureView.

Needed for Remote registry

Remote registry already uses the generic apply and delete methods that internally handle the logic for different feature view types. I think it's best to keep this consistent and not make a special case here.

Yeah agree, the delete looks generic for all feature view types. For example, we just need to add logic for our SortedFeatureView type here in the existing DeleteFeatureView

We can remove ApplySortedFeatureView and DeleteSortedFeatureViews.

kpulipati29 · 2025-02-11T15:21:57Z

protos/feast/registry/RegistryServer.proto

@@ -421,4 +460,4 @@ message ListProjectsResponse {
 message DeleteProjectRequest {
  string name = 1;
  bool commit = 2;
-}
+}


Oh and this is not the right place to comment, but you should also add SortedFeatureView to Registry.proto somewhere over here

Good catch, I missed that, thanks!

piket · 2025-02-11T16:31:31Z

protos/feast/core/SortedFeatureView.proto

+}
+
+// A collection of multiple SortedFeatureViews.
+message SortedFeatureViewList {


This is redundant with ListSortedFeatureViewsResponse below and not needed.

piket · 2025-02-11T16:32:30Z

protos/feast/registry/RegistryServer.proto

@@ -217,6 +225,7 @@ message AnyFeatureView {
        feast.core.FeatureView feature_view = 1;
        feast.core.OnDemandFeatureView on_demand_feature_view = 2;
        feast.core.StreamFeatureView stream_feature_view = 3;
+        feast.core.SortedFeatureView sorted_feature_view = 4;


Let's leave a buffer on the index for sorted feature view in case Feast adds other feature views and we get a conflict.

piket

You need to add SortedFeatureView similar to the other feature view types to the Registry.proto (not RegistryServer) as well.

…, RegistryServer.proto and SortedFeatureView.proto

…try use case

protos/feast/core/SortedFeatureView.proto

Adding 'SortedFeatureView' type to support range queries

e6a8bee

Manisha4 changed the title ~~Adding 'SortedFeatureView' type to support Range Queries~~ featAdding 'SortedFeatureView' type to support Range Queries Feb 11, 2025

Manisha4 changed the title ~~featAdding 'SortedFeatureView' type to support Range Queries~~ feat: Adding 'SortedFeatureView' type to support Range Queries Feb 11, 2025

Adding type to ApplyFeatureViewRequest

8633534

kpulipati29 reviewed Feb 11, 2025

View reviewed changes

piket reviewed Feb 11, 2025

View reviewed changes

piket requested changes Feb 11, 2025

View reviewed changes

Manisha4 added 2 commits February 11, 2025 11:48

Addressing PR Comments: made modifications to SortedFeatureView.proto…

fab073c

…, RegistryServer.proto and SortedFeatureView.proto

Addressing PR Comments: adding delete and apply back for remote regis…

eb2aed1

…try use case

EXPEbdodla reviewed Feb 11, 2025

View reviewed changes

protos/feast/core/SortedFeatureView.proto Show resolved Hide resolved

Addressing PR Comments: removing delete and apply after some discussion

e80a9d1

piket approved these changes Feb 11, 2025

View reviewed changes

kpulipati29 approved these changes Feb 11, 2025

View reviewed changes

Manisha4 merged commit 38be3e4 into master Feb 11, 2025
23 checks passed

Manisha4 deleted the add/new-feature-view branch February 11, 2025 22:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Adding 'SortedFeatureView' type to support Range Queries #168

feat: Adding 'SortedFeatureView' type to support Range Queries #168

Manisha4 commented Feb 11, 2025 •

edited

Loading

kpulipati29 Feb 11, 2025

kpulipati29 Feb 11, 2025 •

edited

Loading

kpulipati29 Feb 11, 2025 •

edited

Loading

piket Feb 11, 2025

kpulipati29 Feb 11, 2025

kpulipati29 Feb 11, 2025

piket Feb 11, 2025

EXPEbdodla Feb 11, 2025

kpulipati29 Feb 11, 2025

Manisha4 Feb 11, 2025

kpulipati29 Feb 11, 2025 •

edited

Loading

piket Feb 11, 2025

kpulipati29 Feb 11, 2025

EXPEbdodla Feb 11, 2025

kpulipati29 Feb 11, 2025

EXPEbdodla Feb 11, 2025

piket Feb 11, 2025

kpulipati29 Feb 11, 2025

EXPEbdodla Feb 11, 2025

kpulipati29 Feb 11, 2025

Manisha4 Feb 11, 2025

piket Feb 11, 2025

piket Feb 11, 2025

EXPEbdodla Feb 11, 2025

piket left a comment

feat: Adding 'SortedFeatureView' type to support Range Queries #168

feat: Adding 'SortedFeatureView' type to support Range Queries #168

Conversation

Manisha4 commented Feb 11, 2025 • edited Loading

What this PR does / why we need it:

Choose a reason for hiding this comment

kpulipati29 Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

kpulipati29 Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpulipati29 Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piket left a comment

Choose a reason for hiding this comment

Manisha4 commented Feb 11, 2025 •

edited

Loading

kpulipati29 Feb 11, 2025 •

edited

Loading

kpulipati29 Feb 11, 2025 •

edited

Loading

kpulipati29 Feb 11, 2025 •

edited

Loading