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.

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

fix(timestamp conversion): use timestamp_nanos_opt to avoid panics #979

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
1 change: 1 addition & 0 deletions changelog.d/979.fix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Replaced all usages of `timestamp_nanos` with `timestamp_nanos_opt` to avoid panics when the timestamp is out of range.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Replaced all usages of `timestamp_nanos` with `timestamp_nanos_opt` to avoid panics when the timestamp is out of range.
`to_unix_timestamp`, `to_float`, and `uuid_v7` can now return an error if the supplied timestamp is unrepresentable as a nanosecond timestamp. Previously the function calls would panic.

The changelogs should have the user in mind as the audience. I also think this is a breaking change, no? We'll want to document how to migrate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, marking this PR as a draft for now. I will try to find some propose a migration plan. I need to refresh my memory on the deprecation process as well.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We provide linters for such changes. In this way we have eliminated the deprecation of to_timestamp.
I think there is no problem in this case, because the vector during validation will report that to_unix_timestamp returns an error, and this is not handled

8 changes: 6 additions & 2 deletions src/compiler/value/error.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use super::Kind;
use crate::compiler::ExpressionError;
use crate::diagnostic::DiagnosticMessage;

use super::Kind;
use crate::prelude::ValueError::OutOfRange;

#[derive(thiserror::Error, Debug, PartialEq, Eq)]
pub enum ValueError {
Expand Down Expand Up @@ -55,6 +55,9 @@ pub enum ValueError {

#[error("can't merge type {1} into {0}")]
Merge(Kind, Kind),

#[error("can't convert {0}")]
OutOfRange(Kind),
}

impl DiagnosticMessage for ValueError {
Expand All @@ -81,6 +84,7 @@ impl DiagnosticMessage for ValueError {
Lt(..) => 313,
Le(..) => 314,
Merge(..) => 315,
OutOfRange(..) => 316,
}
}
}
Expand Down
10 changes: 7 additions & 3 deletions src/stdlib/to_float.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,13 @@ fn to_float(value: Value) -> Resolved {
Integer(v) => Ok(Value::from_f64_or_zero(v as f64)),
Boolean(v) => Ok(NotNan::new(if v { 1.0 } else { 0.0 }).unwrap().into()),
Null => Ok(NotNan::new(0.0).unwrap().into()),
Timestamp(v) => Ok(Value::from_f64_or_zero(
v.timestamp_nanos() as f64 / 1_000_000_000_f64,
)),
Timestamp(v) => {
let nanos = match v.timestamp_nanos_opt() {
Some(nanos) => nanos as f64,
None => return Err(ValueError::OutOfRange(Kind::timestamp()).into()),
};
Ok(Value::from_f64_or_zero(nanos / 1_000_000_000_f64))
}
Bytes(v) => Conversion::Float
.convert(v)
.map_err(|e| e.to_string().into()),
Expand Down
12 changes: 11 additions & 1 deletion src/stdlib/to_unix_timestamp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ fn to_unix_timestamp(value: Value, unit: Unit) -> Resolved {
Unit::Seconds => ts.timestamp(),
Unit::Milliseconds => ts.timestamp_millis(),
Unit::Microseconds => ts.timestamp_micros(),
Unit::Nanoseconds => ts.timestamp_nanos(),
Unit::Nanoseconds => match ts.timestamp_nanos_opt() {
None => return Err(ValueError::OutOfRange(Kind::timestamp()).into()),
Some(nanos) => nanos,
},
};
Ok(time.into())
}
Expand Down Expand Up @@ -186,5 +189,12 @@ mod test {
want: Ok(1_609_459_200_000_000_000_i64),
tdef: TypeDef::integer().infallible(),
}
crash {
args: func_args![value: chrono::Utc.ymd(0, 1, 1).and_hms_milli(0, 0, 0, 0),
unit: "nanoseconds"
],
want: Err("can't convert timestamp"),
tdef: TypeDef::integer().infallible(),
}
];
}
17 changes: 10 additions & 7 deletions src/stdlib/uuid_v7.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,25 @@ use bytes::Bytes;
use chrono::{DateTime, Utc};
use uuid::{timestamp::Timestamp, NoContext};

fn uuid_v7(timestamp: Option<Value>) -> Value {
let timestamp: DateTime<Utc> = if let Some(timestamp) = timestamp {
timestamp.try_timestamp().unwrap()
fn uuid_v7(timestamp: Option<Value>) -> Resolved {
let utc_timestamp: DateTime<Utc> = if let Some(timestamp) = timestamp {
timestamp.try_timestamp()?
} else {
Utc::now()
};

let seconds = timestamp.timestamp() as u64;
let nanoseconds = timestamp.timestamp_nanos() as u32;
let seconds = utc_timestamp.timestamp() as u64;
let nanoseconds = match utc_timestamp.timestamp_nanos_opt() {
Some(nanos) => nanos as u32,
None => return Err(ValueError::OutOfRange(Kind::timestamp()).into()),
};
let timestamp = Timestamp::from_unix(NoContext, seconds, nanoseconds);

let mut buffer = [0; 36];
let uuid = uuid::Uuid::new_v7(timestamp)
.hyphenated()
.encode_lower(&mut buffer);
Bytes::copy_from_slice(uuid.as_bytes()).into()
Ok(Bytes::copy_from_slice(uuid.as_bytes()).into())
}

#[derive(Clone, Copy, Debug)]
Expand Down Expand Up @@ -77,7 +80,7 @@ impl FunctionExpression for UuidV7Fn {
.map(|m| m.resolve(ctx))
.transpose()?;

Ok(uuid_v7(timestamp))
uuid_v7(timestamp)
}

fn type_def(&self, _: &TypeState) -> TypeDef {
Expand Down
Loading