Skip to content

fix: cast literal to timestamp #5517

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

Merged
alamb merged 5 commits into from
Mar 13, 2023
Merged

fix: cast literal to timestamp #5517

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3d4db81
fix: cast literal to timestamp
Weijun-H Mar 8, 2023
ff9d795
update tests for all transformation
Weijun-H Mar 9, 2023
4f95a55
handle cast between same type
Weijun-H Mar 9, 2023
2ca12a3
refactor cast_between_timestamp to avoid overflow
Weijun-H Mar 9, 2023
89ea816
handle overflow to None
Weijun-H Mar 10, 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
211 changes: 207 additions & 4 deletions datafusion/optimizer/src/unwrap_cast_in_comparison.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,16 +400,40 @@ fn try_cast_literal_to_type(
DataType::UInt32 => ScalarValue::UInt32(Some(value as u32)),
DataType::UInt64 => ScalarValue::UInt64(Some(value as u64)),
DataType::Timestamp(TimeUnit::Second, tz) => {
ScalarValue::TimestampSecond(Some(value as i64), tz.clone())
let value = cast_between_timestamp(
lit_data_type,
DataType::Timestamp(TimeUnit::Second, tz.clone()),
value,
)
.unwrap();
ScalarValue::TimestampSecond(Some(value), tz.clone())
}
DataType::Timestamp(TimeUnit::Millisecond, tz) => {
ScalarValue::TimestampMillisecond(Some(value as i64), tz.clone())
let value = cast_between_timestamp(
lit_data_type,
DataType::Timestamp(TimeUnit::Millisecond, tz.clone()),
value,
)
.unwrap();
ScalarValue::TimestampMillisecond(Some(value), tz.clone())
}
DataType::Timestamp(TimeUnit::Microsecond, tz) => {
ScalarValue::TimestampMicrosecond(Some(value as i64), tz.clone())
let value = cast_between_timestamp(
lit_data_type,
DataType::Timestamp(TimeUnit::Microsecond, tz.clone()),
value,
)
.unwrap();
ScalarValue::TimestampMicrosecond(Some(value), tz.clone())
}
DataType::Timestamp(TimeUnit::Nanosecond, tz) => {
ScalarValue::TimestampNanosecond(Some(value as i64), tz.clone())
let value = cast_between_timestamp(
lit_data_type,
DataType::Timestamp(TimeUnit::Nanosecond, tz.clone()),
value,
)
.unwrap();
ScalarValue::TimestampNanosecond(Some(value), tz.clone())
}
DataType::Decimal128(p, s) => {
ScalarValue::Decimal128(Some(value), *p, *s)
Expand All @@ -428,6 +452,36 @@ fn try_cast_literal_to_type(
}
}

/// Cast a timestamp value from one unit to another
fn cast_between_timestamp(from: DataType, to: DataType, value: i128) -> Option<i64> {

@tustvold tustvold Mar 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this has problematic overflow behaviour, I wonder if we can determine the scale factor and multiply or divide by this instead? This is what arrow-cast does - https://github.com/apache/arrow-rs/blob/master/arrow-cast/src/cast.rs#L1671

Currently I think converting i64::MAX / 1_000 to milliseconds from seconds will silently overflow, when in reality it shouldn't

// avoid useless computation, like cast from second to second
if from == to {
return Some(value as i64);
}

let seconds = match from {

@comphead comphead Mar 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it can be improved. if the cast Second to Second, then the code will do unneccessary mul by 1m and then div by 1m

DataType::Timestamp(TimeUnit::Second, _) => Some(value * 1000 * 1000 * 1000),
DataType::Timestamp(TimeUnit::Millisecond, _) => Some(value * 1000 * 1000),
DataType::Timestamp(TimeUnit::Microsecond, _) => Some(value * 1000),
DataType::Timestamp(TimeUnit::Nanosecond, _) => Some(value),
_ => return Some(value as i64),
};

match to {
DataType::Timestamp(TimeUnit::Second, _) => {
seconds.map(|s| (s / 1000 / 1000 / 1000) as i64)
}
DataType::Timestamp(TimeUnit::Millisecond, _) => {
seconds.map(|s| (s / 1000 / 1000) as i64)
}
DataType::Timestamp(TimeUnit::Microsecond, _) => {
seconds.map(|s| (s / 1000) as i64)
}
DataType::Timestamp(TimeUnit::Nanosecond, _) => seconds.map(|s| s as i64),
_ => None,
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -1070,4 +1124,153 @@ mod tests {
}
}
}

#[test]
fn test_try_cast_literal_to_timestamp() {

@comphead comphead Mar 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please add more tests between timestamps

// same timestamp
let new_scalar = try_cast_literal_to_type(
&ScalarValue::TimestampNanosecond(Some(123456), None),
&DataType::Timestamp(TimeUnit::Nanosecond, None),
)
.unwrap()
.unwrap();

assert_eq!(
new_scalar,
ScalarValue::TimestampNanosecond(Some(123456), None)
);

// TimestampNanosecond to TimestampMicrosecond
let new_scalar = try_cast_literal_to_type(
&ScalarValue::TimestampNanosecond(Some(123456), None),
&DataType::Timestamp(TimeUnit::Microsecond, None),
)
.unwrap()
.unwrap();

assert_eq!(
new_scalar,
ScalarValue::TimestampMicrosecond(Some(123), None)
);

// TimestampNanosecond to TimestampMillisecond
let new_scalar = try_cast_literal_to_type(
&ScalarValue::TimestampNanosecond(Some(123456), None),
&DataType::Timestamp(TimeUnit::Millisecond, None),
)
.unwrap()
.unwrap();

assert_eq!(new_scalar, ScalarValue::TimestampMillisecond(Some(0), None));

// TimestampNanosecond to TimestampSecond
let new_scalar = try_cast_literal_to_type(
&ScalarValue::TimestampNanosecond(Some(123456), None),
&DataType::Timestamp(TimeUnit::Second, None),
)
.unwrap()
.unwrap();

assert_eq!(new_scalar, ScalarValue::TimestampSecond(Some(0), None));

// TimestampMicrosecond to TimestampNanosecond
let new_scalar = try_cast_literal_to_type(
&ScalarValue::TimestampMicrosecond(Some(123), None),
&DataType::Timestamp(TimeUnit::Nanosecond, None),
)
.unwrap()
.unwrap();

assert_eq!(
new_scalar,
ScalarValue::TimestampNanosecond(Some(123000), None)
);

// TimestampMicrosecond to TimestampMillisecond
let new_scalar = try_cast_literal_to_type(
&ScalarValue::TimestampMicrosecond(Some(123), None),
&DataType::Timestamp(TimeUnit::Millisecond, None),
)
.unwrap()
.unwrap();

assert_eq!(new_scalar, ScalarValue::TimestampMillisecond(Some(0), None));

// TimestampMicrosecond to TimestampSecond
let new_scalar = try_cast_literal_to_type(
&ScalarValue::TimestampMicrosecond(Some(123456789), None),
&DataType::Timestamp(TimeUnit::Second, None),
)
.unwrap()
.unwrap();
assert_eq!(new_scalar, ScalarValue::TimestampSecond(Some(123), None));

// TimestampMillisecond to TimestampNanosecond
let new_scalar = try_cast_literal_to_type(
&ScalarValue::TimestampMillisecond(Some(123), None),
&DataType::Timestamp(TimeUnit::Nanosecond, None),
)
.unwrap()
.unwrap();
assert_eq!(
new_scalar,
ScalarValue::TimestampNanosecond(Some(123000000), None)
);

// TimestampMillisecond to TimestampMicrosecond
let new_scalar = try_cast_literal_to_type(
&ScalarValue::TimestampMillisecond(Some(123), None),
&DataType::Timestamp(TimeUnit::Microsecond, None),
)
.unwrap()
.unwrap();
assert_eq!(
new_scalar,
ScalarValue::TimestampMicrosecond(Some(123000), None)
);
// TimestampMillisecond to TimestampSecond
let new_scalar = try_cast_literal_to_type(
&ScalarValue::TimestampMillisecond(Some(123456789), None),
&DataType::Timestamp(TimeUnit::Second, None),
)
.unwrap()
.unwrap();
assert_eq!(new_scalar, ScalarValue::TimestampSecond(Some(123456), None));

// TimestampSecond to TimestampNanosecond
let new_scalar = try_cast_literal_to_type(
&ScalarValue::TimestampSecond(Some(123), None),
&DataType::Timestamp(TimeUnit::Nanosecond, None),
)
.unwrap()
.unwrap();
assert_eq!(
new_scalar,
ScalarValue::TimestampNanosecond(Some(123000000000), None)
);

// TimestampSecond to TimestampMicrosecond
let new_scalar = try_cast_literal_to_type(
&ScalarValue::TimestampSecond(Some(123), None),
&DataType::Timestamp(TimeUnit::Microsecond, None),
)
.unwrap()
.unwrap();
assert_eq!(
new_scalar,
ScalarValue::TimestampMicrosecond(Some(123000000), None)
);

// TimestampSecond to TimestampMillisecond
let new_scalar = try_cast_literal_to_type(
&ScalarValue::TimestampSecond(Some(123), None),
&DataType::Timestamp(TimeUnit::Millisecond, None),
)
.unwrap()
.unwrap();
assert_eq!(
new_scalar,
ScalarValue::TimestampMillisecond(Some(123000), None)
);
}
}