alamb commented on code in PR #9689:
URL: https://github.com/apache/arrow-datafusion/pull/9689#discussion_r1530656303
##########
datafusion/functions/src/datetime/to_char.rs:
##########
@@ -215,8 +219,19 @@ fn _to_char_array(args: &[ColumnarValue]) ->
Result<ColumnarValue> {
};
// this isn't ideal but this can't use ValueFormatter as it isn't
independent
// from ArrayFormatter
- let formatter = ArrayFormatter::try_new(arrays[0].as_ref(),
&format_options)?;
- let result = formatter.value(idx).try_to_string();
+ let result = match format {
+ Some(_) => {
Review Comment:
I think in this case we should also be formatting to None, as in change
```rust
let mut results: Vec<String> = vec![];
```
to
```rust
let mut results: Vec<Option<String>> = vec![];
```
so that it can properly represent nulls
##########
datafusion/functions/src/datetime/to_char.rs:
##########
@@ -215,8 +219,19 @@ fn _to_char_array(args: &[ColumnarValue]) ->
Result<ColumnarValue> {
};
// this isn't ideal but this can't use ValueFormatter as it isn't
independent
// from ArrayFormatter
- let formatter = ArrayFormatter::try_new(arrays[0].as_ref(),
&format_options)?;
- let result = formatter.value(idx).try_to_string();
+ let result = match format {
+ Some(_) => {
+ let formatter =
+ ArrayFormatter::try_new(arrays[0].as_ref(),
&format_options)?;
+ formatter.value(idx).try_to_string()
+ }
+ None => {
+ let null_array =
ColumnarValue::create_null_array(1).into_array(1)?;
+ let formatter =
+ ArrayFormatter::try_new(null_array.as_ref(),
&format_options)?;
+ formatter.value(0).try_to_string()
+ }
+ };
Review Comment:
Something else I noticed while looking at this code is that
```
ColumnarValue::Array(_) =>
Ok(ColumnarValue::Array(Arc::new(StringArray::from(
results,
)) as ArrayRef)),
```
Effectively means the strings are copied twice (once to `results` and then
again to the array).
As a follow on PR we could potentially use `StringBuilder` to build the
final string array directly rather than allocating a bunch of small strings.
##########
datafusion/functions/src/datetime/to_char.rs:
##########
@@ -171,7 +171,11 @@ fn _to_char_scalar(
// of the implementation in arrow-rs we need to convert it to an array
let data_type = &expression.data_type();
let is_scalar_expression = matches!(&expression, ColumnarValue::Scalar(_));
- let array = expression.into_array(1)?;
+ let array_from_expr = expression.into_array(1)?;
Review Comment:
While it is true that `ArrayFormatter::try_new` allows specifying the string
to use for null values, I don't think that functionality is exposed via
`to_char`
Thus I think this actually should simply return a new `StringArray` of all
null values (which confusingly is different than `NullArray`)
So in this case I think if the format is `None` the code should return a
null string value (namely `ColumnarValue::Scalar(ScalarValue::Utf8(None))`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
