Fix implicit feature set version retrieval #441
Conversation
khorshuheng
changed the title
|
Is it worth having a test that would have caught the issue? |
|
@ches Certainly, let me make unit tests as part of the scope of this PR as well. |
|
/hold |
|
Does this preserve the functionality of being able to retrieve with OR without version? |
|
/unhold |
Yes, it does preserve the functionality. |
Turns out that there is already a test for this: https://github.com/gojek/feast/blob/ff9bb75283cbb2a61a0add8a852ce4c9cfac4ddd/serving/src/test/java/feast/serving/service/CachedSpecServiceTest.java#L217 The reason this is not being caught, is that if https://github.com/gojek/feast/blob/ff9bb75283cbb2a61a0add8a852ce4c9cfac4ddd/serving/src/main/java/feast/serving/specs/CachedSpecService.java#L200 returns the stream in an order already sorted based on version, then the resultant map will still be accurate. I am not sure how to mock this behaviour, so perhaps the fix to the unit test should be another PR as it requires a lot more thoughts. |
|
/lgtm |
|
/approve |
1 similar comment
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: khorshuheng, zhilingc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
In release 4.0 we have introduced implicit version retrieval, where latest version of a feature set will be retrieved if no version is explicitly specified. This feature didn't work as intended, as the feature to feature ref map is incorrectly updated.
Does this PR introduce a user-facing change?:
Implicit version retrieval should work as intended after this fix, for both batch and online retrieval.