ARROW-11790: [Rust][DataFusion] RFC: Change builder signatures to take Vec<Expr> rather than &[Expr]

[alamb]

NOTE:

Since is a fairly major backwards incompatible change (many callsites need to be updated, though mostly mechanically), I have opened this PR with an example of how it would work for a few callsites, and wanted to gauge community interest before getting the whole thing working.

It is part of my overall plan to make the DataFusion optimizer more idiomatic and do much less copying ARROW-11689

Rationale:

All callsites currently need an owned Vec (or equivalent) so they can pass in &[Expr] and then Datafusion copies all the Exprs. Many times the original Vec<Expr> is discarded immediately after use (I'll point out where this happens in a few places below). Thus I it would better (more idiomatic and often less copy/faster) to take something that could produce an iterator over Expr

Changes

Change Dataframe / LogicalPlanBuilder so it takes Vec<Expr> rather than &[Expr]

[github-actions]

https://issues.apache.org/jira/browse/ARROW-11790

[alamb]

Here is the API change -- most of the rest of this PR is updating all the call sites. Note that DataFrame::filter takes in an owned Expr so taking an owned Vec is not that large of a departure

[alamb]

Here is where the Expr::clone occurs and no longer does after the changes contemplated in this PR

[alamb]

I would love your thoughts on this approach @jorgecarleitao @andygrove @houqp @Dandandan @ilya-biryukov @seddonm1 -- I wonder if this approach would be worth bringing up on the mailing list

[houqp]

Since it aligns with our previous discussion at #9347 (review), I am all for this change! I think we should adopt this pattern across the entire code base. Clone should be managed at callsites whenever possible.

[andygrove]

I am all for this change. I had started out with the pattern of passing Vec but at the time I got feedback that this wasn't idiomatic Rust code and that we should always pass slices, so had switched to that pattern.

[houqp]

this wasn't idiomatic Rust code and that we should always pass slices

I also remember seeing that recommendation before and adopted it for awhile. I wonder how wide-spread this is in the Rust community 🤔

[Dandandan]

I think this makes sense. This is a partial revert from a PR from me (I believe). But I think it is better for "builder" kind of functions where it needs "owned" data.

In general, &Vec and &String are anti patterns though, and some functions taking Vec and String still could/should be using references instead.

[houqp]

In general, &Vec and &String are anti patterns though, and some functions taking Vec and String still could/should be using references instead.

Yeah, I think it's always better to use &[] instead of &Vec and &str instead of &String.

In case of Vec v.s. &[], I think it largely depends on the clone overhead. For example, if the function doesn't do any clone at all internally, then we should definitely pass in &[] instead of an owned Vec. If there is a decent amount of clone overhead, then it's better to manage the clone on callsite.

[Dandandan]

In general, &Vec and &String are anti patterns though, and some functions taking Vec and String still could/should be using references instead.

Yeah, I think it's always better to use &[] instead of &Vec and &str instead of &String.

In case of Vec v.s. &[], I think it largely depends on the clone overhead. For example, if the function doesn't do any clone at all internally, then we should definitely pass in &[] instead of an owned Vec. If there is a decent amount of clone overhead, then it's better to manage the clone on callsite.

Exactly, this is my current understanding too. It is best to evaluate on a case by case basis 👍

[jorgecarleitao]

LGTM. Thanks a lot, @alamb !

I agree with what was said: when the function needs ownership, it is better to leave the caller to make the decision on whether they want to clone it and keep the object or pass the ownership to the function.

I do think that in some cases we could simplify some structs by having a lifetime parameter and &'a [] instead of Vec, but since this is not high performant code, I do not think we need to worry much.

[alamb]

@Dandandan and @houqp

What do you think about something like this (which is even more Idiomatic (TM) I think), though harder for beginner Rust programmers to grok?

    pub fn project(&self, expr: impl IntoIterator<Item=Expr>) -> Result<Self> {

This would allow PlanBuilder::project to be called with a Vec<Expr> as well as things like BTreeSet<Expr>...

Sadly, I couldn't give the same treatment to the DataFrame trait as when I tried, rustc started complaining to me about trait objects

dataframe::DataFrame` cannot be made into an object
   |
   = help: consider moving `select` to another trait
note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>

[houqp]

Yes, I think if the function only requires sequential access, iterator would have been even better. Although I would prefer to write it as a generic function rather than using Trait object.

[jorgecarleitao]

Another option is to use T: AsRef<[Expr]>, which works for any type that can be represented as a reference to an array (e.g. vec and &[]`)

@houqp , AFAIK impl is just syntactic sugar for a generic; a trait object would be &dyn Expr.

I am not sure the IntoIterator or AsRef would help, though: we would still need to collect it into a vector, which afaik results in a clone.

[houqp]

@jorgecarleitao yeah, you are right, sorry I got it mixed up with dyn :P

I think IntoIterator<Item = Expr> won't require copy or clone because the iterator owns those values. AsRef with slice on the other side would require copy/clone due to use of slice, which behaves closer to IntoIterator<Item=&Expr>.

[jorgecarleitao]

@houqp , you are right, AsRef does not work. 👍 playground.

Thank you for that piece of knowledge! :)

[alamb]

Closing in favor of #9703