-
Notifications
You must be signed in to change notification settings - Fork 1.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
Implement array_pop_back
function
#7348
Conversation
@@ -601,6 +601,22 @@ pub fn array_slice(args: &[ArrayRef]) -> Result<ArrayRef> { | |||
define_array_slice(list_array, key, extra_key, false) | |||
} | |||
|
|||
pub fn array_pop_back(args: &[ArrayRef]) -> Result<ArrayRef> { |
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.
@tanruixiang Do you want to implement array_pop_front
function (See the ticket: #6996)? It has the similar syntax and it's a quite improvement your contribution in the project.
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.
Yes, I would love to implement array_pop_front
, do I need to implement array_pop_front
in this pr? Or in the next pr.
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.
Next pr.
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 is the excellent improvement 🥇! Thank you, @tanruixiang!
cc @alamb @jayzhan211
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!
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 too -- thank you @tanruixiang
I think this PR just needs to have conflicts resolved and it should be good to merge
Thank you @jayzhan211 and @izveigor for reviews
@alamb @jayzhan211 @izveigor Thanks to all for the reviews, the conflict has been resolved! |
Thanks @tanruixiang |
Which issue does this PR close?
Closes #6997 .
Rationale for this change
see #6997
What changes are included in this PR?
Are these changes tested?
yes
Are there any user-facing changes?