Fix: to_char Function Now Correctly Handles DATE Values in DataFusion #14970
+31
−47
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kosiew
commented
Mar 3, 2025
Comment on lines
-681
to
-707
|
|
||
| #[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"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove test which was added in #14908
kosiew
commented
Mar 3, 2025
Comment on lines
-215
to
-222
| // 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())), | ||
| }); | ||
| } |
kosiew
commented
Mar 3, 2025
Comment on lines
-263
to
-269
| // 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; | ||
| } | ||
|
|
12 tasks
alamb
approved these changes
Mar 4, 2025
| 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()) |
|
Thanks again @kosiew |
|
Thank you @alamb for the review. |
danila-b
pushed a commit
to danila-b/datafusion
that referenced
this pull request
Mar 8, 2025
…apache#14970) * revert _to_char_scalar to before apache#14908 * Amend formatted loop - handle null * revert _to_char_array to before apache#14908 * add slt test
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
After #14908, the
to_charfunction was incorrectly returningNULLfor validDATEvalues due to improper handling of theDATEtype. This fix ensures thatto_charcorrectly formats non-nullDATEvalues while preservingNULLvalues as expected.This is a re-implementation of #14908
What changes are included in this PR?
to_charfunction to properly processDATEvalues.DATEvalues.Are these changes tested?
Yes, new test cases have been added to confirm that:
to_charcorrectly formats validDATEvalues.NULLvalues remain unaffected.Are there any user-facing changes?
No breaking changes. Users will now see correctly formatted
DATEvalues when usingto_char, instead of unexpectedNULLresults.