fix: support INT32 type for entity keys #103
Conversation
go/internal/feast/featurestore.go
Outdated
| fvName, _, _ := onlineserving.ParseFeatureReference(featuresRequested) | ||
| _, err := fs.GetFeatureView(fvName, false) | ||
| if err != nil { | ||
| odfv, err := fs.GetOnDemandFeatureView(fvName) |
There was a problem hiding this comment.
When request has features from Feature view and ODFV, this logic fails the request. It's a valid case.
There was a problem hiding this comment.
This code doesn't exist. I modified it to get request sources from on demand FVs only.
| @@ -32,10 +32,10 @@ type Registry struct { | |||
| project string | |||
| registryStore RegistryStore | |||
| cachedFeatureServices map[string]map[string]*core.FeatureService | |||
| cachedEntities map[string]map[string]*core.Entity | |||
| CachedEntities map[string]map[string]*core.Entity | |||
There was a problem hiding this comment.
Can you please revert the changes to this file?
| if len(requestSources) > 0 { | ||
| return requestSources, nil | ||
| } | ||
| return nil, fmt.Errorf("Request sources for feature views %v not found", fVList) |
There was a problem hiding this comment.
ODFVs can exist without Request Sources.
There was a problem hiding this comment.
You mean that ODFVs will have sources other than request sources?
| } | ||
| entitiesProto[key] = value.ToProto() | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Feast does support zero entities
| fVList = append(fVList, fv.Name) | ||
| } | ||
| } else if request.Features != nil && len(request.Features) > 0 { | ||
| log.Info().Msgf("request.Features %v", request.Features) |
There was a problem hiding this comment.
Remove this. This just logs for every request.
| @@ -38,3 +39,11 @@ func TestUnmarshalJSON(t *testing.T) { | |||
| assert.Nil(t, u.UnmarshalJSON([]byte("[[true, false, true], [false, true, false]]"))) | |||
| assert.Equal(t, [][]bool{{true, false, true}, {false, true, false}}, u.boolListVal) | |||
| } | |||
|
|
|||
| func testTypecastToCorrectTypeWithInt32Val(t *testing.T) { | |||
| @@ -1,6 +1,7 @@ | |||
| package server | |||
|
|
|||
| import ( | |||
| prototypes "github.com/feast-dev/feast/go/protos/feast/types" | |||
There was a problem hiding this comment.
Add more tests for the logic added
| } | ||
| } | ||
| } | ||
| } else { |
There was a problem hiding this comment.
When you refine logic, please check if the else logic is still needed.
| fieldType, ok1 := unifiedMap[key] | ||
| if ok1 { | ||
| typecastToEntitySchemaType(&value, fieldType) | ||
| } else { | ||
| logSpanContext.Error().Msgf("Entity type/request source type for key %s not found. Check if your join key names or request sources are correct", key) | ||
| writeJSONError(w, fmt.Errorf("Entity type/request source type for key %s not found. Check if your join key names or request sources are correct", key), http.StatusNotFound) | ||
| return | ||
| } |
There was a problem hiding this comment.
fieldType, ok := unifiedMap[key]
if !ok {
// Error Logic
}
typecastToEntitySchemaType(&value, fieldType)
| for key, value := range request.RequestContext { | ||
| requestContextProto[key] = value.ToProto() | ||
| if len(unifiedMap) > 0 { | ||
| if request.Entities != nil && len(request.Entities) > 0 { |
There was a problem hiding this comment.
you can just use len(request.entities) > 0
There was a problem hiding this comment.
I added that condition in to check for the map, but I guess if i'm checking the length of the entity it's probably not needed. I'll remove it.
What this PR does / why we need it:
Add support for INT32 type for entity keys. This is because there's a bug when using INT32 for entity values, where the feature server would return those values as null. This is because the JSON marshaling for these values are only supported for INT64.
Also add support for request sources to be specified in the
request_contextas wellEdit: We will need to update the Feature Definitions related documentation to be clear on the fact that we should pass entity join keys when retrieving features, not entities themselves and also use the request Context to pass their request sources.
Which issue(s) this PR fixes:
Fixes # https://jira.expedia.biz/browse/EAPC-11996