-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Implement user defined planner for create_struct
& create_named_struct
#11273
Merged
Merged
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
30672ce
add UserDefinedSQLPlanner for create struct
dharanad 727451c
fix linting
dharanad 44994e0
add create name struct user defined sql planner
dharanad 52df4c6
simplify
dharanad 1f08951
refactor
dharanad 29ce24c
Merge branch 'refs/heads/main' into move-create_struct-to-udf
dharanad 6b41353
refactor
dharanad 953ad31
remove named_struct from functions
dharanad eca8c9d
formatting
dharanad 1dcca70
revert 953ad31
dharanad bc6afff
update docs
dharanad 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
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.
@jayzhan211 Is this what u suggesting ?
If you were suggesting to move
create_struct_expr
&create_named_struct_expr
to planner. I see an issues over there i.e these functions intnerally callself.sql_expr_to_logical_expr
. I would rather have it here to keep it simple. Sorry if i understood it wrong.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.
Maybe like #11318 ?
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.
Based on my understanding we also have a
named_struct
function which is an alias forstruct
function with input expressions optionally named.In this PR i am not moving
named_struct
function to udp.My understanding was to move
create_named_struct
&create_struct
which were solely handlingstruct
function.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.
Right, they are not possible to move into the planner. Another thing we can improve on is, forgetting about
create_struct_expr
and usecreate_named_struct_expr
andnamed_struct
. But, we can change this in next PRThere 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.
Liked mentioned earlier. Thats a great idea, i will make the change in another PR. Thanks for your help @jayzhan211