Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(common): interval input/output overflow and negative handling #8613

Merged
merged 7 commits into from
Mar 17, 2023
Merged

fix(common): interval input/output overflow and negative handling #8613

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bb2e74b
output + after -
xiangjinwu Mar 16, 2023
2a05ee2
input allows +
xiangjinwu Mar 16, 2023
36a63b0
handle overflow during parsing
xiangjinwu Mar 16, 2023
9af08af
fix parsing add overflow panic
xiangjinwu Mar 17, 2023
37b54c9
parse negative hms
xiangjinwu Mar 17, 2023
0bc849c
fix output i64::MIN abs panic
xiangjinwu Mar 17, 2023
12468f9
resolve review comments
xiangjinwu Mar 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 31 additions & 39 deletions e2e_test/batch/types/interval.slt.part
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,43 +120,35 @@ select '1 mons 1 days 00:00:00.0000001'::INTERVAL;
----
1 mon 1 day

# Tests moved from regress tests due to not matching exactly.
# parsing large values

query T
select '2562047788:00:54.775807'::interval;
----
2562047788:00:54.775807

statement error
select '2562047788:00:54.775808'::interval;

query T
select '4 years 2147483599 mon'::interval;
----
178956970 years 7 mons

statement error
select '4 years 2147483600 mon'::interval;

# In mixed sign intervals, PostgreSQL displays positive sign after negative
# (e.g. `-1 mons +1 day`) while we display it without sign
# (e.g. `-1 mons 1 day`).
# But the main purpose of this test case is ordering of large values.

statement ok
CREATE TABLE INTERVAL_TBL_OF (f1 interval);

statement ok
INSERT INTO INTERVAL_TBL_OF (f1) VALUES
('2147483647 days 2147483647 months'),
('2147483647 days -2147483648 months'),
('1 year'),
('-2147483648 days 2147483647 months'),
('-2147483648 days -2147483648 months');

statement ok
FLUSH;

query TT
SELECT r1.*, r2.*
FROM INTERVAL_TBL_OF r1, INTERVAL_TBL_OF r2
WHERE r1.f1 > r2.f1
ORDER BY r1.f1, r2.f1;
----
-178956970 years -8 mons 2147483647 days -178956970 years -8 mons -2147483648 days
1 year -178956970 years -8 mons -2147483648 days
1 year -178956970 years -8 mons 2147483647 days
178956970 years 7 mons -2147483648 days -178956970 years -8 mons -2147483648 days
178956970 years 7 mons -2147483648 days -178956970 years -8 mons 2147483647 days
178956970 years 7 mons -2147483648 days 1 year
178956970 years 7 mons 2147483647 days -178956970 years -8 mons -2147483648 days
178956970 years 7 mons 2147483647 days -178956970 years -8 mons 2147483647 days
178956970 years 7 mons 2147483647 days 1 year
178956970 years 7 mons 2147483647 days 178956970 years 7 mons -2147483648 days

statement ok
DROP TABLE INTERVAL_TBL_OF;
query T
select '-2562047788:00:54.775807'::interval;
----
-2562047788:00:54.775807

query T
select '-2562047788:00:54.775808'::interval;
----
-2562047788:00:54.775808

statement error
select '-2562047788:00:54.775809'::interval;

# Tests moved from regress tests due to not matching exactly.
xiangjinwu marked this conversation as resolved.
Show resolved Hide resolved
137 changes: 82 additions & 55 deletions src/common/src/types/interval.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,42 +813,45 @@ impl Display for IntervalUnit {
let months = self.months % 12;
let days = self.days;
let mut space = false;
let mut write = |arg: std::fmt::Arguments<'_>| {
let mut need_pos = false;
let mut write_i32 = |arg: i32, unit: &str| -> std::fmt::Result {
if arg == 0 {
return Ok(());
}
if space {
write!(f, " ")?;
}
write!(f, "{arg}")?;
if need_pos && arg > 0 {
write!(f, "+")?;
}
write!(f, "{arg} {unit}")?;
if arg != 1 {
write!(f, "s")?;
}
space = true;
need_pos = arg < 0;
kwannoel marked this conversation as resolved.
Show resolved Hide resolved
Ok(())
};
if years == 1 {
write(format_args!("{years} year"))?;
} else if years != 0 {
write(format_args!("{years} years"))?;
}
if months == 1 {
write(format_args!("{months} mon"))?;
} else if months != 0 {
write(format_args!("{months} mons"))?;
}
if days == 1 {
write(format_args!("{days} day"))?;
} else if days != 0 {
write(format_args!("{days} days"))?;
}
write_i32(years, "year")?;
write_i32(months, "mon")?;
write_i32(days, "day")?;
if self.usecs != 0 || self.months == 0 && self.days == 0 {
let usecs = self.usecs.abs();
let ms = usecs / 1000;
let hours = ms / 1000 / 3600;
let minutes = (ms / 1000 / 60) % 60;
let seconds = ms % 60000 / 1000;
let secs_fract = usecs % USECS_PER_SEC;

if self.usecs < 0 {
write(format_args!("-{hours:0>2}:{minutes:0>2}:{seconds:0>2}"))?;
} else {
write(format_args!("{hours:0>2}:{minutes:0>2}:{seconds:0>2}"))?;
// `abs` on `self.usecs == i64::MIN` would overflow, so we divide first then abs
let secs_fract = (self.usecs % USECS_PER_SEC).abs();
let total_secs = (self.usecs / USECS_PER_SEC).abs();
let hours = total_secs / 3600;
let minutes = (total_secs / 60) % 60;
let seconds = total_secs % 60;

if space {
write!(f, " ")?;
}
if need_pos && self.usecs > 0 {
write!(f, "+")?;
} else if self.usecs < 0 {
write!(f, "-")?;
}
write!(f, "{hours:0>2}:{minutes:0>2}:{seconds:0>2}")?;
if secs_fract != 0 {
let mut buf = [0u8; 7];
write!(buf.as_mut_slice(), ".{:06}", secs_fract).unwrap();
Expand Down Expand Up @@ -953,7 +956,7 @@ fn parse_interval(s: &str) -> Result<Vec<TimeStrToken>> {
let mut hour_min_sec = Vec::new();
for (i, c) in s.chars().enumerate() {
match c {
'-' => {
'-' | '+' => {
num_buf.push(c);
}
'.' => {
Expand Down Expand Up @@ -1001,7 +1004,9 @@ fn parse_interval(s: &str) -> Result<Vec<TimeStrToken>> {
convert_digit(&mut num_buf, &mut tokens)?;
}
convert_unit(&mut char_buf, &mut tokens)?;
convert_hms(&mut hour_min_sec, &mut tokens)?;
convert_hms(&mut hour_min_sec, &mut tokens).ok_or_else(|| {
ErrorCode::InvalidInputSyntax(format!("Invalid interval: {:?}", hour_min_sec))
})?;

Ok(tokens)
}
Expand Down Expand Up @@ -1037,34 +1042,46 @@ fn convert_unit(c: &mut String, t: &mut Vec<TimeStrToken>) -> Result<()> {
/// [`TimeStrToken::Num(1)`, `TimeStrToken::TimeUnit(DateTimeField::Hour)`,
/// `TimeStrToken::Num(2)`, `TimeStrToken::TimeUnit(DateTimeField::Minute)`,
/// `TimeStrToken::Second("3")`, `TimeStrToken::TimeUnit(DateTimeField::Second)`]
fn convert_hms(c: &mut Vec<String>, t: &mut Vec<TimeStrToken>) -> Result<()> {
fn convert_hms(c: &mut Vec<String>, t: &mut Vec<TimeStrToken>) -> Option<()> {
if c.len() > 3 {
return Err(ErrorCode::InvalidInputSyntax(format!("Invalid interval: {:?}", c)).into());
return None;
}
let mut is_neg = false;
for (i, s) in c.iter().enumerate() {
match i {
0 => {
xiangjinwu marked this conversation as resolved.
Show resolved Hide resolved
t.push(TimeStrToken::Num(s.parse().map_err(|_| {
ErrorCode::InternalError(format!("Invalid interval: {}", c[0]))
})?));
let v = s.parse().ok()?;
is_neg = v < 0;
t.push(TimeStrToken::Num(v));
t.push(TimeStrToken::TimeUnit(DateTimeField::Hour))
}
1 => {
xiangjinwu marked this conversation as resolved.
Show resolved Hide resolved
t.push(TimeStrToken::Num(s.parse().map_err(|_| {
ErrorCode::InternalError(format!("Invalid interval: {}", c[0]))
})?));
let mut v: i64 = s.parse().ok()?;
if !(0..60).contains(&v) {
kwannoel marked this conversation as resolved.
Show resolved Hide resolved
return None;
}
if is_neg {
v = v.checked_neg()?;
}
t.push(TimeStrToken::Num(v));
t.push(TimeStrToken::TimeUnit(DateTimeField::Minute))
}
2 => {
xiangjinwu marked this conversation as resolved.
Show resolved Hide resolved
t.push(TimeStrToken::Second(s.parse().map_err(|_| {
ErrorCode::InternalError(format!("Invalid interval: {}", c[0]))
})?));
let mut v: OrderedF64 = s.parse().ok()?;
// PostgreSQL allows '60.x' for seconds.
if !(0f64 <= *v && *v < 61f64) {
return None;
}
if is_neg {
v = v.checked_neg()?;
}
t.push(TimeStrToken::Second(v));
t.push(TimeStrToken::TimeUnit(DateTimeField::Second))
}
_ => unreachable!(),
}
}
Ok(())
Some(())
}

impl IntervalUnit {
Expand Down Expand Up @@ -1092,11 +1109,19 @@ impl IntervalUnit {

(|| match leading_field {
Year => {
let months = num.checked_mul(12)?;
Some(IntervalUnit::from_month_day_usec(months as i32, 0, 0))
let months = num.checked_mul(12)?.try_into().ok()?;
Some(IntervalUnit::from_month_day_usec(months, 0, 0))
}
Month => Some(IntervalUnit::from_month_day_usec(num as i32, 0, 0)),
Day => Some(IntervalUnit::from_month_day_usec(0, num as i32, 0)),
Month => Some(IntervalUnit::from_month_day_usec(
num.try_into().ok()?,
0,
0,
)),
Day => Some(IntervalUnit::from_month_day_usec(
0,
num.try_into().ok()?,
0,
)),
Hour => {
let usecs = num.checked_mul(3600 * USECS_PER_SEC)?;
Some(IntervalUnit::from_month_day_usec(0, 0, usecs))
Expand Down Expand Up @@ -1127,13 +1152,13 @@ impl IntervalUnit {
while let Some(num) = token_iter.next() && let Some(interval_unit) = token_iter.next() {
match (num, interval_unit) {
(TimeStrToken::Num(num), TimeStrToken::TimeUnit(interval_unit)) => {
result = result + (|| match interval_unit {
result = (|| match interval_unit {
Year => {
let months = num.checked_mul(12)?;
Some(IntervalUnit::from_month_day_usec(months as i32, 0, 0))
let months = num.checked_mul(12)?.try_into().ok()?;
Some(IntervalUnit::from_month_day_usec(months, 0, 0))
}
Month => Some(IntervalUnit::from_month_day_usec(num as i32, 0, 0)),
Day => Some(IntervalUnit::from_month_day_usec(0, num as i32, 0)),
Month => Some(IntervalUnit::from_month_day_usec(num.try_into().ok()?, 0, 0)),
Day => Some(IntervalUnit::from_month_day_usec(0, num.try_into().ok()?, 0)),
Hour => {
let usecs = num.checked_mul(3600 * USECS_PER_SEC)?;
Some(IntervalUnit::from_month_day_usec(0, 0, usecs))
Expand All @@ -1147,17 +1172,19 @@ impl IntervalUnit {
Some(IntervalUnit::from_month_day_usec(0, 0, usecs))
}
})()
.and_then(|rhs| result.checked_add(&rhs))
.ok_or_else(|| ErrorCode::InvalidInputSyntax(format!("Invalid interval {}.", s)))?;
}
(TimeStrToken::Second(second), TimeStrToken::TimeUnit(interval_unit)) => {
result = result + match interval_unit {
result = match interval_unit {
Second => {
// If unsatisfied precision is passed as input, we should not return None (Error).
let usecs = (second.into_inner() * (USECS_PER_SEC as f64)).round() as i64;
Some(IntervalUnit::from_month_day_usec(0, 0, usecs))
}
_ => None,
}
.and_then(|rhs| result.checked_add(&rhs))
.ok_or_else(|| ErrorCode::InvalidInputSyntax(format!("Invalid interval {}.", s)))?;
}
_ => {
Expand Down Expand Up @@ -1275,11 +1302,11 @@ mod tests {
(11 * 3600 + 45 * 60 + 14) * USECS_PER_SEC + 233
)
.to_string(),
"-1 years -2 mons 3 days 11:45:14.000233"
"-1 years -2 mons +3 days 11:45:14.000233"
);
assert_eq!(
IntervalUnit::from_month_day_usec(-14, 3, 0).to_string(),
"-1 years -2 mons 3 days"
"-1 years -2 mons +3 days"
);
assert_eq!(IntervalUnit::default().to_string(), "00:00:00");
assert_eq!(
Expand All @@ -1289,7 +1316,7 @@ mod tests {
-((11 * 3600 + 45 * 60 + 14) * USECS_PER_SEC + 233)
)
.to_string(),
"-1 years -2 mons 3 days -11:45:14.000233"
"-1 years -2 mons +3 days -11:45:14.000233"
);
}

Expand Down
Loading