Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Nov 28, 2020

Built this based on what I saw existing for the numeric implementations. Most of it was already there.

@github-actions
Copy link

Copy link
Contributor

@nevi-me nevi-me left a comment

Choose a reason for hiding this comment

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

Thanks @wjones1

@alamb this is a low-risk change, and I need to update the nested struct level calculation if it gets merged first, so I'm happy that we merge it soon as someone else's reviewed it, and CI is happy.

}

fn get_bool_array_slice(array: &arrow_array::BooleanArray) -> Vec<bool> {
let mut values = Vec::with_capacity(array.len() - array.null_count());
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic that we were using was incorrect (at least once we get to lists). #8792 fixes this, so if it gets merged first, we would need to change this to populate values correctly .

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

LGTM

}

#[test]
#[should_panic(
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants