kosiew commented on code in PR #26127:
URL: https://github.com/apache/datafusion/pull/26127#discussion_r4357745371
##########
datafusion/spark/src/function/datetime/next_day.rs:
##########
@@ -41,8 +42,23 @@ impl Default for SparkNextDay {
impl SparkNextDay {
pub fn new() -> Self {
Self {
- signature: Signature::exact(
- vec![DataType::Date32, DataType::Utf8],
+ // Spark casts the start date to DATE, so STRING and TIMESTAMP are
accepted
+ signature: Signature::coercible(
+ vec![
+ Coercion::new_implicit(
+ TypeSignatureClass::Native(logical_date()),
+ vec![
+ TypeSignatureClass::Native(logical_string()),
+ TypeSignatureClass::Timestamp,
+ ],
+ NativeType::Date,
+ ),
+ Coercion::new_implicit(
+ TypeSignatureClass::Native(logical_string()),
+ vec![TypeSignatureClass::Numeric],
Review Comment:
The new coercible signature rejects weekday inputs previously accepted
through implicit string coercion: `next_day(DATE '2015-07-23',
arrow_cast('Mon', 'Binary'))` previously returned `2015-07-27`, while Boolean
inputs previously produced NULL rather than planning errors. Please preserve
these conversions and add a Binary regression test, or justify the intentional
compatibility change with reference-backed tests, without changing shared
coercion policy.
##########
datafusion/sqllogictest/test_files/spark/datetime/next_day.slt:
##########
@@ -100,3 +100,52 @@ query I
SELECT arrow_cast(next_day(arrow_cast(95026230, 'Date32'), 'Tue'::string),
'Int32');
----
95026237
+
+# https://github.com/apache/datafusion/issues/23889
+# Spark casts the start date to DATE, so STRING and TIMESTAMP are accepted
+query D
+SELECT next_day('2015-07-23'::string, 'Mon'::string);
+----
+2015-07-27
+
+query D
+SELECT next_day('2015-07-23 12:12:12'::string, 'Mon'::string);
+----
+2015-07-27
+
+query D
+SELECT next_day('2015-07-23 12:12:12'::timestamp, 'Mon'::string);
+----
+2015-07-27
+
+query D
+SELECT next_day(a, 'Mon'::string)
+FROM VALUES
+ ('2015-07-23'::string),
+ (NULL::string) as t(a);
+----
+2015-07-27
+NULL
+
+# Timestamp column with a day-of-week column
+query D
+SELECT next_day(a, b)
+FROM VALUES
+ ('2015-07-23 12:12:12'::timestamp, 'Mon'::string),
+ ('2015-07-23 23:59:59'::timestamp, 'Fri'::string),
+ (NULL::timestamp, 'Mon'::string) as t(a, b);
+----
+2015-07-27
+2015-07-24
+NULL
+
+# String start date and LargeUtf8 / Utf8View day-of-week
+query D
+SELECT next_day('2015-07-23', arrow_cast('Mon', 'LargeUtf8'));
Review Comment:
The new string-layout tests cover the weekday argument but not the newly
accepted start-date argument. Consider adding `LargeUtf8` and `Utf8View` start
dates using `arrow_cast('2015-07-23', ...)`, both expecting `2015-07-27`, to
protect the logical-string-to-Date32 coercion.
##########
datafusion/sqllogictest/test_files/spark/datetime/next_day.slt:
##########
@@ -100,3 +100,52 @@ query I
SELECT arrow_cast(next_day(arrow_cast(95026230, 'Date32'), 'Tue'::string),
'Int32');
----
95026237
+
+# https://github.com/apache/datafusion/issues/23889
+# Spark casts the start date to DATE, so STRING and TIMESTAMP are accepted
+query D
+SELECT next_day('2015-07-23'::string, 'Mon'::string);
+----
+2015-07-27
+
+query D
+SELECT next_day('2015-07-23 12:12:12'::string, 'Mon'::string);
+----
+2015-07-27
+
+query D
+SELECT next_day('2015-07-23 12:12:12'::timestamp, 'Mon'::string);
Review Comment:
Consider adding `next_day(TIMESTAMP '1969-12-31 23:59:59', 'Thu')`,
expecting `1970-01-01`, to cover timestamp-to-date conversion before the Unix
epoch. The current implementation already handles this correctly, but
truncation toward epoch zero could incorrectly produce `1970-01-08`; this is
optional integration coverage.
--
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]
