-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: to_char Function Now Correctly Handles DATE Values in DataFusion #14970
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -212,14 +212,6 @@ fn _to_char_scalar( | |
let is_scalar_expression = matches!(&expression, ColumnarValue::Scalar(_)); | ||
let array = expression.into_array(1)?; | ||
|
||
// fix https://github.com/apache/datafusion/issues/14884 | ||
// If the input date/time is null, return a null Utf8 result. | ||
if array.is_null(0) { | ||
return Ok(match is_scalar_expression { | ||
true => ColumnarValue::Scalar(ScalarValue::Utf8(None)), | ||
false => ColumnarValue::Array(new_null_array(&Utf8, array.len())), | ||
}); | ||
} | ||
if format.is_none() { | ||
if is_scalar_expression { | ||
return Ok(ColumnarValue::Scalar(ScalarValue::Utf8(None))); | ||
|
@@ -234,15 +226,21 @@ fn _to_char_scalar( | |
}; | ||
|
||
let formatter = ArrayFormatter::try_new(array.as_ref(), &format_options)?; | ||
let formatted: Result<Vec<_>, ArrowError> = (0..array.len()) | ||
.map(|i| formatter.value(i).try_to_string()) | ||
let formatted: Result<Vec<Option<String>>, ArrowError> = (0..array.len()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
.map(|i| { | ||
if array.is_null(i) { | ||
Ok(None) | ||
} else { | ||
formatter.value(i).try_to_string().map(Some) | ||
} | ||
}) | ||
.collect(); | ||
|
||
if let Ok(formatted) = formatted { | ||
if is_scalar_expression { | ||
Ok(ColumnarValue::Scalar(ScalarValue::Utf8(Some( | ||
formatted.first().unwrap().to_string(), | ||
)))) | ||
Ok(ColumnarValue::Scalar(ScalarValue::Utf8( | ||
formatted.first().unwrap().clone(), | ||
))) | ||
} else { | ||
Ok(ColumnarValue::Array( | ||
Arc::new(StringArray::from(formatted)) as ArrayRef | ||
|
@@ -260,13 +258,6 @@ fn _to_char_array(args: &[ColumnarValue]) -> Result<ColumnarValue> { | |
let data_type = arrays[0].data_type(); | ||
|
||
for idx in 0..arrays[0].len() { | ||
// fix https://github.com/apache/datafusion/issues/14884 | ||
// If the date/time value is null, push None. | ||
if arrays[0].is_null(idx) { | ||
results.push(None); | ||
continue; | ||
} | ||
|
||
Comment on lines
-263
to
-269
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. undo #14908 |
||
let format = if format_array.is_null(idx) { | ||
None | ||
} else { | ||
|
@@ -678,31 +669,4 @@ mod tests { | |
"Execution error: Format for `to_char` must be non-null Utf8, received Timestamp(Nanosecond, None)" | ||
); | ||
} | ||
|
||
#[test] | ||
fn test_to_char_input_none_array() { | ||
let date_array = Arc::new(Date32Array::from(vec![Some(18506), None])) as ArrayRef; | ||
let format_array = | ||
StringArray::from(vec!["%Y-%m-%d".to_string(), "%Y-%m-%d".to_string()]); | ||
let args = datafusion_expr::ScalarFunctionArgs { | ||
args: vec![ | ||
ColumnarValue::Array(date_array), | ||
ColumnarValue::Array(Arc::new(format_array) as ArrayRef), | ||
], | ||
number_rows: 2, | ||
return_type: &DataType::Utf8, | ||
}; | ||
let result = ToCharFunc::new() | ||
.invoke_with_args(args) | ||
.expect("Expected no error"); | ||
if let ColumnarValue::Array(result) = result { | ||
let result = result.as_any().downcast_ref::<StringArray>().unwrap(); | ||
assert_eq!(result.len(), 2); | ||
// The first element is valid, second is null. | ||
assert!(!result.is_null(0)); | ||
assert!(result.is_null(1)); | ||
} else { | ||
panic!("Expected an array value"); | ||
} | ||
} | ||
Comment on lines
-681
to
-707
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove test which was added in #14908 |
||
} |
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.
Undo #14908