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

Simplified new signature of growable API #238

Merged
merged 2 commits into from Jul 30, 2021
Merged

Conversation

jorgecarleitao
Copy link
Owner

It also removes an extra allocation that was being done internally and an extra check.

Backward incompatible changes:

Growable*::new now expects a vector (not slice) of references of their corresponding types and no longer panic (the type is by definition correct). To migrate, downcast the vector of arrays prior to passing them to the growable.

let array = Utf8Array::<i32>::from_iter(vec![Some("a"), Some("bc"), None, Some("defh")]);
let mut a = GrowableUtf8::new(vec![&array], false, 0);

I.e. signature change is new(&[&'a dyn Array], ...) -> new(Vec<&'a A>, ...) where A is the corresponding array type.

Also removes an extra allocation.
@codecov
Copy link

codecov bot commented Jul 30, 2021

Codecov Report

Merging #238 (f271258) into main (3cd8cff) will decrease coverage by 0.02%.
The diff coverage is 76.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #238      +/-   ##
==========================================
- Coverage   76.82%   76.79%   -0.03%     
==========================================
  Files         229      229              
  Lines       19586    19619      +33     
==========================================
+ Hits        15046    15066      +20     
- Misses       4540     4553      +13     
Impacted Files Coverage Δ
src/compute/filter.rs 63.19% <0.00%> (ø)
src/array/growable/mod.rs 36.23% <45.45%> (+1.53%) ⬆️
src/array/growable/binary.rs 97.29% <100.00%> (-0.08%) ⬇️
src/array/growable/boolean.rs 84.84% <100.00%> (-0.87%) ⬇️
src/array/growable/fixed_binary.rs 86.48% <100.00%> (+2.73%) ⬆️
src/array/growable/list.rs 98.36% <100.00%> (-0.04%) ⬇️
src/array/growable/primitive.rs 100.00% <100.00%> (ø)
src/array/growable/structure.rs 84.25% <100.00%> (ø)
src/array/growable/utf8.rs 100.00% <100.00%> (ø)
src/compute/take/list.rs 100.00% <100.00%> (ø)
... and 2 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 3cd8cff...f271258. Read the comment docs.

@ritchie46
Copy link
Collaborator

I was thinking about a generic constructor to merge the two into one (in #229), but I do think this better. The caller can be responsible for creating the needed data structure.

As there is already a lifetime in Growable... How do you feel about the following?

pub struct GrowableBinary<'a, 'b, O: Offset> {
    arrays: &'b[&'a BinaryArray<O>>],
    ...
}

Or using the same lifetime 'a for the slice/ Cow (in case of ownership problems).

@jorgecarleitao
Copy link
Owner Author

I agree that we can further simplify this. Would it be ok to leave it to a next iteration? I think I need to think about this a bit more: it seems to me that we do not even need the Vec in all cases for as long as we internally allocate references to the supporting buffers and validities. It needs a small internal refactor, but it might work.

@jorgecarleitao jorgecarleitao force-pushed the growable_systematic branch 2 times, most recently from af9de72 to afec796 Compare July 30, 2021 08:14
@jorgecarleitao jorgecarleitao merged commit e08a8a2 into main Jul 30, 2021
@jorgecarleitao jorgecarleitao deleted the growable_systematic branch July 30, 2021 11:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backwards-incompatible enhancement An improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants