Consolidate ConstantFolding and SimplifyExpression
#1375
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
alamb
changed the title
ConstantFolding and SimplifyExpression
alamb
force-pushed
the
alamb/simplify_simplify
branch
from
fd1c9b8
to
882356e
Compare
alamb
changed the title
ConstantFolding and SimplifyExpressionConstantFolding and SimplifyExpression
alamb
commented
Nov 30, 2021
| // projected columns. With just the projected schema, it's not possible to infer types for | ||
| // expressions that references non-projected columns within the same project plan or its | ||
| // children plans. | ||
| let mut simplifier = |
There was a problem hiding this comment.
this logic is moved from ConstantFolding
Comment on lines
+304
to
+305
| // TODO combine simplify into Simplifier | ||
| let e = simplify(&e); |
There was a problem hiding this comment.
Here is the call into the existing simplify function that is in the SimplifyExpressions pass (originally written by @jgoday)
| let new_name = &new_e.name(plan.schema()); | ||
|
|
||
| // TODO simplify this logic | ||
| if let (Ok(expr_name), Ok(new_expr_name)) = (name, new_name) { |
| @@ -1562,4 +1556,388 @@ mod tests { | |||
| .unwrap(), | |||
| ) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
these tests are just moved from constant_folding.rs
alamb
commented
Nov 30, 2021
| Arc::new(ConstantFolding::new()), | ||
| // Simplify expressions first to maximize the chance | ||
| // of applying other optimizations | ||
| Arc::new(SimplifyExpressions::new()), |
There was a problem hiding this comment.
This change has the additional benefit of allowing even more filters to be pushed down (e.g. column * 1 < 5 which is simplified by the code in simplify to column < 5 can now also be pushed down
houqp
approved these changes
Dec 2, 2021
alamb
added
enhancement
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?
Builds on #1374
Re #1160 (more sophisticated expression simplification) and #1316 don't change names during constant folding
Rationale for this change
ConstantFoldingandSimplifyExpressionboth simplify expressions, in slightly different ways, which is confusing and redundant.It also means bug fixes such as #1319 didn't get applied to
SimplifyExpression.For example, on master (not the column name is
column1, due to the simplification that has occured):On this branch the column is correctly named:
What changes are included in this PR?
ConstantFoldingoptimizer pass by compilingConstantFoldingintoSimplifyExpressionoptimizer passNote:
I plan more PRs to combine
simplifyandSimplifierlogic but to keep reviews easier I am going to do that in another PR