Introducing list types for feast features retrieval #212
Conversation
E.g., flatten, unique, StdDev, mean, etc.
ariefrahmansyah
force-pushed
the
support-list-features
branch
from
1db7240 to
2b462f1
Compare
| mockFeasts: []mockFeast{ | ||
| { | ||
| request: &feastSdk.OnlineFeaturesRequest{ | ||
| Project: "default", // used as identifier for mocking. must match config |
There was a problem hiding this comment.
Since we don't assert the request to feast, have you make sure it's as expected?
There was a problem hiding this comment.
Yeah, I manually checked it by logging the request here: https://github.com/gojek/merlin/pull/212/files/c9656a225bb7b0d9c3f59bac6f00d8be7c7c4686#diff-744e4800800a7662f4f3e134e1885cbd4571a9f3a802bbedeab46cf168251fe0R777
I don't like this approach though, we should be able to verify the request to the feast.
|
LGTM, this is awesome!! |
| @@ -367,7 +367,7 @@ func broadcastScalar(colMap map[string]interface{}, length int) map[string]inter | |||
| continue | |||
| } | |||
|
|
|||
| val = colValueVal.Index(0) | |||
| val = colValueVal.Index(0).Interface() | |||
There was a problem hiding this comment.
Because, when creating series of list elements, colValueVal.Index(0) returns reflect.Value instead of interface{}. So when we pass these reflect.Value to NewInferType to create the series, it will use wrong series' type [1]->[2], because it sends reflect.Struct instead of the correct slice type.
[1] https://github.com/gojek/merlin/blob/support-list-features/api/pkg/transformer/types/series/series.go#L60-L66
[2] https://github.com/gojek/merlin/blob/support-list-features/api/pkg/transformer/types/series/series.go#L238-L260
|
lgtm |
* Introducing list types for feast features * Add feast list retrieval * Add series's expression functions E.g., flatten, unique, StdDev, mean, etc. * Use github.com/gojekfarm/gota * Address review: Add more test cases * Address review: Assert mock feast on testing
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?:
Checklist