Simplify small InListExpr #4090
Conversation
Simplify small InListExpr
Dandandan
force-pushed
the
simplify_inlist
branch
from
f8e80e9 to
fb57849
Compare
Dandandan
force-pushed
the
simplify_inlist
branch
from
e58a0aa to
942cb17
Compare
|
Until the fix for #4100 is merged, clippy will be failing on this PR as well |
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs
Outdated
Show resolved
Hide resolved
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs
Outdated
Show resolved
Hide resolved
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
There was a problem hiding this comment.
Looks great! One question I had was whether it makes sense to change THRESHOLD_INLINE_INLIST to be a configurable property (maybe inside SimplifyInfo) since this is more of a specific simplification for datafusion's physical in operator (when n is greater than 1) where other implementations might not have to perform as bad as they do for this smaller lists. Though it might be easier to wait until somebody actually needs to change the limit, so I think as is this PR looks perfect 💯
| expr, | ||
| list, | ||
| negated, | ||
| } if list.len() == 1 |
There was a problem hiding this comment.
Took me a while to understand why the length of 1 is special-cased. Maybe we could mention that the column reference check is strictly for ensuring that we are not doing an unnecessary evaluation of the left side over and over again (so a length of 1 is always fine or if it is something simple as a column access then it is also fine).
There was a problem hiding this comment.
I agree documenting the rationale would be super helpful
| expr, | ||
| list, | ||
| negated, | ||
| } if list.len() == 1 |
There was a problem hiding this comment.
I agree documenting the rationale would be super helpful
I would recommend putting this on |
|
Benchmark runs are scheduled for baseline = 7e944ed and contender = 60f3ef6. 60f3ef6 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
👏 |
* Simplify small InListExpr Simplify small InListExpr * Tweak Tweak * Update datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> * Update datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> * Feedback * Feedback * Tweak * Tweak Tweak * Fmt * clippy Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
* Simplify small InListExpr Simplify small InListExpr * Tweak Tweak * Update datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> * Update datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> * Feedback * Feedback * Tweak * Tweak Tweak * Fmt * clippy Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Which issue does this PR close?
Closes #4089
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?