fix decimal precision issue in simplify expression optimize rule#15588
fix decimal precision issue in simplify expression optimize rule#15588alamb merged 7 commits intoapache:mainfrom
Conversation
github-actions
bot
added
optimizer
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
FYI @shehabgamin
Do you have some time to review this PR
Yes! Will carve out some time on Monday. Thanks @jayzhan211 !! |
shehabgamin
left a comment
shehabgamin
left a comment
There was a problem hiding this comment.
@jayzhan211 Nice job! Just left some minor comments :)
| match BinaryTypeCoercer::new(&left_type, op, &right_type).get_result_type() { | ||
| Ok(result_type) => { | ||
| // Only cast if the types differ | ||
| if left_type != result_type { |
There was a problem hiding this comment.
We might as well check if right_type != result_type too to be extra safe, right?
There was a problem hiding this comment.
since we only care about left side expr, whatever right side is the result doesn't change
| match BinaryTypeCoercer::new(&left_type, op, &right_type).get_result_type() { | ||
| Ok(result_type) => { | ||
| // Only cast if the types differ | ||
| if right_type != result_type { |
There was a problem hiding this comment.
We might as well check if left_type != result_type too to be extra safe, right?
| { | ||
| Ok(result_type) => { | ||
| // Only cast if the types differ | ||
| if left_type != result_type { |
There was a problem hiding this comment.
We might as well check if right_type != result_type too to be extra safe, right?
| // Check if resulting type would be different due to coercion | ||
| let left_type = info.get_data_type(&left)?; | ||
| let right_type = info.get_data_type(right)?; | ||
| match BinaryTypeCoercer::new(&left_type, &Operator::Divide, &right_type) |
There was a problem hiding this comment.
This should be op instead of &Operator::Divide
There was a problem hiding this comment.
the function is only used in Divide not other op, different op may behave differently.
In Multiple case, I switch left and right instead.
There was a problem hiding this comment.
@jayzhan211 Got it! Was going to suggest to rename the function for clarity but it looks like you did that in your latest commit!
| info: &S, | ||
| left: Box<Expr>, | ||
| right: &Expr, | ||
| ) -> Result<Transformed<Expr>> { |
There was a problem hiding this comment.
I think op is required if it fits well more then 1 op
shehabgamin
left a comment
shehabgamin
left a comment
There was a problem hiding this comment.
@jayzhan211 @alamb LGTM!
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @jayzhan211 and @shehabgamin
…che#15588) * fix decimal precision * return expr if fail to find the casted type * move fn out * comment * fmt * fix * fix name and doc
3 participants
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?