-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add algebraic simplifications to constant_folding #1208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add algebraic simplifications to constant_folding #1208
Conversation
|
@alamb i took a first cut at adding an optimization and a test for it. would you mind giving it a quick review to make sure its good before i proceed with the rest? |
|
It's pretty bikesheddy so feel free to ignore me but for symmetric matches, like for or, I tend to prefer matching like Which lets there be a single implementation instead of having two nearly identical branches. |
|
@pjmore thank you for the recommendation - it makes sense. i will update. |
|
After making the above simplification getting an error on the inner match... im a bit confused by the error as it looks to me like the match arms have the same types. Will keep looking into it. |
|
I plan to review this PR later today, FYI |
|
@matthewmturner Ah that's what I get for not fully testing my suggestion. The compilation error comes from an ownership issue. As far as I can tell without box patterns or some unnecessary allocations in the fall-through pattern there isn't a way to do what I suggested. Sorry about that, bad suggestion on my part. |
|
@pjmore no problem, thanks for explanation. i think i understand better. To confirm, it's because ill admit im still a little confused why i'll revert to the old implementation for now. |
| _ => Expr::Literal(ScalarValue::Boolean(None)), | ||
| }, | ||
| (Expr::Literal(ScalarValue::Boolean(b)), _) | ||
| if self.is_boolean_type(&right) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alamb to help me understand, can you provide color on whats an example of an Expr that isnt a Boolean but that has a boolean type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm, i get it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One example is if someone writes col_a OR true and col_a has type of int64 or something -- I don't know how far such an expression will get (it will likely error out when trying to to actually run the PhysicalExpr)
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks great @matthewmturner -- thank you! I am sorry for the delay in reviewing
| Expr::Literal(ScalarValue::Boolean(l)), | ||
| Expr::Literal(ScalarValue::Boolean(r)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ScalarValue == ScalarValue case is covered by ConstEvaluator -- so while it is not incorrect to add it here, I also don't think it is necessary (and might be confusing as it would imply ConstEvaluator can't handle this.
| if self.is_boolean_type(&right) => | ||
| { | ||
| match b { | ||
| Some(true) => Expr::Literal(ScalarValue::Boolean(Some(true))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so cool!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can match null as well -- aka ScalarValue::Boolean(None)
because null OR true == true
alamb=# select null or true;
?column?
----------
t
(1 row)
| if self.is_boolean_type(&left) => | ||
| { | ||
| match b { | ||
| Some(true) => Expr::Literal(ScalarValue::Boolean(Some(true))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also add the rules that null OR false --> null
alamb=# select null or false;
?column?
----------
(1 row)
| right, | ||
| }, | ||
| }, | ||
| Operator::And => match (left.as_ref(), right.as_ref()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rules for null and AND are NULL AND true --> NULL, and NULL AND false --> false
alamb=# select null and true;
?column?
----------
(1 row)
alamb=# select null and false;
?column?
----------
f
(1 row)
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthewmturner -- I left some feedback (but I don't think it is required) -- let me know if you want to address it as part of this PR or if we should do it as a follow on PR.
Thanks again!
thanks! yes, i will update this PR. |
|
@alamb I believe ive handled all your comments...can you review when you have time? |
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great -- thank you @matthewmturner very cool
* Add test for showing empty DataFrame and improve print output for empty DataFrames * Add tests for handling empty DataFrames and zero-row queries * Add tests for showing DataFrames with no rows and improve output messages * Fix assertion in test_show_from_empty_batch to ensure proper output for empty DataFrames * feat(tests): add a blank line before test_show_select_where_no_rows function for improved readability
Which issue does this PR close?
Closes #1162
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?