martin-g commented on code in PR #19442: URL: https://github.com/apache/datafusion/pull/19442#discussion_r2641218674
########## docs/source/user-guide/sql/scalar_functions.md: ########## @@ -2971,7 +2971,7 @@ Additional examples can be found [here](https://github.com/apache/datafusion/blo ### `to_unixtime` -Converts a value to seconds since the unix epoch (`1970-01-01T00:00:00Z`). Supports strings, dates, timestamps and double types as input. Strings are parsed as RFC3339 (e.g. '2023-07-20T05:44:00') if no [Chrono formats](https://docs.rs/chrono/latest/chrono/format/strftime/index.html) are provided. +Converts a value to seconds since the unix epoch (`1970-01-01T00:00:00Z`). Supports strings, dates, timestamps, integer, unsigned integer, and float types as input. Strings are parsed as RFC3339 (e.g. '2023-07-20T05:44:00') if no [Chrono formats](https://docs.rs/chrono/latest/chrono/format/strftime/index.html) are provided. Integers, unsigned integers, and floats are interpreted as seconds since the unix epoch (`1970-01-01T00:00:00Z`). Review Comment: ```suggestion Converts a value to seconds since the unix epoch (`1970-01-01T00:00:00`). Supports strings, dates, timestamps, integer, unsigned integer, and float types as input. Strings are parsed as RFC3339 (e.g. '2023-07-20T05:44:00') if no [Chrono formats](https://docs.rs/chrono/latest/chrono/format/strftime/index.html) are provided. Integers, unsigned integers, and floats are interpreted as seconds since the unix epoch (`1970-01-01T00:00:00`). ``` ########## datafusion/functions/src/datetime/to_unixtime.rs: ########## @@ -27,7 +27,7 @@ use std::any::Any; #[user_doc( doc_section(label = "Time and Date Functions"), - description = "Converts a value to seconds since the unix epoch (`1970-01-01T00:00:00Z`). Supports strings, dates, timestamps and double types as input. Strings are parsed as RFC3339 (e.g. '2023-07-20T05:44:00') if no [Chrono formats](https://docs.rs/chrono/latest/chrono/format/strftime/index.html) are provided.", + description = "Converts a value to seconds since the unix epoch (`1970-01-01T00:00:00Z`). Supports strings, dates, timestamps, integer, unsigned integer, and float types as input. Strings are parsed as RFC3339 (e.g. '2023-07-20T05:44:00') if no [Chrono formats](https://docs.rs/chrono/latest/chrono/format/strftime/index.html) are provided. Integers, unsigned integers, and floats are interpreted as seconds since the unix epoch (`1970-01-01T00:00:00Z`).", Review Comment: ```suggestion description = "Converts a value to seconds since the unix epoch (`1970-01-01T00:00:00`). Supports strings, dates, timestamps, integer, unsigned integer, and float types as input. Strings are parsed as RFC3339 (e.g. '2023-07-20T05:44:00') if no [Chrono formats](https://docs.rs/chrono/latest/chrono/format/strftime/index.html) are provided. Integers, unsigned integers, and floats are interpreted as seconds since the unix epoch (`1970-01-01T00:00:00`).", ``` The unix epoch is timezone independant. `Z` should be used in formats, not in values, too. ########## datafusion/sqllogictest/test_files/timestamps.slt: ########## @@ -3451,6 +3451,56 @@ select to_unixtime(arrow_cast(1599523200.414, 'Float64')); ---- 1599523200 +query I +select to_unixtime(arrow_cast(-1, 'Int8')); +---- +-1 + +query I +select to_unixtime(arrow_cast(1000, 'Int16')); +---- +1000 + +query I +select to_unixtime(arrow_cast(255, 'UInt8')); +---- +255 + +query I +select to_unixtime(arrow_cast(65535, 'UInt16')); +---- +65535 + +query I +select to_unixtime(arrow_cast(1599523200, 'UInt32')); +---- +1599523200 + +query I +select to_unixtime(arrow_cast(1599523200, 'UInt64')); +---- +1599523200 + +query I +select to_unixtime(arrow_cast(1000, 'Float16')); Review Comment: ```suggestion select to_unixtime(arrow_cast(1000.12, 'Float16')); ``` because it is a floating point integer ########## datafusion/functions/src/datetime/to_unixtime.rs: ########## Review Comment: this is not directly related to this PR ```suggestion // Format arguments only make sense for string inputs match arg_args[0].data_type() { DataType::Utf8View | DataType::LargeUtf8 | DataType::Utf8 => { validate_data_types(arg_args, "to_unixtime")?; } _ => { return exec_err\!("to_unixtime function only accepts format arguments with string input, got {} arguments", arg_args.len()); } } ``` If you agree with the suggestion then please also add some tests ########## datafusion/sqllogictest/test_files/timestamps.slt: ########## @@ -3451,6 +3451,56 @@ select to_unixtime(arrow_cast(1599523200.414, 'Float64')); ---- 1599523200 +query I +select to_unixtime(arrow_cast(-1, 'Int8')); +---- +-1 + +query I +select to_unixtime(arrow_cast(1000, 'Int16')); +---- +1000 + +query I +select to_unixtime(arrow_cast(255, 'UInt8')); +---- +255 + +query I +select to_unixtime(arrow_cast(65535, 'UInt16')); +---- +65535 + +query I +select to_unixtime(arrow_cast(1599523200, 'UInt32')); +---- +1599523200 + +query I +select to_unixtime(arrow_cast(1599523200, 'UInt64')); Review Comment: (Also) test with a u64 value that is bigger than i64::MAX ########## datafusion/sqllogictest/test_files/timestamps.slt: ########## @@ -3451,6 +3451,56 @@ select to_unixtime(arrow_cast(1599523200.414, 'Float64')); ---- 1599523200 Review Comment: test `Null`: `select to_unixtime(arrow_cast(null, 'Int8'))` -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
