-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Return null for date_trunc(null) instead of panic #6723
Conversation
query P | ||
SELECT DATE_TRUNC('year', NULL); | ||
---- | ||
1970-01-01T00:00:00 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -287,31 +287,31 @@ pub fn date_trunc(args: &[ColumnarValue]) -> Result<ColumnarValue> { | |||
"minute" => { | |||
// trunc to minute | |||
let second = ScalarValue::TimestampNanosecond( | |||
Some(nano.unwrap() / 1_000_000_000 * 1_000_000_000), | |||
Some(nano.unwrap_or_default() / 1_000_000_000 * 1_000_000_000), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently in date_trunc
, nano
is assumed to be Some
. To return null for null input instead of unwrap
on it, we can simply return a null timestamp scalar if nano
is None.
I know this push doesn't work, but I really don't get it. Now the unwrap() error is gone, but I get this error.
I've looked into ScalarValue to find a null timestamp, but it doesn't exist. I'm not sure if Null should work in this case or if TimestampNull needs to be implemented? Thank you for the help, but I will let some one else complete it. I don't think I have the skills to complete this issue. |
@@ -283,6 +283,11 @@ pub fn date_trunc(args: &[ColumnarValue]) -> Result<ColumnarValue> { | |||
Ok(match array { | |||
ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(v, tz_opt)) => { | |||
let nano = (f)(*v)?; | |||
|
|||
if nano.is_none() { | |||
return Ok(ColumnarValue::Scalar(ScalarValue::Null)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is because you use ScalarValue::Null
here.
You can take a look of enum ScalarValue
. ScalarValue::Null
is for DataType::Null
. That's why there is an error for a different schema
.
For other types, they can take a Some(value)
or a None
which represents a null value for the type.
For TimestampNanosecond
type, you just need to put ScalarValue::TimestampNanosecond
with None
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how TimestampNanosecond
is defined in enum ScalarValue
:
/// Timestamp Nanoseconds
TimestampNanosecond(Option<i64>, Option<Arc<str>>),
The first argument is Option<i64>
. If it is a None
, meaning that is a null TimestampNanosecond
scalar value.
You are pretty close to finish this. |
Just a format issue: if nano.is_none() {
- return Ok(ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(None, tz_opt.clone())));
+ return Ok(ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(
+ None,
+ tz_opt.clone(),
+ )));
} |
You can run cargo fmt locally or use it in your editor plugin so the code will be formatted automatically. |
Thank you for your help, I really appreciate it |
Thank you @BryanEmond |
Which issue does this PR close?
Closes #6701.
Rationale for this change
see #6701
What changes are included in this PR?
Returns None if scalar input to
date_trunc
is null value.Are these changes tested?
yes
Are there any user-facing changes?
Yes. Previously
date_trunc(null)
panics, now after this patch it returns null.