Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Extend for Builder #1841

Closed
tustvold opened this issue Jun 10, 2022 · 3 comments · Fixed by #3563
Closed

Implement Extend for Builder #1841

tustvold opened this issue Jun 10, 2022 · 3 comments · Fixed by #3563
Labels
enhancement Any new improvement worthy of a entry in the changelog good first issue Good for newcomers help wanted

Comments

@tustvold
Copy link
Contributor

tustvold commented Jun 10, 2022

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

The FromIterator trait is a standard trait in the ecosystem for building a collection from an iterator. Supporting it within the builders would allow writing code like (not tested)

let strings = ["1", "2", "3"];

let array = strings
    .iter()
    .map(|s| Some(s.parse()?))
    .collect::<Result<Int32Builder>>()
    .unwrap()
    .finish();

Describe the solution you'd like

Implement FromIterator for the Builder types. In most cases FromIterator is implemented for the corresponding Array type, and so it should largely just be a case of moving this logic into the builder and making the array implementation call it.

Describe alternatives you've considered

We could not do this

@tustvold tustvold added enhancement Any new improvement worthy of a entry in the changelog good first issue Good for newcomers help wanted labels Jun 10, 2022
@heyrutvik
Copy link
Contributor

@tustvold I'm trying to understand the ticket. If I'm not wrong, Extend will provide us extend method and then we can extend an existing builder using an iterator.

Example,

let ints = [1, 2, 3];
let mut builder = Int32Builder::new(0);
builder.extend(ints.into_iter());
let array = builder.finish();
assert_eq!(3, array.len());

given that we have impl Extend<i32> for Int32Builder implemented.

I'm confused because you mentioned Extend and not using any of its method in your example snippet. I believe collect is the main focus of your example which builds collection from an iterator. Could you please elaborate?

@tustvold
Copy link
Contributor Author

tustvold commented Jul 7, 2022

Yes this is likely a brain fart on my part, the example should already be possible as we implement FromIterator. I think there is still value in implementing Extend, but perhaps less...

Edit: It was indeed a brain fart, I've updated the issue to be slightly more coherent. Let me know if you have any questions.

@tustvold tustvold changed the title Implement Extend for Builder Implement FromIterator for Builder Jul 7, 2022
@heyrutvik
Copy link
Contributor

heyrutvik commented Jul 10, 2022

Thanks @tustvold for the clarification. I started working on the ticket, and opened a draft PR #2038 for you to review to make sure whether I'm on the right direction or not. It adds FromIterator to BooleanBuilder and uses it in BooleanArray implementation. Once you confirm, I'll continue moving other existing FromIterator implementations. I moved all existing implementations to builder.

@tustvold tustvold changed the title Implement FromIterator for Builder Implement Extend for Builder Jan 19, 2023
tustvold added a commit to tustvold/arrow-rs that referenced this issue Jan 19, 2023
tustvold added a commit that referenced this issue Jan 20, 2023
* Implement Extend for ArrayBuilder (#1841)

* Add dictionaries

* Add tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any new improvement worthy of a entry in the changelog good first issue Good for newcomers help wanted
Projects
None yet
2 participants