support decimal data type for the optimizer rule of PreCastLitInComparisonExpressions #3245
Conversation
liukun4515
force-pushed
the
support_decimal_type_#3031
branch
from
a815c62 to
51029d5
Compare
Codecov Report
@@ Coverage Diff @@
## master #3245 +/- ##
==========================================
+ Coverage 85.89% 85.92% +0.02%
==========================================
Files 294 294
Lines 53373 53442 +69
==========================================
+ Hits 45845 45918 +73
+ Misses 7528 7524 -4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| target_type: &DataType, | ||
| ) -> Result<bool> { | ||
| if integer_lit_value.is_null() { | ||
| ) -> Result<(bool, Option<ScalarValue>)> { |
There was a problem hiding this comment.
It looks like this function always returns either (true, Some(_)) or (false, None) so maybe it should just return the Option without the bool?
There was a problem hiding this comment.
good suggestion.
Done for your comments.
PTAL @andygrove
| match lit_value_target_type { | ||
| None => Ok((false, None)), | ||
| Some(value) => { | ||
| match value >= target_min && value <= target_max { |
There was a problem hiding this comment.
I think it is better to use an if statement rather than a match on a boolean expression
|
@andygrove PTAL, But I think it will conflict with your #3260 |
liukun4515
force-pushed
the
support_decimal_type_#3031
branch
from
a13dd59 to
6170817
Compare
| let (target_min, target_max) = match target_type { | ||
| DataType::Int8 => (i8::MIN as i128, i8::MAX as i128), | ||
| DataType::Int16 => (i16::MIN as i128, i16::MAX as i128), | ||
| DataType::Int32 => (i32::MIN as i128, i32::MAX as i128), | ||
| DataType::Int64 => (i64::MIN as i128, i64::MAX as i128), | ||
| DataType::Decimal128(precision, _) => ( | ||
| MIN_DECIMAL_FOR_EACH_PRECISION[*precision - 1], |
There was a problem hiding this comment.
Could you add a comment here to explain what is going on?
There was a problem hiding this comment.
Thanks @liukun4515. LGTM, although I am not an expert wth decimal manipulation.
|
Benchmark runs are scheduled for baseline = 90a0e7c and contender = b1db5ff. b1db5ff 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?
part of #3031
Rationale for this change
What changes are included in this PR?
In our case, we have many columns with the decimal data type. The decimal column will be applied with filter like
c1 is decimal data type, the literal may be integer or decimal data type.
After this pr, the plan will try to cast the literal to the type of c1.
For example in a table:
The query plan:
explain verbose
Are there any user-facing changes?