-
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
feat: Updating protos to separate transformation #4018
Changes from 8 commits
8f97cf1
bc981c7
326ca4a
0ea16a3
5121d8d
cbbfcf8
01b82c8
8712b2e
f1e3764
21c1c35
ff57b45
6de6fcc
1212c12
2883d1b
ea5e559
ef0795b
529acac
6748fe7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
syntax = "proto3"; | ||
package feast.core; | ||
|
||
option go_package = "github.com/feast-dev/feast/go/protos/feast/core"; | ||
option java_outer_classname = "FeatureTransformationProto"; | ||
option java_package = "feast.proto.core"; | ||
|
||
import "google/protobuf/duration.proto"; | ||
|
||
// Serialized representation of python function. | ||
message UserDefinedFunction { | ||
// The function name | ||
string name = 1; | ||
|
||
// The python-syntax function body (serialized by dill) | ||
bytes body = 2; | ||
|
||
// The string representation of the udf | ||
string body_text = 3; | ||
} | ||
|
||
// A feature transformation executed as a user-defined function | ||
message FeatureTransformation { | ||
// Note this Transformation starts at 5 for backwards compatibility | ||
oneof transformation { | ||
UserDefinedFunction user_defined_function = 5; | ||
OnDemandSubstraitTransformation on_demand_substrait_transformation = 6; | ||
} | ||
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's a good time to rethink the naming here. My first suggestion was to rename the field (not message type) to
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 was planning on doing that in a follow up PR to not add too much complexity here. |
||
} | ||
|
||
message OnDemandSubstraitTransformation { | ||
bytes substrait_plan = 1; | ||
} |
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.
What's the point of
mode
field here since this is already a breaking change anyway?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.
For my team at Affirm :)
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.
I remember that, but I'm a little confused. If you care about proto encoding compatibility, this change will break your protos anyway, with or without
mode
field. If all you care about is user-facing API (in other wordsmode
parameter inon_demand_feature_view
decorator) we can do that without a redundantmode
field in the proto.mode
parameter inon_demand_feature_view
would simply determine which one ofoneof
fields will be populated. Maybe I'm missing something...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.
That's a fair point but it does make API interface similar to stream feature view, which I prefer.
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.
I'm not disputing the
mode
field in the API. I agree we should do that as part of native python PR. All I'm saying is we don't need amode
field in proto for that to be possible.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.
i gues @tokoko means to handle
mode
in Python only. either way it works. Explicitly adding it in the proto does help understanding the Feature View schema.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.
+1