Add `to_date` function #9019

Tangruilin · 2024-01-27T09:59:01Z

Which issue does this PR close?

Closes #8987 .

Rationale for this change

What changes are included in this PR?

Add to_date function, and change the protobuf file

Are these changes tested?

Not now, but it should be added in this PR.

Are there any user-facing changes?

Add a function named to_date, users can use select to_date(1243124); to try it.

Tangruilin · 2024-01-27T10:01:41Z

datafusion/proto/src/logical_plan/from_proto.rs

@@ -1813,6 +1814,7 @@ pub fn parse_expr(
                ScalarFunction::StructFun => {
                    Ok(struct_fun(parse_expr(&args[0], registry)?))
                }
+                ScalarFunction::ToDate => Ok(struct_fun(parse_expr(&args[0], registry)?)), //TODO: ensure how to fill it


Here, I am not sure how to do with it, I need some suggestion

Tangruilin · 2024-01-27T10:05:24Z

@Omega359
Hey! you can look it if you have time, I need you suggestion about the protobuf file and if there are something that i do not think about.

If it looks ok, I will write test code these days.

Thanks very much

Omega359 · 2024-01-27T15:01:26Z

I haven't had time to look it over yet but first thing I did see was the use of Date64 - I can't see any good reason to use that vs Date32

Tangruilin · 2024-01-28T04:19:47Z

I haven't had time to look it over yet but first thing I did see was the use of Date64 - I can't see any good reason to use that vs Date32

Date64 contains more number in my opinion. Maybe Date32 is better?

Weijun-H · 2024-01-28T05:07:06Z

datafusion/proto/src/logical_plan/from_proto.rs

@@ -1813,6 +1814,7 @@ pub fn parse_expr(
                ScalarFunction::StructFun => {
                    Ok(struct_fun(parse_expr(&args[0], registry)?))
                }
+                ScalarFunction::ToDate => Ok(struct_fun(parse_expr(&args[0], registry)?)), //TODO: ensure how to fill it


It should be

Suggested change

ScalarFunction::ToDate => Ok(struct_fun(parse_expr(&args[0], registry)?)), //TODO: ensure how to fill it

ScalarFunction::ToDate => Ok(to_date(parse_expr(&args[0], registry)?)),

Weijun-H · 2024-01-28T05:09:30Z

datafusion/physical-expr/src/datetime_expressions.rs

+// fn to_date_impl<T: ArrowTimestampType + ScalarType<i64>>(
+//     args: &[ColumnarValue],
+// ) -> Result<ColumnarValue> {
+// }
+


Suggested change

// fn to_date_impl<T: ArrowTimestampType + ScalarType<i64>>(

// args: &[ColumnarValue],

// ) -> Result<ColumnarValue> {

// }

Omega359 · 2024-01-28T15:36:49Z

Date64 contains more number in my opinion. Maybe Date32 is better?

    /// A signed 32-bit date representing the elapsed time since UNIX epoch (1970-01-01)
    /// in days (32 bits).
    Date32,
    /// A signed 64-bit date representing the elapsed time since UNIX epoch (1970-01-01)
    /// in milliseconds (64 bits). Values are evenly divisible by 86400000.
    Date64,

Date32 is the way.

Tangruilin · 2024-01-29T13:31:32Z

Date64 contains more number in my opinion. Maybe Date32 is better?

    /// A signed 32-bit date representing the elapsed time since UNIX epoch (1970-01-01)
    /// in days (32 bits).
    Date32,
    /// A signed 64-bit date representing the elapsed time since UNIX epoch (1970-01-01)
    /// in milliseconds (64 bits). Values are evenly divisible by 86400000.
    Date64,

Date32 is the way.

Okey! I will change it to Date32
I may finish it these two days

Tangruilin · 2024-01-30T13:34:58Z

@alamb @Omega359 This PR is ready to be reviewed

Weijun-H

Thanks @Tangruilin I left some minor suggestions.

Weijun-H · 2024-01-31T02:30:45Z

datafusion/physical-expr/src/datetime_expressions.rs

@@ -1308,6 +1349,40 @@ fn validate_to_timestamp_data_types(
    None
 }

+// TODO: 实现这个函数


Suggested change

// TODO: 实现这个函数

Weijun-H · 2024-01-31T02:31:23Z

datafusion/physical-expr/src/datetime_expressions.rs

+/// to_date SQL function implementation
+pub fn to_date_invoke(args: &[ColumnarValue]) -> Result<ColumnarValue> {
+    if args.is_empty() {
+        return internal_err!(


Suggested change

return internal_err!(

return exec_err!(

it seems that other functions are all using internal_err. If i need to change all of them？

Based on #8781 (comment), I vote for exec_err

internal_err: something that wasn't expected/anticipated by the implementation and that is most likely a bug (the error message even encourages users to open a bug report). I user should not be able to trigger this under normal circumstances. Note that I/O errors (or any error that happens due to external systems) do NOT fall under this category (there's an external error variant for that)

exec_err: a processing error that happens during execution, e.g. the user passed malformed arguments to a SQL method, opened a CSV file that is broken, tried to divide an integer by zero. these errors shouldn't happen for a "good" query + "good" data, but since both the query and the data can easily be broken or misaligned, these are common occurrences in ETL systems / databases.

So need I change internal_err in to_timestamp_invoke to exec_err?
Or it should be done in another PR?

A follow on pr is better

Weijun-H · 2024-01-31T02:37:33Z

datafusion/physical-expr/src/datetime_expressions.rs

+        DataType::Int32 | DataType::Int64 => {
+            cast_column(&args[0], &DataType::Date32, None)
+        }
+        DataType::Null | DataType::Float64 => {
+            cast_column(&args[0], &DataType::Date32, None)
+        }
+        DataType::Date32 | DataType::Date64 => {
+            cast_column(&args[0], &DataType::Date32, None)
+        }


Suggested change

DataType::Int32 | DataType::Int64 => {

cast_column(&args[0], &DataType::Date32, None)

}

DataType::Null | DataType::Float64 => {

cast_column(&args[0], &DataType::Date32, None)

}

DataType::Date32 | DataType::Date64 => {

cast_column(&args[0], &DataType::Date32, None)

}

DataType::Int32

| DataType::Int64

| DataType::Null

| DataType::Float64

| DataType::Date32

| DataType::Date64 => cast_column(&args[0], &DataType::Date32, None),

Weijun-H · 2024-01-31T02:45:27Z

datafusion/sqllogictest/test_files/dates.slt

+SELECT to_date('01-14-2023 01:01:30+05:30', '%q', '%d-%m-%Y %H/%M/%S', '%+', '%m-%d-%Y %H:%M:%S%#z');
+----
+2023-01-13
+


add a test for wrong input and return NULL

Now the wrong input will return error but not NULL like to_timestamp

Weijun-H · 2024-01-31T02:47:49Z

datafusion/physical-expr/src/datetime_expressions.rs

+            |s, format| {
+                string_to_timestamp_nanos_formatted(s, format)
+                    .map(|n| {
+                        println!("{n}");


Suggested change

println!("{n}");

Weijun-H · 2024-01-31T02:51:10Z

datafusion/physical-expr/src/datetime_expressions.rs

+                    })
+                    .and_then(|v| {
+                        v.try_into().map_err(|_| {
+                            DataFusionError::NotImplemented("()".to_string())


Suggested change

DataFusionError::NotImplemented("()".to_string())

internal_datafusion_err!("Unable to cast to Date32")

Weijun-H · 2024-01-31T02:51:29Z

datafusion/physical-expr/src/datetime_expressions.rs

+                    .map(|n| n / (1_000_000 * 24 * 60 * 60 * 1_000))
+                    .and_then(|v| {
+                        v.try_into().map_err(|_| {
+                            DataFusionError::NotImplemented("()".to_string())


Suggested change

DataFusionError::NotImplemented("()".to_string())

internal_datafusion_err!("Unable to cast to Date32")

Tangruilin · 2024-01-31T05:11:29Z

datafusion/sqllogictest/test_files/dates.slt

+2023-01-13
+
+statement error DataFusion error: Internal error: to_date function unsupported data type at index 1: List
+SELECT to_date('2022-08-03T14:38:50+05:30', make_array('%s', '%q', '%d-%m-%Y %H:%M:%S%#z', '%+'));


refer to this, this is a example for wrong input

I think there shjould be more tests of just dates without time or tz component. Need test to verify null, invalid formats, etc.

Can we also please add a test for invalid dates like

to_date('21311111')

I think the tests here could be copied and adapted to cover to_date quite well:

https://github.com/apache/arrow-datafusion/blob/7641a3228156aab0e48c4bab5a6834b44f722d89/datafusion/sqllogictest/test_files/timestamps.slt#L2073-L2229

Weijun-H · 2024-01-31T08:17:38Z

datafusion/expr/src/expr_fn.rs

+nary_scalar_expr!(
+    ToDate,
+    to_date,
+    "converts a string and optional formats to a `Date32`"


Suggested change

"converts a string and optional formats to a `Date32`"

"converts string to date according to the given format"

Weijun-H · 2024-01-31T08:48:06Z

datafusion/physical-expr/src/datetime_expressions.rs

+        1 => handle::<Date32Type, _, Date32Type>(
+            args,
+            |s| {
+                string_to_timestamp_nanos_shim(s)
+                    .map(|n| n / (1_000_000 * 24 * 60 * 60 * 1_000))
+                    .and_then(|v| {
+                        v.try_into().map_err(|_| {
+                            internal_datafusion_err!("Unable to cast to Date32 for converting from i64 to i32 failed")
+                        })
+                    })
+            },
+            name,
+        ),


Maybe we can use to_timestamp_impl to avoid code duplication.

Suggested change

1 => handle::<Date32Type, _, Date32Type>(

args,

|s| {

string_to_timestamp_nanos_shim(s)

.map(|n| n / (1_000_000 * 24 * 60 * 60 * 1_000))

.and_then(|v| {

v.try_into().map_err(|_| {

internal_datafusion_err!("Unable to cast to Date32 for converting from i64 to i32 failed")

})

})

},

name,

),

1 => to_timestamp_impl::<TimestampNanosecondType>(args, name),

We can not do that

for the reason the Date32 need i32 type and time_stamp need i64 type.

to_timestamp_impl did not deal with the converting from i64 to i32

Tangruilin · 2024-02-20T06:30:00Z

@Tangruilin - Do you think you'll have time to complete this PR before v36 is released?

You can review it now

Tangruilin · 2024-02-21T02:12:49Z

@Omega359
You can review this PR now, thanks

alamb · 2024-02-21T07:45:51Z

@Omega359 You can review this PR now, thanks

@Omega359 once you have reviewed this PR please let me know and I'll give it a final look too and hopefully merge it

Thank you for your work @Tangruilin

Omega359

Thanks for you work on this @Tangruilin! I left a few comments, mostly minor but 1 issue around the rustdoc where it'll unfortunately have to have 'ignore' added. You have some repeated tests still in dates.slt that should be cleaned up then I believe it's ready to go.

Omega359 · 2024-02-21T13:17:05Z

datafusion-examples/examples/to_date.rs

+use datafusion::error::Result;
+use datafusion::prelude::*;
+
+/// This example demonstrates how to use the to_timestamp series


to_timestamp -> to_date

Suggested change

/// This example demonstrates how to use the to_timestamp series

/// This example demonstrates how to use the to_date series

Omega359 · 2024-02-21T13:17:47Z

datafusion-examples/examples/to_date.rs

+    ctx.register_batch("t", batch)?;
+    let df = ctx.table("t").await?;
+
+    // use to_timestamp function to convert col 'a' to timestamp type using the default parsing


to_timestamp -> to_date, convert 'a to date type

Suggested change

// use to_timestamp function to convert col 'a' to timestamp type using the default parsing

// use to_date function to convert col 'a' to timestamp type using the default parsing

Omega359 · 2024-02-21T13:20:45Z

datafusion/physical-expr/src/datetime_expressions.rs

+/// use datafusion::arrow::datatypes::{DataType, Field, Schema};
+/// use datafusion::arrow::record_batch::RecordBatch;
+/// use datafusion::error::Result;
+/// use datafusion::prelude::*;


Unfortunately we'll have to add an ignore for this because of #9277 See https://github.com/apache/arrow-datafusion/pull/9279/files#diff-67ae8808785b2e651767d7ff67dd7c53be04ca098857b52c82ed19927e071cdaR514 for an example. I believe once the functions are move to the new dataframe-functions crate we should be able to re-enable.

Omega359 · 2024-02-21T13:21:15Z

datafusion/physical-expr/src/datetime_expressions.rs

+///     ctx.register_batch("t", batch)?;
+///     let df = ctx.table("t").await?;
+
+///     // use to_timestamp function to convert col 'a' to timestamp type using the default parsing


timestamp => date

Omega359 · 2024-02-21T13:22:25Z

datafusion/physical-expr/src/datetime_expressions.rs

+            |n| n,
+            "to_date",
+        ),
+        _ => internal_err!("Unsupported 0 argument count for function to_date"),


This should be an exec_err (See #9241 for details)

Omega359 · 2024-02-21T13:22:59Z

datafusion/physical-expr/src/datetime_expressions.rs

+        | DataType::Date64 => cast_column(&args[0], &DataType::Date32, None),
+        DataType::Utf8 => to_date(args),
+        other => {
+            internal_err!("Unsupported data type {:?} for function to_date", other)


Tangruilin · 2024-02-27T16:40:39Z

@Omega359 The cargo test --doc command is passed on my mac.
But the CI fail. I do not know why

Omega359 · 2024-02-27T17:35:55Z

@Omega359 The cargo test --doc command is passed on my mac. But the CI fail. I do not know why

I think it's because you are attempting to include /// # use datafusion::prelude::*; which isn't imported in that module. You can't import it either because of #9277. As mentioned in my review you'll have to ignore that doc test until we move the functions out of this module to the new datafusion/functions area.

alamb

I think this PR looks good to me -- thank you @Tangruilin and @Omega359

There are several cleanup suggestions left outstanding from @Omega359 related to fixing up repeated tests / comments. Is this something you have time to fix @Tangruilin before we merge the PR? Otherwise we can do it as a follow on PR

alamb · 2024-02-28T12:27:03Z

datafusion-examples/examples/to_date.rs

+use datafusion::error::Result;
+use datafusion::prelude::*;
+
+/// This example demonstrates how to use the to_timestamp series


Suggested change

/// This example demonstrates how to use the to_timestamp series

/// This example demonstrates how to use the to_date series

alamb · 2024-02-28T12:27:10Z

datafusion-examples/examples/to_date.rs

+    ctx.register_batch("t", batch)?;
+    let df = ctx.table("t").await?;
+
+    // use to_timestamp function to convert col 'a' to timestamp type using the default parsing


Suggested change

// use to_timestamp function to convert col 'a' to timestamp type using the default parsing

// use to_date function to convert col 'a' to timestamp type using the default parsing

Tangruilin · 2024-02-28T16:28:34Z

I think this PR looks good to me -- thank you @Tangruilin and @Omega359

There are several cleanup suggestions left outstanding from @Omega359 related to fixing up repeated tests / comments. Is this something you have time to fix @Tangruilin before we merge the PR? Otherwise we can do it as a follow on PR

I think i can fix it in this PR

Tangruilin · 2024-02-28T17:25:34Z

I think this PR looks good to me -- thank you @Tangruilin and @Omega359
There are several cleanup suggestions left outstanding from @Omega359 related to fixing up repeated tests / comments. Is this something you have time to fix @Tangruilin before we merge the PR? Otherwise we can do it as a follow on PR

I think i can fix it in this PR

Done

Omega359 · 2024-02-28T17:38:35Z

datafusion/sqllogictest/test_files/dates.slt

+# to_date with invalid formatting
+query error input contains invalid characters
+SELECT to_date('2020-09-08 12/00/00+00:00', '%c', '%+')
+
+# to_date with invalid formatting
+query error input contains invalid characters
+SELECT to_date('2020-09-08 12/00/00+00:00', '%c', '%+')
+
+# to_date with invalid formatting
+query error input contains invalid characters
+SELECT to_date('2020-09-08 12/00/00+00:00', '%c', '%+')
+
+# to_date with invalid formatting
+query error input contains invalid characters
+SELECT to_date('2020-09-08 12/00/00+00:00', '%c', '%+')
+
+# to_date with broken formatting
+query error bad or unsupported format string
+SELECT to_date('2020-09-08 12/00/00+00:00', '%q')


Suggested change

# to_date with invalid formatting

query error input contains invalid characters

SELECT to_date('2020-09-08 12/00/00+00:00', '%c', '%+')

# to_date with invalid formatting

query error input contains invalid characters

SELECT to_date('2020-09-08 12/00/00+00:00', '%c', '%+')

# to_date with invalid formatting

query error input contains invalid characters

SELECT to_date('2020-09-08 12/00/00+00:00', '%c', '%+')

# to_date with invalid formatting

query error input contains invalid characters

SELECT to_date('2020-09-08 12/00/00+00:00', '%c', '%+')

# to_date with broken formatting

query error bad or unsupported format string

SELECT to_date('2020-09-08 12/00/00+00:00', '%q')

Omega359 · 2024-02-28T17:39:38Z

datafusion/sqllogictest/test_files/dates.slt

+# to_date with broken formatting
+query error bad or unsupported format string
+SELECT to_date('2020-09-08 12/00/00+00:00', '%q')
+
+# to_date with broken formatting
+query error bad or unsupported format string
+SELECT to_date('2020-09-08 12/00/00+00:00', '%q')
+
+# to_date with broken formatting
+query error bad or unsupported format string
+SELECT to_date('2020-09-08 12/00/00+00:00', '%q')


Suggested change

# to_date with broken formatting

query error bad or unsupported format string

SELECT to_date('2020-09-08 12/00/00+00:00', '%q')

# to_date with broken formatting

query error bad or unsupported format string

SELECT to_date('2020-09-08 12/00/00+00:00', '%q')

# to_date with broken formatting

query error bad or unsupported format string

SELECT to_date('2020-09-08 12/00/00+00:00', '%q')

Done，you can review

Signed-off-by: tangruilin <tang.ruilin@foxmail.com>

alamb

Thanks again @Tangruilin and @Omega359 -- the feature and the review is really appreciated. Nice work

github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt) labels Jan 27, 2024

Tangruilin commented Jan 27, 2024

View reviewed changes

Tangruilin marked this pull request as draft January 27, 2024 10:02

Weijun-H reviewed Jan 28, 2024

View reviewed changes

Tangruilin force-pushed the task#8987 branch 2 times, most recently from 474fb34 to 1a8ff01 Compare January 30, 2024 13:26

Tangruilin marked this pull request as ready for review January 30, 2024 13:27

Tangruilin force-pushed the task#8987 branch from 1a8ff01 to 30eaae6 Compare January 30, 2024 13:33

alamb mentioned this pull request Jan 30, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 29, 2024 #9030

Closed

6 tasks

Tangruilin force-pushed the task#8987 branch from 30eaae6 to 55c50db Compare January 31, 2024 01:56

Weijun-H reviewed Jan 31, 2024

View reviewed changes

Tangruilin force-pushed the task#8987 branch 2 times, most recently from 740e38c to da55c5d Compare January 31, 2024 03:52

Tangruilin commented Jan 31, 2024

View reviewed changes

Tangruilin force-pushed the task#8987 branch 2 times, most recently from d141650 to dc8d355 Compare January 31, 2024 05:15

Tangruilin requested review from Weijun-H January 31, 2024 06:13

Weijun-H reviewed Jan 31, 2024

View reviewed changes

Tangruilin force-pushed the task#8987 branch 2 times, most recently from 0eade48 to bbf1b36 Compare February 1, 2024 04:03

Tangruilin requested review from Omega359 and alamb February 19, 2024 08:03

Tangruilin marked this pull request as ready for review February 19, 2024 08:03

Tangruilin force-pushed the task#8987 branch from da4d780 to 7290549 Compare February 19, 2024 14:32

Omega359 reviewed Feb 21, 2024

View reviewed changes

Tangruilin force-pushed the task#8987 branch 5 times, most recently from e5adc84 to 4262b5c Compare February 27, 2024 16:14

github-actions bot added the core Core DataFusion crate label Feb 27, 2024

Tangruilin force-pushed the task#8987 branch from 4262b5c to b65125a Compare February 28, 2024 10:59

alamb approved these changes Feb 28, 2024

View reviewed changes

Tangruilin force-pushed the task#8987 branch 2 times, most recently from b7e202f to da8c3b7 Compare February 28, 2024 16:54

github-actions bot removed the core Core DataFusion crate label Feb 28, 2024

Omega359 reviewed Feb 28, 2024

View reviewed changes

Tangruilin force-pushed the task#8987 branch from da8c3b7 to 4f484c9 Compare February 29, 2024 01:59

[task apache#8987]add_to_date_function

8ec5100

Signed-off-by: tangruilin <tang.ruilin@foxmail.com>

Tangruilin force-pushed the task#8987 branch from 4f484c9 to 8ec5100 Compare February 29, 2024 02:00

alamb approved these changes Feb 29, 2024

View reviewed changes

alamb merged commit de902de into apache:main Feb 29, 2024
23 checks passed

	ScalarFunction::ToDate => Ok(struct_fun(parse_expr(&args[0], registry)?)), //TODO: ensure how to fill it
	ScalarFunction::ToDate => Ok(to_date(parse_expr(&args[0], registry)?)),

	DataFusionError::NotImplemented("()".to_string())
	internal_datafusion_err!("Unable to cast to Date32")

	"converts a string and optional formats to a `Date32`"
	"converts string to date according to the given format"

	/// This example demonstrates how to use the to_timestamp series
	/// This example demonstrates how to use the to_date series

	// use to_timestamp function to convert col 'a' to timestamp type using the default parsing
	// use to_date function to convert col 'a' to timestamp type using the default parsing

Add to_date function #9019

Add to_date function #9019

Conversation

Tangruilin commented Jan 27, 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

Tangruilin commented Jan 27, 2024

Omega359 commented Jan 27, 2024

Tangruilin commented Jan 28, 2024

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 Jan 28, 2024

Tangruilin commented Jan 29, 2024

Tangruilin commented Jan 30, 2024

Weijun-H left a 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

Choose a reason for hiding this comment

Weijun-H Jan 31, 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

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

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

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

Tangruilin commented Feb 20, 2024

Tangruilin commented Feb 21, 2024

alamb commented Feb 21, 2024

Omega359 left a 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

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

Tangruilin commented Feb 27, 2024

Omega359 commented Feb 27, 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

Tangruilin commented Feb 28, 2024

Tangruilin commented Feb 28, 2024

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

Choose a reason for hiding this comment

Add `to_date` function #9019

Add `to_date` function #9019

Tangruilin commented Jan 27, 2024 •

edited

Loading

Weijun-H Jan 31, 2024 •

edited

Loading