Optimizer now simplifies multiplication, division, module arg is a literal Decimal zero or one #3782
Conversation
drrtuy
changed the title
| if *_s < DECIMAL128_MAX_PRECISION && POWS_OF_TEN[*_s as usize] == *v { | ||
| true | ||
| } else { | ||
| false | ||
| } |
There was a problem hiding this comment.
| if *_s < DECIMAL128_MAX_PRECISION && POWS_OF_TEN[*_s as usize] == *v { | |
| true | |
| } else { | |
| false | |
| } | |
| *_s < DECIMAL128_MAX_PRECISION && POWS_OF_TEN[*_s as usize] == *v |
There was a problem hiding this comment.
A way more laconic expression.
… is a literal zero or one and division, modulo when right arg is one
drrtuy
force-pushed
the
simpl_mul_div_mod_decimal_arg
branch
from
01d56d0
to
c531481
Compare
There was a problem hiding this comment.
Looks great to me -- thank you @drrtuy
@liukun4515 can you please confirm this is ok with you as well?
Thanks @alamb I will take a look this issue and pr tomorrow. |
| @@ -137,6 +179,9 @@ fn is_one(s: &Expr) -> bool { | |||
| | Expr::Literal(ScalarValue::UInt64(Some(1))) => true, | |||
| Expr::Literal(ScalarValue::Float32(Some(v))) if *v == 1. => true, | |||
| Expr::Literal(ScalarValue::Float64(Some(v))) if *v == 1. => true, | |||
| Expr::Literal(ScalarValue::Decimal128(Some(v), _p, _s)) => { | |||
| *_s < DECIMAL128_MAX_PRECISION && POWS_OF_TEN[*_s as usize] == *v | |||
There was a problem hiding this comment.
Question: Is there a specific reason to embed a POWS_OF_TEN array instead of dynamically calculating it at the runtime? (or the same goes to taking to log10() of the v, instead of raising _s) E.g.
i128::pow(10, *_s as u32) == *vThere was a problem hiding this comment.
pow call make the patch lesser but will do a real multiplication instead of a pointer deref.
This is perf degradaton IMHO. In the worst case there will be 6 such multiplications(2 for Multiply, 2 for Divide, 2 for Modulo) and there might be multiple scalars binary ops in the expression.
There was a problem hiding this comment.
On the other side I don't see much benefit from using pow except there will be no static array. Not a big profit given a potential perf gain.
There was a problem hiding this comment.
What is your argument in support for using pow() though?
There was a problem hiding this comment.
I think this code will run ~1 per query / expression, so I don't expect the any performance difference to be measurable.
For what it is worth, I think both approaches are reasonable. Thank you both for the discussion.
There was a problem hiding this comment.
If the function or the logic will be call many time in the runtime, it is better to use static value or const value which will get good performance.
|
I took the liberty of resolving the conflict in 47a13f5 |
Much appreciated. |
|
I plan to merge this PR when CI passes (we are a bit backed up now) |
|
Thanks again @drrtuy and @isidentical and @liukun4515 for the discussion and review |
|
Benchmark runs are scheduled for baseline = b0f58dd and contender = a226587. a226587 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
…
Which issue does this PR close?
Closes #3643.
Rationale for this change
To improve expression simplification efficiency.
What changes are included in this PR?
Are there any user-facing changes?
No user-facing changes are expected.