[GitHub] [arrow-datafusion] viirya commented on a diff in pull request #5643: Support `date_bin` with 2 arguments

["viirya (via GitHub)" <gi...@apache.org>]

viirya commented on code in PR #5643:
URL: https://github.com/apache/arrow-datafusion/pull/5643#discussion_r1141476398


##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -783,19 +788,11 @@ mod tests {
         ]);
         assert!(res.is_ok());
 
-        //
-        // Fallible test cases
-        //

Review Comment:
   This is not only for "invalid number of arguments". All test cases under this section are fallible. We should keep it.



##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -783,19 +788,11 @@ mod tests {
         ]);
         assert!(res.is_ok());

Review Comment:
   We can add a test case verifying two-argument call equal to three-argument one with epoch.



##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -319,13 +319,18 @@ fn date_bin_single(stride: i64, source: i64, origin: i64) -> i64 {
 
 /// DATE_BIN sql function
 pub fn date_bin(args: &[ColumnarValue]) -> Result<ColumnarValue> {
-    if args.len() != 3 {
+    if args.len() != 2 && args.len() != 3 {
         return Err(DataFusionError::Execution(
-            "DATE_BIN expected three arguments".to_string(),
+            "DATE_BIN expected two or three arguments".to_string(),
         ));
     }
 
-    let (stride, array, origin) = (&args[0], &args[1], &args[2]);
+    let (stride, array) = (&args[0], &args[1]);
+    let epoch = &ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(
+        Some(0),
+        Some("+00:00".to_owned()),
+    ));
+    let origin = if args.len() == 2 { epoch } else { &args[2] };

Review Comment:
   We can put `epoch` inside `if` block. Otherwise we don't need it.



##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -783,19 +788,11 @@ mod tests {
         ]);
         assert!(res.is_ok());
 
-        //
-        // Fallible test cases
-        //
-
-        // invalid number of arguments
         let res = date_bin(&[
             ColumnarValue::Scalar(ScalarValue::IntervalDayTime(Some(1))),
             ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(Some(1), None)),
         ]);
-        assert_eq!(
-            res.err().unwrap().to_string(),
-            "Execution error: DATE_BIN expected three arguments"
-        );
+        assert!(res.is_ok());

Review Comment:
   I think we can/should still have test case of "invalid number of arguments".



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org