-
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: Rename OnDemandTransformations to Transformations #4038
Merged
Merged
Changes from all commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
7383550
feat: updating protos to separate transformation
franciscojavierarceo 6bcff8d
fixed stuff...i think
franciscojavierarceo ea58ace
updated tests and registry diff function
franciscojavierarceo 1713313
updated base registry
franciscojavierarceo 4a00c12
updated react component
franciscojavierarceo 97a8bb6
formatted
franciscojavierarceo 5190d6c
updated stream feature view proto
franciscojavierarceo 23ae349
making the proto changes backwards compatable
franciscojavierarceo 1d598d2
trying to make this backwards compatible
franciscojavierarceo 81c6f82
caught a bug and fixed the linter
franciscojavierarceo 7687e23
actually linted
franciscojavierarceo 6507808
updated ui component
franciscojavierarceo f44c227
accidentally commented out fixtures
franciscojavierarceo dd2a5ca
Updated
franciscojavierarceo 9ac6793
incrementing protos
franciscojavierarceo 5a1db09
updated tests
franciscojavierarceo e6bf1e9
fixed linting issue and made backwards compatible
franciscojavierarceo 0daf027
feat: Renaming OnDemandTransformations to Transformations
franciscojavierarceo 9417006
updated proto name
franciscojavierarceo eff1497
renamed substrait proto
franciscojavierarceo ae19919
renamed substrait proto
franciscojavierarceo e34b604
updated
franciscojavierarceo 9cd0ebe
updated
franciscojavierarceo 7b9f180
updated integration test
franciscojavierarceo 19544f4
missed one
franciscojavierarceo 41524c9
updated to include Substrait type
franciscojavierarceo 7de39ab
linter
franciscojavierarceo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Empty file.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 know this isn't ready, but let me suggest some names. What if we call this
PythonTransformation
instead ofUserDefinedFunctionV2
. We could reuse that message type both forpandas_transformation
and upcomingpython_transformation
fields and V2 in the naming (I think) will no longer be necessary. wdyt?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 tend to like stating V2 so that people understand it's a replacement for the
deprecated
proto. Are you thinking of makingPythonTransformation
an enum as well withPandas
andPython
as elements? Feel free to suggest what you're thinking to make it a little more concret if you want.My guess is something like
Or something else?
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.
No, that would leave the possibility of having both python and substrait fields set, so probably not the best approach. I was thinking more like this (I'll omit V2s here just for brevity).
note that
pandas_transformation
andpython_transformation
fields share the message type but that's just incidental because it just so happens that they need same type of information. If in the future we see that that's no longer the case, we could introducePandasTransformation
message as well and the first field of transformation will becomePandasTransformation pandas_transformation = 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.
I like the UDF structure as it's a common industry pattern/convention especially for Spark.
@HaoXuAI any thoughts?
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 specifically against UDFs, but the way I like to think about it all these options are sort of udfs anyway, so calling the message just UDF without any quilifier seems redundant, if it was called
PythonUserDefinedFunction
then it would be okay. I guess what I'm saying is I'm equally okay with the trio of (PythonTransformation
,SubstraitTransformation
,PandasTransformation
) and with that of (PythonUDF
,SubstraitUDF
andPandasUDF
).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.
@jeremyary @etirelli any opinions here? I am in favor of
user_defined_function
and the code for this PR is ready otherwise.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.
Lazy consensus will win here. I'm going to merge as is since everything's covered now.