-
Notifications
You must be signed in to change notification settings - Fork 1k
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
chore: Go Sql Online Store #2446
Changes from all commits
501f59d
0fbd8dc
21c8900
01d9a1d
2c4fccd
34dcd15
80f4579
17acc6a
64fa88b
0601f0a
2233ff8
4de021b
e3f9970
d9f5333
a631c52
993ead4
63fbee6
c325834
f2202b8
c242587
08e51fb
b551639
0755e05
eb8a320
c3472bd
a5b6607
5f68689
3896f99
111c2d0
c6197ce
a5d4d28
14d5602
02eda7a
14876a2
3bfcfad
8739e68
bd052a3
189d3d2
acc86bb
34ab63d
fa0abf3
941aea8
025a7e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ package feast | |
import ( | ||
"context" | ||
"fmt" | ||
|
||
"github.com/feast-dev/feast/go/protos/feast/serving" | ||
"github.com/feast-dev/feast/go/protos/feast/types" | ||
"github.com/golang/protobuf/ptypes/timestamp" | ||
|
@@ -33,7 +34,7 @@ type OnlineStore interface { | |
// Feature object as pointers in GetOnlineFeaturesResponse) | ||
// => allocate memory for each field once in OnlineRead | ||
// and reuse them in GetOnlineFeaturesResponse? | ||
OnlineRead(ctx context.Context, entityKeys []types.EntityKey, featureViewNames []string, featureNames []string) ([][]FeatureData, error) | ||
OnlineRead(ctx context.Context, entityKeys []*types.EntityKey, featureViewNames []string, featureNames []string) ([][]FeatureData, error) | ||
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. Why are we changing the interface here? 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 think it doesn't make sense to have to copy the EntityKey around, changing it to a pointer doesn't change the functionality, just makes it more efficient |
||
// Destruct must be call once user is done using OnlineStore | ||
// This is to comply with the Connector since we have to close the plugin | ||
Destruct() | ||
|
@@ -51,12 +52,13 @@ func getOnlineStoreType(onlineStoreConfig map[string]interface{}) (string, bool) | |
func NewOnlineStore(config *RepoConfig) (OnlineStore, error) { | ||
onlineStoreType, ok := getOnlineStoreType(config.OnlineStore) | ||
if !ok { | ||
return nil, fmt.Errorf("could not get online store type from online store config: %+v", config.OnlineStore) | ||
onlineStore, err := NewSqliteOnlineStore(config.Project, config, config.OnlineStore) | ||
return onlineStore, err | ||
Comment on lines
54
to
+56
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. This seems wrong ? DOn't we need 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. In the python implementation, no type for online store config automatically assumes sqlite as the onlinestore => I believe I tried adding type = sqlite there were some small config collisions so I decided it makes sense to adhere to the python online store configuraiton. |
||
} | ||
if onlineStoreType == "redis" { | ||
onlineStore, err := NewRedisOnlineStore(config.Project, config.OnlineStore) | ||
return onlineStore, err | ||
} else { | ||
return nil, fmt.Errorf("%s online store type is currently not supported; only Redis is supported", onlineStoreType) | ||
return nil, fmt.Errorf("%s online store type is currently not supported; only redis and sqlite are supported", onlineStoreType) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add here check for feature values as well, since we read them from the source file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DOne.