Improve & unify validation in LogicalPlan::with_new_exprs #12264
Conversation
findepi
force-pushed
the
findepi/improve-unify-validation-in-logicalplan-with-new-exprs-903b47
branch
from
b5efaab
to
97e8e2f
Compare
When adding new plan node type, `LogicalPlan::with_new_exprs` needs to be updated. Different code branches apply different inputs validation style (no validation, just assert, or assert with messages), so it's unclear which code pattern to follow. This commit unifies the validation and adds it to the branches where there was none.
findepi
force-pushed
the
findepi/improve-unify-validation-in-logicalplan-with-new-exprs-903b47
branch
from
97e8e2f
to
e4c5a92
Compare
|
@comphead would you like to take a look? |
| ); | ||
| } | ||
|
|
||
| fn only_expr(&self, mut expr: Vec<Expr>) -> Expr { |
There was a problem hiding this comment.
I doubt whether this is helpful, a little over abstraction IMO 🤔
There was a problem hiding this comment.
i had this initially inline but then the check code would take more space, and it was making it harder to see the important part of the logic (obviously this is subjective)
another subjective bonus is there is type system help -- i cannot check there are no expressions or that there is only one expression and still use expressions vector to pull something later from it.
if you would prefer to have these functions inlined back, just let me know, happy to do so
There was a problem hiding this comment.
Maybe we could add some comments explaining the rationale as a way to make the reason for the extra abstraction clear
There was a problem hiding this comment.
Thanks @findepi it does make sense for me to unify assertions, we probably should not doing assertions itself but return Result from check* methods. And make them better naming like assert instead of check. It would be more intuitive.
Removing swap_remove feels ok, it was an performance issue long time ago rust-lang/rust#52150
We can probably also set #[inline] hit to this methods and check the planner benchmark if we dont have any perf drop
There was a problem hiding this comment.
Thank you @findepi @comphead and @jayzhan211
I think this PR is an improvement as it makes the assumptions about inputs explicit., and since it doesn't change the API for users it is good to me
I do think it would be worth considering @comphead 's suggestion for returning an internal error rather than a panic (what happens when assert fails) but I also think we could do that as a follow on PR or never
| ); | ||
| } | ||
|
|
||
| fn only_expr(&self, mut expr: Vec<Expr>) -> Expr { |
There was a problem hiding this comment.
Maybe we could add some comments explaining the rationale as a way to make the reason for the extra abstraction clear
|
thanks @comphead @alamb for the feedback!
@alamb i applied this suggestion locally already, just pushed out |
|
Thanks again @comphead @jayzhan211 and @findepi |
findepi
deleted the
findepi/improve-unify-validation-in-logicalplan-with-new-exprs-903b47
branch
When adding new plan node type,
LogicalPlan::with_new_exprsneeds tobe updated. Different code branches apply different inputs validation
style (no validation, just assert, or assert with messages), so it's
unclear which code pattern to follow. This commit unifies the
validation and adds it to the branches where there was none.