namanjain24-sudo commented on code in PR #25498:
URL: https://github.com/apache/datafusion/pull/25498#discussion_r4062452547


##########
datafusion/functions/src/datetime/date_trunc.rs:
##########
@@ -1441,6 +1468,98 @@ mod tests {
         }
     }
 
+    /// Evaluates `date_trunc(granularity, value)` once on a scalar and once 
on a
+    /// one-row column holding the same value.
+    fn date_trunc_scalar_and_array(
+        granularity: &str,
+        value: ScalarValue,
+    ) -> (
+        datafusion_common::Result<ScalarValue>,
+        datafusion_common::Result<ScalarValue>,
+    ) {
+        let invoke = |arg: ColumnarValue| {
+            let data_type = value.data_type();
+            let args = ScalarFunctionArgs {
+                args: 
vec![ColumnarValue::Scalar(ScalarValue::from(granularity)), arg],
+                arg_fields: vec![
+                    Field::new("a", DataType::Utf8, false).into(),
+                    Field::new("b", data_type.clone(), true).into(),
+                ],
+                number_rows: 1,
+                return_field: Field::new("f", data_type, true).into(),
+                config_options: Arc::new(ConfigOptions::default()),
+            };
+            match DateTruncFunc::new().invoke_with_args(args)? {
+                ColumnarValue::Scalar(result) => Ok(result),
+                ColumnarValue::Array(result) => 
ScalarValue::try_from_array(&result, 0),
+            }
+        };
+        (
+            invoke(ColumnarValue::Scalar(value.clone())),
+            invoke(ColumnarValue::Array(value.to_array().unwrap())),
+        )
+    }
+
+    /// A timestamp beyond the nanosecond range is truncated the same way as a
+    /// scalar and as a column: neither converts it to nanoseconds first.
+    #[test]
+    fn scalar_and_array_accept_timestamps_beyond_nanosecond_range() {
+        // 2286-11-20T17:46:40, after the last nanosecond timestamp in 2262
+        let seconds = 10_000_000_000;
+        let utc: Option<Arc<str>> = Some("UTC".into());
+        let cases = [
+            (
+                ScalarValue::TimestampSecond(Some(seconds + 59), None),
+                "minute",
+            ),
+            (
+                ScalarValue::TimestampSecond(Some(seconds + 1), None),
+                "hour",
+            ),
+            (ScalarValue::TimestampSecond(Some(seconds + 1), None), "day"),
+            (ScalarValue::TimestampSecond(Some(seconds), None), "second"),
+            (
+                ScalarValue::TimestampSecond(Some(seconds + 59), utc.clone()),
+                "minute",
+            ),
+            (
+                ScalarValue::TimestampMillisecond(Some(seconds * 1_000 + 999), 
None),
+                "second",
+            ),
+            (
+                ScalarValue::TimestampMicrosecond(Some(seconds * 1_000_000 + 
999), None),
+                "millisecond",
+            ),
+            (
+                ScalarValue::TimestampMicrosecond(Some(-seconds * 1_000_000 - 
1), utc),
+                "second",
+            ),
+        ];

Review Comment:
   Thanks, agreed. In 6a213a76f each group of cases in 
`scalar_and_array_accept_timestamps_beyond_nanosecond_range` now has a short 
comment saying what it covers (the input unit and granularity, the case where 
the granularity is the input's own unit, the time zone, and the pre-epoch 
value). It's comments only, no change to the cases or the assertions. I ran the 
`date_trunc` unit tests, `cargo fmt --check` and clippy on 
`datafusion-functions` locally.
   



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

Reply via email to