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

Add extend/extend_unchecked for MutableUtf8Array #413

Merged
merged 5 commits into from
Sep 18, 2021

Conversation

VasanthakumarV
Copy link
Contributor

@VasanthakumarV VasanthakumarV commented Sep 16, 2021

This PR adds TrustedLen-based extend and extend_unchecked methods for MutableUtf8Array.

@VasanthakumarV VasanthakumarV marked this pull request as draft September 16, 2021 13:03
Copy link
Contributor Author

@VasanthakumarV VasanthakumarV left a comment

Choose a reason for hiding this comment

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

@jorgecarleitao I started working on MutableUtf8Array, it passes a simple test case.

Will you be able to look at the code, and suggest changes, just to make sure I am on the correct track.

@jorgecarleitao
Copy link
Owner

Thanks for picking this up! This is a great start!

Two notes: if we use a vector, we will need to double-allocate: one for the vector, one for the self.values. The goal is to extend the self.values directly.

An idea: create a new function (not method) fn unsafe extend_from_trusted_len_iter_values and modify trusted_len_values_iter to become something like

fn unsafe trusted_len_values_iter(iterator) {
    /// initialize the state 
    let capacity = iterator.size_hint().1.unwrap();
    let mut offsets = MutableBuffer::with_capacity(capacity + 1);
    offsets.push(O::default());
    let mut values = MutableBuffer::new();
    extend_from_trusted_len_iter_values(&mut offsets, &mut values, iterator);
    (offsets, values)
}

then, move the for loop logic currently in trusted_len_values_iter to extend_from_trusted_len_iter_values. This way, both creation and extension use the same code based (in extend_from_trusted_len_iter_values).

The same ideas can be applied to the one with the validity.

@VasanthakumarV
Copy link
Contributor Author

@jorgecarleitao Now,

If you can review once more, and provide feedback, I will feel more confident working on the others independently, and hopefully disturb you less often.

PS: All of the existing tests and new tests passed locally, I will spend more time cleaning up new tests written for this PR.

@codecov
Copy link

codecov bot commented Sep 17, 2021

Codecov Report

Merging #413 (2d1aa11) into main (ee23796) will decrease coverage by 0.08%.
The diff coverage is 96.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #413      +/-   ##
==========================================
- Coverage   80.96%   80.88%   -0.09%     
==========================================
  Files         347      353       +6     
  Lines       22141    22554     +413     
==========================================
+ Hits        17927    18242     +315     
- Misses       4214     4312      +98     
Impacted Files Coverage Δ
src/array/utf8/mutable.rs 89.91% <94.82%> (+1.29%) ⬆️
tests/it/array/utf8/mutable.rs 100.00% <100.00%> (ø)
src/compute/temporal.rs 83.78% <0.00%> (-10.07%) ⬇️
src/array/ord.rs 64.21% <0.00%> (-2.13%) ⬇️
tests/it/array/mod.rs 100.00% <0.00%> (ø)
tests/it/compute/temporal.rs 100.00% <0.00%> (ø)
src/compute/comparison/mod.rs 92.16% <0.00%> (ø)
src/compute/comparison/utf8.rs 67.41% <0.00%> (ø)
src/compute/comparison/binary.rs 67.41% <0.00%> (ø)
src/compute/comparison/boolean.rs 70.10% <0.00%> (ø)
... and 14 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 ee23796...2d1aa11. Read the comment docs.

Copy link
Owner

@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.

Awesome, looks great! 💯

Let two minor comments.

Feel free to move this to non-draft, change the title to be only for the MutableUtf8Array and we can merge this one.

src/array/utf8/mutable.rs Outdated Show resolved Hide resolved
src/array/utf8/mutable.rs Outdated Show resolved Hide resolved
src/array/utf8/mutable.rs Outdated Show resolved Hide resolved
@VasanthakumarV VasanthakumarV changed the title [WIP] Add extend/extend_unchecked on builder/mutablearrays Add extend/extend_unchecked for MutableUtf8Array Sep 17, 2021
@VasanthakumarV VasanthakumarV marked this pull request as ready for review September 17, 2021 17:41
@jorgecarleitao jorgecarleitao added the enhancement An improvement to an existing feature label Sep 17, 2021
@jorgecarleitao jorgecarleitao merged commit aa42ac2 into jorgecarleitao:main Sep 18, 2021
@VasanthakumarV VasanthakumarV deleted the extend branch September 18, 2021 09:49
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.

2 participants