Conversation
Codecov ReportBase: 83.12% // Head: 83.09% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1311 +/- ##
==========================================
- Coverage 83.12% 83.09% -0.04%
==========================================
Files 369 370 +1
Lines 40180 40386 +206
==========================================
+ Hits 33399 33558 +159
- Misses 6781 6828 +47
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
jorgecarleitao
left a comment
jorgecarleitao
left a comment
There was a problem hiding this comment.
Thanks a lot for the contribution! This looks great!
I left minor comments, but otherwise I think we just need a bit more tests covering the different APIs.
One imo useful test here is:
let mut mutable = MutableMapArray::...
let a = mutable.take_into();
let b: MapArray = mutable.into()
assert_eq!(b.len(), 0)|
|
||
| use super::MapArray; | ||
|
|
||
| /// The mutable version lf [`MapArray`]. |
There was a problem hiding this comment.
| /// The mutable version lf [`MapArray`]. | |
| /// The mutable version of [`MapArray`]. |
| fn take_into(&mut self) -> MapArray { | ||
| MapArray::from_data( | ||
| self.data_type.clone(), | ||
| std::mem::take(&mut self.offsets).into(), |
There was a problem hiding this comment.
if we take here, the offsets end up being empty, which I believe violates an invariant of self
There was a problem hiding this comment.
Ah yeah, the API on MutableArray is unclear here. Shouldn't as_box and as_arc take self instead of &mut self?
There was a problem hiding this comment.
Yes, but that would require MutableArray to not be object safe, and we need that for using it as a trait object.
| /// * The inner types of `data_type` are not equal to those of `values` | ||
| /// * `validity` is not `None` and its length is different from the `values`'s length | ||
| pub fn from_data( | ||
| pub fn try_from_data( |
There was a problem hiding this comment.
could we rename it to try_new instead? It is more aligned with the rest of the containers
|
Hey! Could you rebase so we can merge it in? :) |
2 participants
There's still a few things to do on this (2 todo!()'s still in thewanted to get early feedback.MutableArrayimpl), butCloses #1310