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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
