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

Add entries to MapBuilder to return both key and value array builders #5218

Merged
merged 2 commits into from
Dec 18, 2023

Conversation

viirya
Copy link
Member

@viirya viirya commented Dec 17, 2023

Which issue does this PR close?

Closes #.

Rationale for this change

MapBuilder provides keys and values functions to get key and value array builders of map separately. As it is mutable borrow of the map builder, when we want to set keys and values in a loop like:

let key_builder = builder.keys();
let value_builder = builder.values();

let keys = ...;
let values = ...;

for idx in 0..keys.get_num_elements() {
    let key = keys.get(idx);
    let value = values.get(idx);

    key_builder.append_value(key);
    value_builder.append_value(value);
}

The compiler will complain error[E0499]: cannot borrow *builder as mutable more than once at a time.

Although there is workaround that is to get key and value builders inside the loop, no sure if it is efficient. More, especially we can have Box<dyn ArrayBuilder> as key and value builders now, it means that we need to downcast both key and value builders for each iteration in the loop.

This patch adds entries API to MapBuilder that can get both key and value builders in one mutable borrow.

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the arrow Changes to the arrow crate label Dec 17, 2023
@@ -119,6 +119,11 @@ impl<K: ArrayBuilder, V: ArrayBuilder> MapBuilder<K, V> {
&mut self.value_builder
}

/// Returns both the key and value array builders of the map
pub fn keys_and_values(&mut self) -> (&mut K, &mut V) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub fn keys_and_values(&mut self) -> (&mut K, &mut V) {
pub fn entries(&mut self) -> (&mut K, &mut V) {

Perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks good to me.

@viirya viirya changed the title Add keys_and_values to MapBuilder to return both key and value array builders Add entries to MapBuilder to return both key and value array builders Dec 17, 2023
@viirya viirya changed the title Add entries to MapBuilder to return both key and value array builders Add entries to MapBuilder to return both key and value array builders Dec 17, 2023
@tustvold tustvold merged commit 9e060dc into apache:master Dec 18, 2023
26 checks passed
@viirya
Copy link
Member Author

viirya commented Dec 18, 2023

Thank you @tustvold

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants