Towards #75: Further refactor for using GATs + clippy fixes for rust 1.65 #76

ncpenke · 2022-10-10T20:54:36Z

@aldanor This cleans up your change further by removing ArrowArray. I think the next change that would be worth investigating/doing before generics is using arrow2's StructIter and UnionIter which would remove the need to autogenerate our own StructArray and EnumArray and the unimplemented into_iter. I need to double check the downsides of that. I think I was concerned about having a dyn lookup in an iterator, but perhaps a cleaner API/implementation justifies it for now.

…1.65-beta

ncpenke · 2022-10-10T21:26:08Z

arrow2_convert/src/deserialize.rs

@@ -48,23 +48,36 @@ pub trait ArrowDeserialize: ArrowField + Sized {
    /// something like for<'a> &'a T::ArrayType: IntoIterator<Item=Option<E>>,
    /// However, the E parameter seems to confuse the borrow checker if it's a reference.
    fn arrow_deserialize_internal(


@aldanor Cleaning this up will need a bit more thought. Elements in arrow2 arrays are optional, since they can be nullable. Therefore when converting from Option<ArrowType> -> T (where T is non-nullable), we should check that the arrow array doesn't have a validity bitmap set in this case (otherwise the conversion should just fail upfront). If we can make the deserialize of required fields take ArrowType instead of Option<ArrowType>, that may be much cleaner.

Option/internal stuff aside, being able to take ArrowType instead of Option<ArrowType> would be a great move and make API a lot cleaner in general, without having to mess with options and manual .map() all over the place (also making it more accessible for people who just want to write their own serializers/deserializers).

Thanks for the feedback @aldanor. I started this discussion in arrow2 (jorgecarleitao/arrow2#1273). Lets see where that lands. I don't think this should hold up wrapping up GATs and your generics change. We can make another pass after those changes before v0.4.0 release.

codecov-commenter · 2022-10-10T21:38:06Z

Codecov Report

❗ No coverage uploaded for pull request base (feature/v0.4.0@37f5500). Click here to learn what that means.
The diff coverage is n/a.

@@                Coverage Diff                @@
##             feature/v0.4.0      #76   +/-   ##
=================================================
  Coverage                  ?   95.32%           
=================================================
  Files                     ?        8           
  Lines                     ?     1454           
  Branches                  ?        0           
=================================================
  Hits                      ?     1386           
  Misses                    ?       68           
  Partials                  ?        0

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

aldanor · 2022-10-10T22:23:34Z

arrow2_convert/src/deserialize.rs

    /// Iterator type.
-    type Iter<'a>: Iterator
+    type Iterator<'a>: Iterator


The reason this was Iter is because this is the common naming e.g. in the standard library (this way you can infer what kind of a symbol it is without going to hunt for its definition):

std::option::Iter, std::result::Iter, etc, always a *Iter if it's a type

*Iterator if it's a trait, like DoubleEndedIterator

Definitely +1 on RefIntoIterator, but I think Iter<'_> is more consistent with the commonly used naming scheme?

Do you think it is something we can potentially tackle as well in the GAT rework?

Yes, absolutely. I'll rework this PR based on your feedback, and either include this change in this PR or next one.

aldanor · 2022-10-10T22:25:55Z

arrow2_convert/src/deserialize.rs

+    /// TODO: this can be removed up by using arrow2::array::StructArray and
+    /// arrow2::array::UnionArray to perform the iteration for unions and structs
+    /// which should be possible if structs and unions are deserialized via scalars.


@ncpenke Could you please expand on this? E.g. what needs to be done and what exactly does "deserialized via scalars" mean/imply?

@aldanor The arrow2::array::StructArray iterator iterates over arrow2 scalars (https://github.com/jorgecarleitao/arrow2/blob/main/src/array/struct_/iterator.rs#L27). The concept of arrow scalars is described in the C++ API (https://arrow.apache.org/docs/cpp/api/scalar.html). I don't think they're an official part of the arrow spec, but an implementation choice to hold results of aggregate operations (for example sum of an array).

However, they're also used to represent a single Struct element when iterating over a StructArray. I'm backtracking on this TODO though. I think existing approach is fine if the unimplemented into_iter hack is cleaned up.

aldanor · 2022-10-10T22:31:39Z

arrow2_convert/src/deserialize.rs

+        array: &dyn Array,
+    ) -> Option<<Self::ArrayType as RefIntoIterator>::Iterator<'_>>
+    where
+        Self::ArrayType: 'static,


Does this have to be 'static? Or just within the lifetime of &dyn?

I tried with a named lifetime. I'm not fully sure, but I think 'static is needed for the downcast_ref to facilitate the vtable generation (since in that case it would have to be a type that would live for the lifetime of the program).

Ohh, right, because Any: 'static bound. Nevermind then!

Might even add a comment to avoid future confusion if you'd like:

// because of `Any: 'static` bound

aldanor · 2022-10-10T22:33:19Z

arrow2_convert/src/deserialize.rs

+    ///
+    /// Overridden by struct and enum arrays generated by the derive macro, to
+    /// downcast to the arrow2 array type.
+    fn arrow_array_ref_into_iter(


Merging ArrowArray into ArrowDeserialize has pros and cons - I guess now you have a "standalone" overridable method that doesn't depend on any of the deserialize methods which is a bit weird -- however, this removes an extra trait which is always a plus... maybe it's the right move.

Removing it seemed simpler, but I think I'll have to bring it back in a different way to cleanly iterate through structs and enums.

aldanor · 2022-10-10T22:39:48Z

dyn lookup in an iterator, but perhaps a cleaner API/implementation justifies it for now

I would definitely bench it first, on small examples, like Foo(i32, i32) - how invasive would be dyn lookup there? (I have a feeling it would be noticeable).

aldanor · 2022-10-11T11:12:36Z

Btw, a bit of an offtopic, but related to GATs :) You also mention in the README:

Availability of generaic associated types would simplify the implementation for large and fixed types, since a generic MutableArray can be defined. Ideally for code reusability, we wouldn’t have to reimplement ArrowSerialize and ArrowDeserialize for large and fixed size types since the primitive types are the same. However, this requires the trait functions to take a generic bounded mutable array as an argument instead of a single array type. This requires the ArrowSerialize and ArrowDeserialize implementations to be able to specify the bounds as part of the associated type, which is not possible without generic associated types.

Do you think it is something we can potentially tackle as well in the GAT rework?

aldanor · 2022-10-16T13:39:54Z

@ncpenke Not to rush, but wonder if you have any particular plans for this PR? I'd love to get back working on the generics part but it seems like we need to figure out and stabilize everything else first, otherwise it will be much harder to refactor it...

ncpenke · 2022-10-17T04:30:39Z

@aldanor Thanks for following up. I was waiting for the discussion in arrow2 to be resolved. I will proceed forward on this PR this week so that you’re unblocked for the generics changes.

ncpenke · 2022-10-18T04:42:33Z

@aldanor I’ll reference @jorgecarleitao’s change once it merges to arrow2. I have a change already, but have one thing left to figure out related to using non-null elements. I’ll update this PR soon as that’s done.

aldanor · 2022-10-18T14:11:03Z

@ncpenke Looks like jorgecarleitao/arrow2#1270 has been merged into master, wonder if we can temporarily switch to git-version of arrow2 here so as not to have to wait for the version bump?

aldanor · 2022-10-18T14:38:13Z

Also, if there's anything to bikeshed, discuss or brainstorm (re: figuring things out), would always be glad to help or participate :)

ncpenke · 2022-10-18T19:55:58Z

Thanks @aldanor. I'm planning to point to the latest arrow2 main to use the change. I'll keep you posted if I'm stuck, and ask for feedback. The last couple of weeks have been a bit busy, but I will try to get this wrapped up asap, so that you can move forward with your changes.

- also cleanup dependencies

ncpenke · 2022-10-26T13:14:31Z

@aldanor Just following up, I did a bit of a major refactor and also setup the building blocks to address some of the issues you filed. I'm just wrapping up the serialize path then will post an update.

aldanor · 2022-10-26T13:58:50Z

That's awesome, looking forward to any incoming updates and further bikeshedding :)

aldanor · 2022-11-13T12:41:12Z

@ncpenke Hey, I've been out for a few weeks myself, but wondering what's the current story? Any roadblocks or maybe anything to discuss? Would really want to jump in and help out with anything if I could :)

(Since it's been quite a bit of time already, I'd actually have to re-read my own local branch with generics implementation since I've fell a little bit out of context...)

Plus, GATs have landed already, so we can just continue working on stable.

ncpenke · 2022-11-13T15:38:20Z

Hey @aldanor, thanks for following up. I've been slammed the past few weeks, so I apologize for the slowness in wrapping this change. I very much appreciate your help, and really enjoying our discussion and collaboration.

I agree we can move to stable now, let's do that once this PR is merged.
I pushed up my latest changes with rough notes (please note what I pushed up doesn't build).
I'm pretty happy with deserialize. We handle null and non-null now, and a much cleaner external interface. I'm not so happy with serialize, and that's where this change has been stuck. I need to give it a good think, but basically am trying to see if we can have serialize follow conversion semantics rather than inserting into a mutable array. If we had impl returns in trait it would be easy, so maybe what we have is the best we can do. Please lmk your thoughts.

Besides that and the GAT you're working on, there are a few issues that came in that would be good to fix. We can discuss those once both these changes are done, in case it's something you want help out with. I'm actively looking for an opportunity to carve out some time to wrap up this change, will do it asap.

ncpenke · 2022-11-13T15:44:36Z

@aldanor just a heads up the approach for deserialize also sets up the framework for #79 that you filed via the new GenericList type.

ncpenke · 2023-01-03T16:10:01Z

@aldanor Going to close this PR and rebase off main.

ncpenke changed the base branch from main to feature/v0.4.0 October 10, 2022 20:54

ncpenke force-pushed the ncpenke/gat-cntd branch from ac90962 to f6c9617 Compare October 10, 2022 21:09

ncpenke changed the title ~~Towards #75: Further refactor for using GATs + clippy fixes for rust 1.64~~ Towards #75: Further refactor for using GATs + clippy fixes for rust 1.65 Oct 10, 2022

ncpenke added 2 commits October 10, 2022 14:09

Towards #75: Further refactor for using GATs + clippy fixes for rust …

8d5f8dd

…1.65-beta

Towards #75 Fix CI

783fe65

ncpenke force-pushed the ncpenke/gat-cntd branch from f6c9617 to 783fe65 Compare October 10, 2022 21:10

ncpenke commented Oct 10, 2022

View reviewed changes

Towards #75 Remove stray print

6192b8c

aldanor reviewed Oct 10, 2022

View reviewed changes

ncpenke added 3 commits October 21, 2022 04:57

reference arrow2 main for latest iterator changes

e021b8b

- also cleanup dependencies

cp

eec346b

cp

00a66a6

Notes + serialize WIP

ff6ba00

ncpenke marked this pull request as draft November 13, 2022 15:29

ncpenke closed this Jan 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Towards #75: Further refactor for using GATs + clippy fixes for rust 1.65 #76

Towards #75: Further refactor for using GATs + clippy fixes for rust 1.65 #76

ncpenke commented Oct 10, 2022

ncpenke Oct 10, 2022 •

edited

Loading

aldanor Oct 10, 2022

ncpenke Oct 11, 2022

codecov-commenter commented Oct 10, 2022

aldanor Oct 10, 2022 •

edited

Loading

ncpenke Oct 11, 2022

aldanor Oct 10, 2022

ncpenke Oct 11, 2022

aldanor Oct 10, 2022

ncpenke Oct 11, 2022 •

edited

Loading

aldanor Oct 11, 2022

aldanor Oct 10, 2022

ncpenke Oct 11, 2022

aldanor commented Oct 10, 2022

aldanor commented Oct 11, 2022 •

edited

Loading

aldanor commented Oct 16, 2022

ncpenke commented Oct 17, 2022

ncpenke commented Oct 18, 2022

aldanor commented Oct 18, 2022

aldanor commented Oct 18, 2022

ncpenke commented Oct 18, 2022

ncpenke commented Oct 26, 2022 •

edited

Loading

aldanor commented Oct 26, 2022

aldanor commented Nov 13, 2022

ncpenke commented Nov 13, 2022

ncpenke commented Nov 13, 2022

ncpenke commented Jan 3, 2023

Towards #75: Further refactor for using GATs + clippy fixes for rust 1.65 #76

Towards #75: Further refactor for using GATs + clippy fixes for rust 1.65 #76

Conversation

ncpenke commented Oct 10, 2022

ncpenke Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 10, 2022

Codecov Report

aldanor Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncpenke Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldanor commented Oct 10, 2022

aldanor commented Oct 11, 2022 • edited Loading

aldanor commented Oct 16, 2022

ncpenke commented Oct 17, 2022

ncpenke commented Oct 18, 2022

aldanor commented Oct 18, 2022

aldanor commented Oct 18, 2022

ncpenke commented Oct 18, 2022

ncpenke commented Oct 26, 2022 • edited Loading

aldanor commented Oct 26, 2022

aldanor commented Nov 13, 2022

ncpenke commented Nov 13, 2022

ncpenke commented Nov 13, 2022

ncpenke commented Jan 3, 2023

ncpenke Oct 10, 2022 •

edited

Loading

aldanor Oct 10, 2022 •

edited

Loading

ncpenke Oct 11, 2022 •

edited

Loading

aldanor commented Oct 11, 2022 •

edited

Loading

ncpenke commented Oct 26, 2022 •

edited

Loading