Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Removed un-necessary allocation in assign_ops #1085

Merged
merged 1 commit into from
Jun 19, 2022
Merged

Conversation

jorgecarleitao
Copy link
Owner

This PR removes unnecessary allocation on assign_ops.

When converting PrimitiveArray -> MutablePrimitiveArray, we need to check that both the values and validities are unique refs. However, when operating over values and validities independently, we do not need a mutable ref to both at the same time.

A common case where this was hitting was when rhs had a validity and lhs did not. In this case, we first cloned the rhs's validity for lhs, but them tried to get a mut ref to both the validity and the values (to only use the values). This would naturally fail, causing us to have to re-use the lhs.

This PR adds PrimitiveArray::get_mut_values and removes (un-released) functions from PrimitiveArray, since the function get_mut_values is more generic.

@jorgecarleitao jorgecarleitao added the enhancement An improvement to an existing feature label Jun 19, 2022
@codecov
Copy link

codecov bot commented Jun 19, 2022

Codecov Report

Merging #1085 (75c7bfb) into main (16d01ef) will decrease coverage by 0.05%.
The diff coverage is 61.90%.

@@            Coverage Diff             @@
##             main    #1085      +/-   ##
==========================================
- Coverage   81.14%   81.08%   -0.06%     
==========================================
  Files         367      367              
  Lines       35316    35281      -35     
==========================================
- Hits        28657    28609      -48     
- Misses       6659     6672      +13     
Impacted Files Coverage Δ
src/array/boolean/mod.rs 80.86% <ø> (-4.01%) ⬇️
src/array/primitive/mutable.rs 95.57% <ø> (+0.63%) ⬆️
src/compute/arity_assign.rs 64.00% <46.42%> (-10.08%) ⬇️
src/buffer/immutable.rs 83.95% <87.50%> (-4.94%) ⬇️
src/array/primitive/mod.rs 79.53% <100.00%> (-2.13%) ⬇️
src/bitmap/assign_ops.rs 83.06% <0.00%> (-3.23%) ⬇️
... and 5 more

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 16d01ef...75c7bfb. Read the comment docs.

@ritchie46
Copy link
Collaborator

However, when operating over values and validities independently, we do not need a mutable ref to both at the same time.

Good observation.

src/array/primitive/mutable.rs Show resolved Hide resolved
src/array/primitive/mod.rs Show resolved Hide resolved
src/compute/arity_assign.rs Show resolved Hide resolved
@jorgecarleitao jorgecarleitao merged commit d1ab4ef into main Jun 19, 2022
@jorgecarleitao jorgecarleitao deleted the improve_assign branch June 19, 2022 13:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement An improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants