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

Add extend_from_iter into growable trait #773

Closed
wants to merge 1 commit into from

Conversation

sundy-li
Copy link
Collaborator

Did not achieve expected performance.

growable::primitive::non_null::non_null                                                                             
                        time:   [2.5629 us 2.5658 us 2.5692 us]

growable::dyn_primitive::non_null::non_null                                                                             
                        time:   [6.3092 us 6.3195 us 6.3328 us]
			
			
After:

growable::dyn_primitive::non_null::non_null                                                                             
                        time:   [6.0020 us 6.0147 us 6.0300 us]


We can't make extend_from_iter accept generic iterator, it'll make this trait none object-safety.

Fixes #772

@codecov
Copy link

codecov bot commented Jan 18, 2022

Codecov Report

Merging #773 (e235988) into main (b70483e) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #773      +/-   ##
==========================================
- Coverage   70.99%   70.99%   -0.01%     
==========================================
  Files         316      316              
  Lines       16733    16736       +3     
==========================================
+ Hits        11879    11881       +2     
- Misses       4854     4855       +1     
Impacted Files Coverage Δ
src/array/growable/mod.rs 20.37% <0.00%> (-1.20%) ⬇️
src/io/parquet/read/nested_utils.rs 78.43% <0.00%> (+0.98%) ⬆️
src/io/avro/read/schema.rs 55.91% <0.00%> (+1.07%) ⬆️

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 b70483e...e235988. Read the comment docs.

@sundy-li
Copy link
Collaborator Author

sundy-li commented Jan 18, 2022

packed_simd is broken in the latest rust nightly toolchain.

datafuselabs/databend#3882

@Xuanwo
Copy link
Contributor

Xuanwo commented Jan 18, 2022

BTW, packed_simd seems superseded by stdsimd.

@ritchie46
Copy link
Collaborator

BTW, packed_simd seems superseded by stdsimd.

We know. See #747. There still are some blockers on that.

@@ -40,6 +40,13 @@ pub trait Growable<'a> {
/// Extends this [`Growable`] with null elements, disregarding the bound arrays
fn extend_validity(&mut self, additional: usize);

/// Extends this [`Growable`] by iterator.
fn extend_from_iter(&mut self, iter: Box<dyn Iterator<Item = (usize, usize, usize)>>) {
Copy link
Owner

Choose a reason for hiding this comment

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

Yeap, I think that this won't work as intended because .extend will still be dynamically dispatched.

We do something like what you intend here: https://github.com/jorgecarleitao/arrow2/blob/main/src/compute/filter.rs#L87

Essentially, specialize the implementation based on the physical type, and use a generic one (https://github.com/jorgecarleitao/arrow2/blob/main/src/compute/filter.rs#L101) for all others.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://users.rust-lang.org/t/is-this-a-curiously-recurring-template-pattern/31004/6

As User @ExpHP said:

trait Trait {
    fn foo(&mut self);
    fn call_foo_twice(&mut self) {
        self.foo(); // <-- this is statically dispatched
        self.foo();
    }

    fn wrap(self) -> Wrapper<Self> { // <-- we can name Self
        Wrapper(self)
    }
}

so .extend is statically dispatched?

@jorgecarleitao
Copy link
Owner

An idea: create a simple function pub fn from_slice_iterator<A: AsRef<dyn Array>, I:Iterator>(&[A], iter: I) -> Box<dyn Array> that matches the arrays' logical type, downcasts them, initializes the static-typed growable, and extend it from the iterator.

@zirconium-n
Copy link

This should be done via extension trait to avoid object safety issue.

@sundy-li
Copy link
Collaborator Author

closing to search for better ideas.

@sundy-li sundy-li closed this May 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Growable by avoiding virtual function call.
5 participants