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.

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

Port ArrayRemove, ArrayRemoveN, ArrayRemoveAll to functions-array subcrate #9656

Merged
merged 2 commits into from
Mar 18, 2024
Merged

Port ArrayRemove, ArrayRemoveN, ArrayRemoveAll to functions-array subcrate #9656

merged 2 commits into from
Mar 18, 2024

Conversation

erenavsarogullari
Copy link
Member

@erenavsarogullari erenavsarogullari commented Mar 17, 2024

Which issue does this PR close?

Closes #9635.

What changes are included in this PR?

This PR aims to do following changes in terms of Epic #9285:
ArrayRemove, ArrayRemoveN, ArrayRemoveAll are ported to functions-array subcrate.

Are these changes tested?

Yes, all array.slt and unnest.slt based ArrayRemove, ArrayRemoveN, ArrayRemoveAll tests are passed.

Are there any user-facing changes?

No

@github-actions github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions labels Mar 17, 2024
@erenavsarogullari
Copy link
Member Author

erenavsarogullari commented Mar 18, 2024

Current Rust / cargo test (win64) build stage failure seems to be related environmental. We can re-trigger this stage if possible.

D:\a\_temp\45213694-13e6-4eb8-97ae-555da09c8695.sh: line 2: /c/Users/runneradmin/.cargo/bin/rustup: Device or resource busy
Error: Process completed with exit code 126.

array_except(
make_array(vec![lit(1), lit(2), lit(3)]),
make_array(vec![lit(1), lit(2)]),
),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you accidentally remove it. Also, add remove's too

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review, addressed.

@jayzhan211
Copy link
Contributor

Current Rust / cargo test (win64) build stage failure seems to be related environmental. We can re-trigger this stage if possible.

D:\a\_temp\45213694-13e6-4eb8-97ae-555da09c8695.sh: line 2: /c/Users/runneradmin/.cargo/bin/rustup: Device or resource busy
Error: Process completed with exit code 126.

It seems like an issue from github

rust-lang/rustup#3709

/// [[1, 2, 3], [2, 3, 4], [2, 3, 4]], [[1, 2, 3], [2, 3, 4], [3, 4, 5]], 1, false => [true, false, false]
/// )
/// ```
pub(crate) fn compare_element_to_list(
Copy link
Contributor

Choose a reason for hiding this comment

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

There are three duplicated code including this one, can you remove other two?

Copy link
Contributor

@jayzhan211 jayzhan211 left a comment

Choose a reason for hiding this comment

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

LGTM

@jayzhan211
Copy link
Contributor

jayzhan211 commented Mar 18, 2024

@erenavsarogullari Let me merge this first, those duplications can be removed later

@jayzhan211 jayzhan211 merged commit c7a72e6 into apache:main Mar 18, 2024
23 checks passed
@erenavsarogullari
Copy link
Member Author

@jayzhan211 Thanks for the merge. I will address duplication by separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logical-expr Logical plan and expressions physical-expr Physical Expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port ArrayRemove, ArrayRemoveN, ArrayRemoveAll to functions-array subcrate
2 participants