Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement func array_pop_front #8142

Merged
merged 5 commits into from
Nov 15, 2023
Merged

Implement func array_pop_front #8142

merged 5 commits into from
Nov 15, 2023

Conversation

Veeupup
Copy link
Contributor

@Veeupup Veeupup commented Nov 12, 2023

Which issue does this PR close?

Closes #6996 .

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt) labels Nov 12, 2023
@Veeupup Veeupup marked this pull request as draft November 12, 2023 05:23
@Veeupup Veeupup marked this pull request as ready for review November 12, 2023 11:11
@Veeupup
Copy link
Contributor Author

Veeupup commented Nov 12, 2023

@alamb @jayzhan211 hi! this PR is ready for review now! : )

@jayzhan211
Copy link
Contributor

@Veeupup Would you like to try rewriting array_pop_front / back with MutableArrayData? I think it will be much easier to understand than the current slice! and list_slice! macros.

@Veeupup
Copy link
Contributor Author

Veeupup commented Nov 12, 2023

@Veeupup Would you like to try rewriting array_pop_front / back with MutableArrayData? I think it will be much easier to understand than the current slice! and list_slice! macros.

yeah, agree with you.. to understand what slice! macro does cost me some time..

but this macro has been used for some other functions and for now implementation with slice! maybe the simplest way to do with it?

@jayzhan211
Copy link
Contributor

jayzhan211 commented Nov 12, 2023 •
edited
Loading

@Veeupup Would you like to try rewriting array_pop_front / back with MutableArrayData? I think it will be much easier to understand than the current slice! and list_slice! macros.

yeah, agree with you.. to understand what slice! macro does cost me some time..

but this macro has been used for some other functions and for now implementation with slice! maybe the simplest way to do with it?

If you also spend much time on slice!, it is a good reason for us to improve on this.

comphead
comphead reviewed Nov 13, 2023
View reviewed changes
Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Veeupup
Please more tests with nulls and empty arrays, please try to think up any possible case to prevent bug to flow in.

And since new built in function gets introduced we need to get this documented in scalar_functions.md and expressions.md

@Veeupup
Copy link
Contributor Author

Veeupup commented Nov 14, 2023 •
edited
Loading

Thanks @Veeupup Please more tests with nulls and empty arrays, please try to think up any possible case to prevent bug to flow in.

And since new built in function gets introduced we need to get this documented in scalar_functions.md and expressions.md

very thanks for your advice! docs have been added!

and the test for empty and NULL has been already added, and you can check empty array test and NULL value test.

@Veeupup
implement array_pop_front
67fe90e 
Signed-off-by: veeupup <code@tanweime.com>
@Veeupup
abstract array_pop
f8e8c92
@Veeupup
fix cargo check
411562d
@Veeupup
add docs for array_pop_front
ec688a4 
Signed-off-by: veeupup <code@tanweime.com>
@Veeupup
fix comments
ffc5af8
alamb
alamb approved these changes Nov 15, 2023
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Veeupup

@alamb alamb merged commit 6b945a4 into apache:main Nov 15, 2023
23 checks passed
@Veeupup Veeupup deleted the array_pop_front branch November 21, 2023 15:06
@matthewgapp matthewgapp mentioned this pull request Jan 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@comphead comphead comphead left review comments

@jayzhan211 jayzhan211 jayzhan211 approved these changes

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
logical-expr Logical plan and expressions physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement array_pop_front function
4 participants
@Veeupup @jayzhan211 @alamb @comphead