-
Notifications
You must be signed in to change notification settings - Fork 998
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 all 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 UserDefinedFunctionV2 { | ||
// 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 FeatureTransformationV2 { | ||
// Note this Transformation starts at 5 for backwards compatibility | ||
oneof transformation { | ||
UserDefinedFunctionV2 user_defined_function = 1; | ||
OnDemandSubstraitTransformationV2 on_demand_substrait_transformation = 2; | ||
} | ||
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 OnDemandSubstraitTransformationV2 { | ||
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.
This hasn't made it to an official release yet, so can't we remove
transformation
oneof here? just leaveUserDefinedFunction user_defined_function = 5 [deprecated = true]
. It would also enable you to call the new fieldtransformation
which sounds less awkward (FeatureTransformationV2 transformation = 10;
) and cut down on overall code size.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 going to do that in a separate PR as a follow up. I'll probably end up with 4 by the end. I want to leave this one as is though.
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.
It's better to have separate clean PRs than one massive one imo.
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.
@tokoko lmk if you're good with this. I have the next PR coming soon where I'll rename things and the subsequent will do the PythonTransformation type.
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.
@franciscojavierarceo yeah, sounds good to me. Let's just not make a new release until we get all these ironed out.