I have a proposal for a minor(?) refactor of our logical plan ops and I'd like to get other people's thoughts on this (@Sammy Sidhu @Desmond Cheong @Cory Grinstead). The problem - Plan ops are created for various reasons through our code - from our dataframe or sql interfaces, to optimization rules, to even op constructors themselves which can sometimes create other ones. All of these cases generally go through the same `new`/`try_new` constructor for each op, which tries to accommodate all of these use cases. This creates complexity, adds unnecessary compute to planning time, and also conflates user input errors with Daft internal errors. For example, I don't expect any optimization rules to create unresolved expressions, expression resolution should only be done for the builder. Another example is the Join op, where inputs such as

join_prefix

and

join_suffix

are only applicable for renaming columns, which should also only happen via the builder. @Desmond Cheong recently added another initializer to some ops for that reason, but it bypasses the validation that is typically done and is not standardized across ops. My solution - every op should provide a non-erroring

new

constructor which contain explicit debug_asserts for all the requirements about the op's state (one example would be that all expression columns exist in the schema), but otherwise should simply put those values into the struct and return it. • Functions such as

LogicalPlan::with_new_children

will just call

new

. • Other constructors may exist that explicitly provide additional functionality and ultimately call

new

. E.g. a

Join::new_with_renaming_project

to rename the right side columns that conflict with the left side. • Things that may error due to user input, such as expression resolution, should be handled by the logical plan builder.

Is the idiomatic convention here to have a

new_unchecked

?

I’m not advocating for an unchecked constructor here per se, in fact the only thing I think the constructor should do is check its arguments and then put them into the struct

another (more complex) alternative would be to have an additional

ASTPlan

that contains the plan as typed (potentially invalid) that gets converted into the LogicalPlan. IIRC, either @jay or @Sammy Sidhu brought up this idea before.

would the ASTPlan essentially hold the plain representation of a dataframe or sql plan and the translation logic into a valid logical plan?

Left some small comments on both approach and a little code question. • In the slack message you have

new_with_children

delegating to

new

but the Github issue has

new_with_children

delegating to

try_new

. Could you tell me a little bit more about

new_with_children

and maybe there's

try_new_with_children

which follows the same convention? • I do like the

new -> T

/

try_new -> Result<T>

but I'm curious where the

_unchecked

convention might come in? I've only bumped into this when writing unsafe Rust with things like

to_foo -> ResultT>

and

to_foo_unchecked -> T

which I think is slightly different than the new/try_new convention you are proposing.

Thanks!

^ edited, I hadn't setup newlines and sent too early 🙂

@R. C. Howell 1. I've decided to stick with the convention of having an erroring

try_new

to reduce the amount of state validation logic. If I had a

new

that only did (debug_)asserts, then I would also need to potentially check the same things in the builder but with errors. 2. I believe you are correct in that

new_unchecked

is typically used for unsafe code. I don't plan on having that

I think the logicalplanbuilder is probably still the best way to go. For spark at least, I know it would really help to work on a "dataframe" abstraction instead of a logical plan abstraction, but most of the "dataframe" concept only exists in python right now.

@Cory Grinstead I've updated the PR with the refactor implementation for all of our logical ops. please give it a re-approval when you get the opportunity to take a look!