Skip to content

Commit

Permalink
tidb_query: fix convert integer to duration (#6459)
Browse files Browse the repository at this point in the history 
Signed-off-by: Lonng <heng@lonng.org>
  • Loading branch information
@lonng @sre-bot
lonng authored and sre-bot committed Jan 10, 2020
1 parent 8a54289 commit 4aa395a
Show file tree
Hide file tree
Showing 2 changed files with 112 additions and 186 deletions.
156 changes: 30 additions & 126 deletions components/tidb_query/src/codec/mysql/duration.rs
View file
Expand Up @@ -9,7 +9,7 @@ use codec::prelude::*;
use super::{check_fsp, Decimal};
use crate::codec::convert::ConvertTo;
use crate::codec::error::{ERR_DATA_OUT_OF_RANGE, ERR_TRUNCATE_WRONG_VALUE};
use crate::codec::mysql::MAX_FSP;
use crate::codec::mysql::{Time as DateTime, TimeType, MAX_FSP};
use crate::codec::{Error, Result, TEN_POW};
use crate::expr::EvalContext;

Expand Down Expand Up @@ -483,6 +483,10 @@ impl Duration {
micros.unwrap_or(0),
);

if minute >= 60 || second > 60 {
return Err(Error::truncated_wrong_val("time", format!("{:?}", input)));
}

if hour == 0 && minute == 0 && second == 0 && micros == 0 {
neg = false;
}
Expand Down Expand Up @@ -648,13 +652,24 @@ impl Duration {
self.format("")
}

/// If the error is overflow, the result will be returned, too.
/// Otherwise, only one of result or err will be returned
pub fn from_i64_without_ctx(mut n: i64, fsp: i8) -> Result<Duration> {
pub fn from_i64(ctx: &mut EvalContext, mut n: i64, fsp: i8) -> Result<Duration> {
let fsp = check_fsp(fsp)?;
if n > i64::from(MAX_DURATION_VALUE) || n < -i64::from(MAX_DURATION_VALUE) {
// FIXME: parse as `DateTime` if `n >= 10000000000`
return Err(Error::overflow("Duration", n));
if n >= 10000000000 {
if let Ok(t) = DateTime::parse_from_i64(ctx, n, TimeType::DateTime, fsp as i8) {
return t.convert(ctx);
}
}
ctx.handle_overflow_err(Error::overflow("Duration", n))?;
// Returns max duration if overflow occurred
return Ok(Duration::new(
n < 0,
MAX_HOURS,
MAX_MINUTES,
MAX_SECONDS,
0,
fsp as u8,
));
}

let negative = n < 0;
Expand All @@ -677,25 +692,6 @@ impl Duration {
);
Ok(dur)
}

pub fn from_i64(ctx: &mut EvalContext, n: i64, fsp: i8) -> Result<Duration> {
Duration::from_i64_without_ctx(n, fsp).or_else(|e| {
if e.is_overflow() {
ctx.handle_overflow_err(e)?;
// Returns max duration if overflow occurred
Ok(Duration::new(
n < 0,
MAX_HOURS,
MAX_MINUTES,
MAX_SECONDS,
0,
fsp as u8,
))
} else {
Err(e)
}
})
}
}

impl ConvertTo<f64> for Duration {
Expand Down Expand Up @@ -940,6 +936,8 @@ mod tests {
(b"18446744073709551615:59:59", 0, None),
(b"1::2:3", 0, None),
(b"1.23 3", 0, None),
(b"1:62:3", 0, None),
(b"1:02:63", 0, None),
];

for (input, fsp, expect) in cases {
Expand Down Expand Up @@ -1253,124 +1251,30 @@ mod tests {
(8376049, 0, Err(Error::truncated_wrong_val("", "")), false),
(8375960, 0, Err(Error::truncated_wrong_val("", "")), false),
(8376049, 0, Err(Error::truncated_wrong_val("", "")), false),
// TODO: fix these test case after Duration::from_f64
// had impl logic for num>=10000000000
(
10000000000,
0,
Ok(Duration::new(
false,
MAX_HOURS,
MAX_MINUTES,
MAX_SECONDS,
0,
0,
)),
true,
Ok(Duration::new(false, 0, 0, 0, 0, 0)),
false,
),
(
10000235959,
0,
Ok(Duration::new(
false,
MAX_HOURS,
MAX_MINUTES,
MAX_SECONDS,
0,
0,
)),
true,
Ok(Duration::new(false, 23, 59, 59, 0, 0)),
false,
),
(
10000000001,
-10000235959,
0,
Ok(Duration::new(
false,
MAX_HOURS,
MAX_MINUTES,
MAX_SECONDS,
0,
0,
)),
true,
),
(
10000000000,
5,
Ok(Duration::new(
false,
MAX_HOURS,
MAX_MINUTES,
MAX_SECONDS,
0,
5,
)),
true,
),
(
10000235959,
5,
Ok(Duration::new(
false,
true,
MAX_HOURS,
MAX_MINUTES,
MAX_SECONDS,
0,
5,
)),
true,
),
(
10000000001,
5,
Ok(Duration::new(
false,
MAX_HOURS,
MAX_MINUTES,
MAX_SECONDS,
0,
5,
)),
true,
),
(
10000000000,
6,
Ok(Duration::new(
false,
MAX_HOURS,
MAX_MINUTES,
MAX_SECONDS,
0,
6,
)),
true,
),
(
10000235959,
6,
Ok(Duration::new(
false,
MAX_HOURS,
MAX_MINUTES,
MAX_SECONDS,
0,
6,
)),
true,
),
(
10000000001,
6,
Ok(Duration::new(
false,
MAX_HOURS,
MAX_MINUTES,
MAX_SECONDS,
0,
6,
)),
true,
false,
),
];
for (input, fsp, expect, overflow) in cs {
Expand Down
142 changes: 82 additions & 60 deletions components/tidb_query/src/rpn_expr/impl_cast.rs
View file
Expand Up @@ -835,16 +835,14 @@ fn cast_int_as_duration(
None => Ok(None),
Some(val) => {
let fsp = extra.ret_field_type.get_decimal() as i8;
Duration::from_i64_without_ctx(*val, fsp)
.map(Some)
.or_else(|err| {
if err.is_overflow() {
ctx.handle_overflow_err(err)?;
Ok(None)
} else {
Err(err.into())
}
})
Duration::from_i64(ctx, *val, fsp).map(Some).or_else(|err| {
if err.is_overflow() {
ctx.handle_overflow_err(err)?;
Ok(None)
} else {
Err(err.into())
}
})
}
}
}
Expand Down Expand Up @@ -4892,7 +4890,14 @@ mod tests {

#[test]
fn test_int_as_duration() {
test_none_with_ctx_and_extra(cast_int_as_duration);
// None
{
let output: Option<Real> = RpnFnScalarEvaluator::new()
.push_param(ScalarValue::Bytes(None))
.evaluate(ScalarFuncSig::CastIntAsDuration)
.unwrap();
assert_eq!(output, None);
}

// This case copy from Duration.rs::tests::test_from_i64
let cs: Vec<(i64, isize, crate::codec::Result<Option<Duration>>, bool)> = vec![
Expand Down Expand Up @@ -4945,64 +4950,81 @@ mod tests {
Ok(Some(Duration::parse(b"-838:59:59", 6).unwrap())),
false,
),
// will overflow
(8385960, 0, Ok(None), true),
(8385960, 1, Ok(None), true),
(8385960, 5, Ok(None), true),
(8385960, 6, Ok(None), true),
(-8385960, 0, Ok(None), true),
(-8385960, 1, Ok(None), true),
(-8385960, 5, Ok(None), true),
(-8385960, 6, Ok(None), true),
// overflow as warning
(
8385960,
0,
Ok(Some(Duration::parse(b"838:59:59", 0).unwrap())),
true,
),
(
-8385960,
0,
Ok(Some(Duration::parse(b"-838:59:59", 0).unwrap())),
true,
),
// will truncated
(8376049, 0, Err(Error::truncated_wrong_val("", "")), false),
(8375960, 0, Err(Error::truncated_wrong_val("", "")), false),
(8376049, 0, Err(Error::truncated_wrong_val("", "")), false),
// TODO: add test for num>=10000000000
// after Duration::from_f64 had impl logic for num>=10000000000
// (10000000000, 0, Ok(Duration::parse(b"0:0:0", 0).unwrap())),
// (10000235959, 0, Ok(Duration::parse(b"23:59:59", 0).unwrap())),
// (10000000000, 0, Ok(Duration::parse(b"0:0:0", 0).unwrap())),
(
10000000000,
0,
Ok(Some(Duration::parse(b"0:0:0", 0).unwrap())),
false,
),
(
10000235959,
0,
Ok(Some(Duration::parse(b"23:59:59", 0).unwrap())),
false,
),
(
-10000235959,
0,
Ok(Some(Duration::parse(b"-838:59:59", 0).unwrap())),
false,
),
];

for (input, fsp, expect, overflow) in cs {
let mut ctx = CtxConfig {
overflow_as_warning: overflow,
..CtxConfig::default()
}
.into();
let rft = FieldTypeConfig {
decimal: fsp,
..FieldTypeConfig::default()
}
.into();
let extra = make_extra(&rft);

let result = cast_int_as_duration(&mut ctx, &extra, &Some(input));

// make log
let expect_str = match expect.as_ref() {
Ok(x) => format!("{:?}", x.map(|x| x.to_string())),
Err(e) => format!("{:?}", e),
};
let result_str = match result {
Ok(Some(x)) => x.to_string(),
_ => format!("{:?}", result),
};
let log = format!(
"input: {}, fsp: {}, expect: {}, result: {:?}",
input, fsp, expect_str, result_str
);

match expect {
Ok(expect) => {
check_result(expect.as_ref(), &result, log.as_str());
check_overflow(&ctx, overflow, log.as_str());
for (input, fsp, expected, overflow) in cs {
let (result, ctx) = RpnFnScalarEvaluator::new()
.context(CtxConfig {
overflow_as_warning: true,
..CtxConfig::default()
})
.push_param(input)
.evaluate_raw(
FieldTypeConfig {
tp: Some(FieldTypeTp::Duration),
decimal: fsp,
..FieldTypeConfig::default()
},
ScalarFuncSig::CastIntAsDuration,
);
match expected {
Ok(expected) => {
let result: Option<Duration> = result.unwrap().into();
assert_eq!(
result, expected,
"input:{:?}, expected:{:?}, got:{:?}",
input, expected, result,
);
}
Err(e) => {
assert!(result.is_err(), "log: {}, output_err: {}", log, e);
Err(_) => {
assert!(
result.is_err(),
"input:{:?}, expected err:{:?}, got:{:?}",
input,
expected,
result
);
}
}
if overflow {
assert_eq!(ctx.warnings.warning_cnt, 1);
assert_eq!(ctx.warnings.warnings[0].get_code(), ERR_DATA_OUT_OF_RANGE);
}
}
}

Expand Down

0 comments on commit 4aa395a

Please sign in to comment.