-
Notifications
You must be signed in to change notification settings - Fork 994
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: Add conversion from ValueTypeProto.Enum to new Feast type system #2489
Conversation
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #2489 +/- ##
==========================================
- Coverage 84.63% 84.62% -0.01%
==========================================
Files 130 130
Lines 10998 11031 +33
==========================================
+ Hits 9308 9335 +27
- Misses 1690 1696 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
sdk/python/feast/types.py
Outdated
|
||
class PrimitiveFeastType(Enum): | ||
""" | ||
A PrimitiveFeastType represents a primitive type in Feast. | ||
|
||
Note that these values must match the values in ValueTypeProto.Enum. |
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.
Add a link to the file? Or path?
sdk/python/feast/types.py
Outdated
return PrimitiveFeastType(value_type) | ||
|
||
# Complex types must be constructed. Currently only arrays are supported. Note that | ||
# enum values for arrays are precisely the enum value for the array's base type plus 10. |
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.
Where does this 10 come from?
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.
Honestly a map is better here because this is sure to break if we add enums that aren't supported or something. We should have a map, and a test that iterates over all possible enum values and ensures that you get a valid feast type.
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: achals, felixwang9817 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 |
Signed-off-by: Felix Wang wangfelix98@gmail.com
What this PR does / why we need it: #2475 added a new type system but did not include a method to convert from the existing type system enums (
ValueTypeProto.Enum
) to the new type system. This PR adds that conversion method.Which issue(s) this PR fixes:
Fixes #