Move inlist rule to expr_simplifier #9692
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: jayzhan211 <[email protected]>
jayzhan211
commented
Mar 19, 2024
| ) => | ||
| { | ||
| match (*left, *right) { | ||
| (Expr::InList(l1), Expr::InList(l2)) => { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: jayzhan211 <[email protected]>
7 tasks
alamb
approved these changes
Mar 19, 2024
There was a problem hiding this comment.
Thank you @jayzhan211 -- this is a nice cleanup and removes a copy (for node in the expression tree).
I am also running the planning benchmark to see if this PR provides an improvement
| // 6. `a in (1,2,3,4) AND a not in (1,2,3,4,5) -> a in (), which is false` | ||
| // 7. `a not in (1,2,3,4) AND a in (1,2,3,4,5) -> a = 5` | ||
| // 8. `a in (1,2,3,4) AND a not in (5,6,7,8) -> a in (1,2,3,4)` | ||
| if let Expr::BinaryExpr(BinaryExpr { left, op, right }) = expr.clone() { |
There was a problem hiding this comment.
Removing this expr.clone() is 👍
| Transformed::yes(Expr::InList(merged_inlist)) | ||
| } | ||
|
|
||
| // Simplify expressions that is guaranteed to be true or false to a literal boolean expression |
| @@ -1482,6 +1590,22 @@ impl<'a, S: SimplifyInfo> TreeNodeRewriter for Simplifier<'a, S> { | |||
| } | |||
| } | |||
|
|
|||
| // TODO: We might not need this after defer pattern for Box is stabilized. https://github.com/rust-lang/rust/issues/87121 | |||
|
Testing with git checkout inlist-rule2
cargo bench --bench sql_planner -- all
#alamb@aal-dev:~/arrow-datafusion4$ git merge-base HEAD apache/main
#b0b329ba39403b9e87156d6f9b8c5464dc6d2480
git checkout b0b329ba39403b9e87156d6f9b8c5464dc6d2480
cargo bench --bench sql_planner -- all |
|
I did not measure any significant difference in performance on the |
I doubt if even the rule is included. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #9140.
I think short inlist simplifier should be an independent simplifier that can't be moved to simplifier, so that it will only be run once after all the other inlist simplification. Therefore, close the issue.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?