improve null handling for to_char #9689

tinfoil-knight · 2024-03-19T10:20:52Z

Which issue does this PR close?

Closes #9217.

Rationale for this change

Before this PR: On using null format with to_char, DataFusion would return the original data.

select to_char('2024-01-01 06:00:00'::timestamp, null);

> datafusion-cli

+-------------------------------------------+
| to_char(Utf8("2024-01-01 06:00:00"),NULL) |
+-------------------------------------------+
| 2024-01-01T06:00:00Z                      |
+-------------------------------------------+
1 row in set. Query took 0.021 seconds.

This is different from what Postgres does which is to return null.

> psql

to_char
---------

(1 row)

This PR intends to make the behaviour for Datafusion the same as Postgres for handling null format in to_char. So now, we''ll get null output too when the specified format is null.

> datafusion-cli

+-------------------------------------------+
| to_char(Utf8("2024-01-01 06:00:00"),NULL) |
+-------------------------------------------+
|                                           |
+-------------------------------------------+
1 row in set. Query took 0.022 seconds.

What changes are included in this PR?

We're passing null instead of the original data when the format is None.

Are these changes tested?

Existing tests have been updated.
1 new test has been added.

Are there any user-facing changes?

Yes. Earlier to_char(timestamp, null) would result in getting the original value back. Now it results in a null string.

tinfoil-knight · 2024-03-19T10:21:58Z

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)?;


Note for Reviewer:
We can't directly return null when we see that the format is None because format_options (passed to ArrayFormatter::try_new) allow specifying the string to show for null & we need to respect this configuration option.

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)))

Implemented this change. Slight difference in the type though. Please see #9689 (comment).

tinfoil-knight · 2024-03-19T10:54:09Z

@Omega359 tagging you since you added the to_char implementations.

Omega359 · 2024-03-19T13:00:11Z

I'll do a full review of this tonight - at first look it looks good though.

alamb

Thanks @tinfoil-knight -- this is looking like a good start

Thank you @Omega359 for offering to review as well

alamb · 2024-03-19T15:49:50Z

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)?;


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)))

alamb · 2024-03-19T15:51:23Z

datafusion/functions/src/datetime/to_char.rs

-        let formatter = ArrayFormatter::try_new(arrays[0].as_ref(), &format_options)?;
-        let result = formatter.value(idx).try_to_string();
+        let result = match format {
+            Some(_) => {


I think in this case we should also be formatting to None, as in change

let mut results: Vec<String> = vec![];

to

let mut results: Vec<Option<String>> = vec![];

so that it can properly represent nulls

Continuing to use the Vec<String> type. Reason is same as #9689 (comment) .

Updated:
Type has been changed to Vec<Option<String>> now.

alamb · 2024-03-19T15:53:11Z

datafusion/functions/src/datetime/to_char.rs

+                    ArrayFormatter::try_new(null_array.as_ref(), &format_options)?;
+                formatter.value(0).try_to_string()
+            }
+        };


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.

For someone whose role is to find ways to optimize things I seem to be rather poor at it sometimes :) Nice catch.

Well there are different degrees of optimization for sure ;)

Omega359 · 2024-03-20T00:54:06Z

The only additions I have beyond what @alamb covered is that we probably want to update the comment in invoke

// null format, use default formats

Since that wouldn't be true anymore.

In addition it would be nice to have the tests in to_char.rs updated to include a test that passes in None for the format and expecting a None result.

tinfoil-knight · 2024-03-20T18:15:30Z

datafusion/functions/src/datetime/to_char.rs

+    if format.is_none() {
+        if is_scalar_expression {
+            return Ok(ColumnarValue::Scalar(ScalarValue::Utf8(
+                Some(String::new()),


Using None instead of Some(String::new()) was causing NULL to appear instead of empty in SQL Logic Tests. (Output was same for CLI though.)

[SQL] select to_char(arrow_cast(123456, 'Duration(Second)'), null); [Diff] (-expected|+actual) - (empty) + NULL at test_files/timestamps.slt:2661

I don't think this is what we want. (empty) is the expected value.

I thought None is the correct value (as it will semantically be aNULL, which is the correct results)

The sqllogictests have special formatting for NULL values (to distinguish them from empty strings)

https://github.com/apache/arrow-datafusion/blob/1d8a41bc8e08b56e90d6f8e6ef20e39a126987e4/datafusion/sqllogictest/src/engines/datafusion_engine/normalize.rs#L198-L200

However, when I double checked, the behavior of to_date in spark seems to be different yet (passing in a null format seems to simply ignore the format string and instead parses with the default -- it doesn't return null)

>>> df = spark.createDataFrame([('1997-02-28 10:30:00',)], ['t']) >>> df.select(functions.to_date(df.t, 'yyyy-MM-dd HH:mm:ss').alias('date')).collect() [Row(date=datetime.date(1997, 2, 28))] >>> df.select(functions.to_date(df.t, None).alias('date')).collect() [Row(date=datetime.date(1997, 2, 28))]

>>> df = spark.createDataFrame([('1997-02-2ddddd',)], ['t']) >>> df.select(functions.to_date(df.t, None).alias('date')).collect() [Row(date=None)] >>> df.select(functions.to_date(df.t, 'yyyy-MM-dd HH:mm:ss').alias('date')).collect() [Row(date=None)]

Any hints @Omega359 ?

datafusion/functions/src/datetime/to_char.rs

+            )));
+        } else {
+            return Ok(ColumnarValue::Array(Arc::new(StringArray::from(
+                vec![String::new(); array.len()],


alamb

Thanks @tinfoil-knight

alamb · 2024-03-21T16:44:36Z

datafusion/functions/src/datetime/to_char.rs

+    if format.is_none() {
+        if is_scalar_expression {
+            return Ok(ColumnarValue::Scalar(ScalarValue::Utf8(
+                Some(String::new()),


I thought None is the correct value (as it will semantically be aNULL, which is the correct results)

The sqllogictests have special formatting for NULL values (to distinguish them from empty strings)

https://github.com/apache/arrow-datafusion/blob/1d8a41bc8e08b56e90d6f8e6ef20e39a126987e4/datafusion/sqllogictest/src/engines/datafusion_engine/normalize.rs#L198-L200

However, when I double checked, the behavior of to_date in spark seems to be different yet (passing in a null format seems to simply ignore the format string and instead parses with the default -- it doesn't return null)

>>> df = spark.createDataFrame([('1997-02-28 10:30:00',)], ['t']) >>> df.select(functions.to_date(df.t, 'yyyy-MM-dd HH:mm:ss').alias('date')).collect() [Row(date=datetime.date(1997, 2, 28))] >>> df.select(functions.to_date(df.t, None).alias('date')).collect() [Row(date=datetime.date(1997, 2, 28))]

>>> df = spark.createDataFrame([('1997-02-2ddddd',)], ['t']) >>> df.select(functions.to_date(df.t, None).alias('date')).collect() [Row(date=None)] >>> df.select(functions.to_date(df.t, 'yyyy-MM-dd HH:mm:ss').alias('date')).collect() [Row(date=None)]

Any hints @Omega359 ?

Omega359 · 2024-03-21T18:14:26Z

I was targeting postgres with this as best as possible vs spark though null/empty testing was unfortunately not a big focus. I think the previous behavior (use default) was the spark behaviour - though that wasn't on purpose on my part.

Postgresql:

SELECT to_char('1997-02-28 10:30:00'::TIMESTAMP, null);
+-------+
|to_char|
+-------+
|null   |
+-------+

I knew about the behaviour difference with spark but it's not something that I can see can be fixed without a flag or dedicated functions per dialect. It actually affects me directly which I'll handle with preprocessing the formats to strip null/empty formats.

alamb · 2024-03-21T18:57:50Z

I was targeting postgres with this as best as possible vs spark though null/empty testing was unfortunately not a big focus. I think the previous behavior (use default) was the spark behaviour - though that wasn't on purpose on my part.

That makes sense -- thank you

So in that case I do agree we should return None (aka NULL) when the format string is NULL.

tinfoil-knight · 2024-03-23T12:06:04Z

I've made the change. We're returning None instead of empty strings now.

alamb

Looks good to me -- thank you @tinfoil-knight

alamb · 2024-03-23T19:24:59Z

datafusion/functions/src/datetime/to_char.rs

+            return Ok(ColumnarValue::Scalar(ScalarValue::Utf8(None)));
+        } else {
+            let result: Vec<Option<String>> = vec![None; array.len()];
+            return Ok(ColumnarValue::Array(Arc::new(StringArray::from(result))));


The performance of this likely doesn't matter, but you can do the same thing more efficiently via new_null_array:

https://docs.rs/arrow/latest/arrow/array/fn.new_null_array.html

alamb

Thanks again @tinfoil-knight -- this PR now makes the function compatible with postgres (though it does something different than spark)

I think this is inevitable until we have some notion of "spark compatible" function packages e.g. #5600

improve null handling for to_char

30d9bf5

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Mar 19, 2024

tinfoil-knight commented Mar 19, 2024

View reviewed changes

alamb reviewed Mar 19, 2024

View reviewed changes

alamb mentioned this pull request Mar 19, 2024

DataFusion weekly project plan (Andrew Lamb) - March 18, 2024 #9675

Closed

7 tasks

early return from to_char for null format

b2789c2

tinfoil-knight commented Mar 20, 2024

View reviewed changes

datafusion/functions/src/datetime/to_char.rs Outdated

)));

} else {

return Ok(ColumnarValue::Array(Arc::new(StringArray::from(

vec![String::new(); array.len()],

This comment was marked as outdated.

Sign in to view

tinfoil-knight added 2 commits March 21, 2024 00:05

remove invalid comment, update example

e505d6c

rename column for consistency across platforms for tests

a3ad030

This comment was marked as outdated.

Sign in to view

tinfoil-knight requested a review from Omega359 March 21, 2024 13:08

alamb reviewed Mar 21, 2024

View reviewed changes

return None instead of empty string from to_char

5156731

tinfoil-knight requested a review from alamb March 23, 2024 12:06

alamb approved these changes Mar 23, 2024

View reviewed changes

tinfoil-knight added 2 commits March 24, 2024 02:00

use arrow:new_null_array for fast init

7c3d258

merge main and fix conflict

1c5c68c

tinfoil-knight requested a review from alamb March 23, 2024 21:36

alamb approved these changes Mar 24, 2024

View reviewed changes

alamb merged commit 227d1f8 into apache:main Mar 24, 2024
23 checks passed

tinfoil-knight deleted the 9217-to_char-null branch March 24, 2024 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve null handling for to_char #9689

improve null handling for to_char #9689

tinfoil-knight commented Mar 19, 2024 •

edited

Loading

tinfoil-knight Mar 19, 2024

alamb Mar 19, 2024

tinfoil-knight Mar 20, 2024 •

edited

Loading

tinfoil-knight commented Mar 19, 2024 •

edited

Loading

Omega359 commented Mar 19, 2024

alamb left a comment

alamb Mar 19, 2024

alamb Mar 19, 2024

tinfoil-knight Mar 20, 2024 •

edited

Loading

tinfoil-knight Mar 23, 2024

alamb Mar 19, 2024

Omega359 Mar 19, 2024

alamb Mar 19, 2024

Omega359 commented Mar 20, 2024

tinfoil-knight Mar 20, 2024 •

edited

Loading

alamb Mar 21, 2024

This comment was marked as outdated.

This comment was marked as outdated.

alamb left a comment

alamb Mar 21, 2024

Omega359 commented Mar 21, 2024 •

edited

Loading

alamb commented Mar 21, 2024

tinfoil-knight commented Mar 23, 2024

alamb left a comment

alamb Mar 23, 2024

tinfoil-knight Mar 23, 2024

alamb left a comment •

edited

Loading

improve null handling for to_char #9689

improve null handling for to_char #9689

Conversation

tinfoil-knight commented Mar 19, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinfoil-knight Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

tinfoil-knight commented Mar 19, 2024 • edited Loading

Omega359 commented Mar 19, 2024

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinfoil-knight Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Omega359 commented Mar 20, 2024

tinfoil-knight Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Omega359 commented Mar 21, 2024 • edited Loading

alamb commented Mar 21, 2024

tinfoil-knight commented Mar 23, 2024

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment • edited Loading

Choose a reason for hiding this comment

tinfoil-knight commented Mar 19, 2024 •

edited

Loading

tinfoil-knight Mar 20, 2024 •

edited

Loading

tinfoil-knight commented Mar 19, 2024 •

edited

Loading

tinfoil-knight Mar 20, 2024 •

edited

Loading

tinfoil-knight Mar 20, 2024 •

edited

Loading

Omega359 commented Mar 21, 2024 •

edited

Loading

alamb left a comment •

edited

Loading