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

Added MutableStructArray #1196

Merged
merged 8 commits into from
Aug 5, 2022
Merged

Added MutableStructArray #1196

merged 8 commits into from
Aug 5, 2022

Conversation

hohav
Copy link
Contributor

@hohav hohav commented Jul 31, 2022

Very rough initial implementation of #703. Hopefully it's at least pointing in the right direction.

@codecov
Copy link

codecov bot commented Jul 31, 2022

Codecov Report

Merging #1196 (aa9af03) into main (3b9d86b) will decrease coverage by 0.17%.
The diff coverage is 38.88%.

@@            Coverage Diff             @@
##             main    #1196      +/-   ##
==========================================
- Coverage   83.48%   83.31%   -0.18%     
==========================================
  Files         356      357       +1     
  Lines       37081    37225     +144     
==========================================
+ Hits        30957    31013      +56     
- Misses       6124     6212      +88     
Impacted Files Coverage Δ
src/array/mod.rs 68.54% <ø> (ø)
src/array/struct_/mod.rs 61.45% <ø> (ø)
src/array/struct_/mutable.rs 38.88% <38.88%> (ø)
src/bitmap/utils/slice_iterator.rs 97.56% <0.00%> (-1.22%) ⬇️
src/io/ipc/read/reader.rs 96.65% <0.00%> (+0.33%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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.

Thanks a lot, @hohav . This looks great!

I agree with that we need to expose &mut A and request the user to extend based on their knowledge of the children's types.

I left minor comments related to performance and documentation, otherwise imo the design makes a lot of sense. 🙇

src/array/struct_/mutable.rs Outdated Show resolved Hide resolved
src/array/struct_/mutable.rs Outdated Show resolved Hide resolved
src/array/struct_/mutable.rs Outdated Show resolved Hide resolved
src/array/struct_/mutable.rs Show resolved Hide resolved
src/array/struct_/mutable.rs Show resolved Hide resolved
false => self.init_validity(),
},
};
// TODO: investigate performance implications of this assertion
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can't get away with this check :) One option we could do here is to offer a function extend_validity() that pushes multiple items into the validity at once, in case the user can/wants to avoid this check on every call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean "can't get away without"?

I wouldn't use extend_validity personally, but I can add it if you think it's useful.

What about adding push_unchecked?

src/array/struct_/mutable.rs Outdated Show resolved Hide resolved
src/array/struct_/mutable.rs Outdated Show resolved Hide resolved
@jorgecarleitao
Copy link
Owner

Would you be able to rebase against main and use the reserve?

@hohav hohav changed the title Initial attempt at MutableStructArray Implement MutableStructArray Aug 5, 2022
@hohav
Copy link
Contributor Author

hohav commented Aug 5, 2022

I rebased and updated reserve. I believe there's only one open question left, about performance of the validity check in push.

@jorgecarleitao jorgecarleitao changed the title Implement MutableStructArray Added MutableStructArray Aug 5, 2022
@jorgecarleitao jorgecarleitao added the feature A new feature label Aug 5, 2022
@jorgecarleitao jorgecarleitao merged commit f485b4d into jorgecarleitao:main Aug 5, 2022
@jorgecarleitao
Copy link
Owner

Thanks a lot @hohav !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants