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

Introduce IterRef trait for deserialization #73

Merged
merged 3 commits into from
Oct 10, 2022

Conversation

aldanor
Copy link

@aldanor aldanor commented Oct 8, 2022

No description provided.

@aldanor
Copy link
Author

aldanor commented Oct 8, 2022

Wonder if we can get rid of deserialize_internal() too... 🤔 (from what I can tell, removing it from Option doesn't "confuse the borrow checker" as is stated in the comments, at least not anymore; it's its usage in the vec deserializers that I'm a bit confused about)

@ncpenke
Copy link
Collaborator

ncpenke commented Oct 10, 2022

@aldanor Thanks for adding the changelog and the changes. I think we may be able to remove the ArrowArray trait. I'll take a stab at this now. CI is failing, but I think will be fixed once we merge the workflow changes, so merging the change as-is.

@ncpenke ncpenke merged commit 37f5500 into DataEngineeringLabs:feature/v0.4.0 Oct 10, 2022
@aldanor aldanor deleted the feature/gat branch October 10, 2022 17:48
@@ -71,11 +88,11 @@ macro_rules! impl_arrow_array {
impl ArrowArray for $array {
type BaseArrayType = Self;

fn iter_from_array_ref(b: &dyn Array) -> <&Self as IntoIterator>::IntoIter {
fn iter_from_array_ref(b: &dyn Array) -> <Self as IterRef>::Iter<'_> {
b.as_any()
.downcast_ref::<Self::BaseArrayType>()
.unwrap()
Copy link
Author

Choose a reason for hiding this comment

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

@ncpenke I was wondering about this unwrap - can it panic?... This doesn't look too nice

Copy link
Collaborator

Choose a reason for hiding this comment

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

@aldanor In theory it can, though we do a top-level schema check. But if the underlying arrow structure is different than the schema it advertises, then we can panic. I'll fix it in my upcoming change. I'll add you as a reviewer so that you can include any other feedback.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@aldanor Just a heads up another thing we need to do is introduce our own error type (#54). We're currently piggybacking-off arrow2's type.

Copy link
Author

Choose a reason for hiding this comment

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

Yea, I can take on that (the error type).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! I'm almost done with my changes. I just changed the unwrap to an Option, and will leave the error type change to you.

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

Successfully merging this pull request may close these issues.

2 participants