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

special case concatenating single element array shortcut #492

Merged
merged 1 commit into from
Jun 23, 2021

Conversation

jimexist
Copy link
Member

Which issue does this PR close?

concatenating single element array shortcut

Closes #.

Rationale for this change

no need to any work when the given array is single element

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the arrow Changes to the arrow crate label Jun 23, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #492 (dad8d16) into master (4c7d418) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #492   +/-   ##
=======================================
  Coverage   82.65%   82.65%           
=======================================
  Files         165      165           
  Lines       45524    45536   +12     
=======================================
+ Hits        37628    37640   +12     
  Misses       7896     7896           
Impacted Files Coverage Δ
arrow/src/compute/kernels/concat.rs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c7d418...dad8d16. Read the comment docs.

@@ -57,6 +57,9 @@ pub fn concat(arrays: &[&Array]) -> Result<ArrayRef> {
return Err(ArrowError::ComputeError(
"concat requires input of at least one array".to_string(),
));
} else if arrays.len() == 1 {
let array = arrays[0];
return Ok(array.slice(0, array.len()));
Copy link
Contributor

Choose a reason for hiding this comment

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

You might be able to do something like Ok(Array.clone()) instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

sadly Array isn't cloneable

Copy link
Member Author

Choose a reason for hiding this comment

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

if the param were passing in as ArrayRef then it's possible but it's just pure ref.

Copy link
Member

Choose a reason for hiding this comment

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

fwiw, this is because if it was clonable, it could not be a trait object. .slice(0, array.len()) is fine imo.

Copy link
Member

@jorgecarleitao jorgecarleitao left a comment

Choose a reason for hiding this comment

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

LGTM

@alamb alamb merged commit 38f9bc5 into apache:master Jun 23, 2021
@alamb alamb changed the title concatenating single element array shortcut special case concatenating single element array shortcut Jun 23, 2021
@jimexist jimexist deleted the concat-single-element-array branch June 23, 2021 11:08
alamb added a commit that referenced this pull request Jun 23, 2021
Co-authored-by: Jiayu Liu <Jimexist@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants